CODEWITHSUNDEEP
f#
Thomas
Thomas

Reputation: 12087

How do I reduce code duplication with nested 'if' statements?

let's consider this code:

let getBuildDate (assembly: Assembly) : DateTime option =

    let buildVersionMetadataPrefix = "+build"
    let attribute = assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()

    if attribute <> null && attribute.InformationalVersion <> null then
        let value = attribute.InformationalVersion
        let index = value.IndexOf(buildVersionMetadataPrefix)
        if index > 0 then
            let value = value.Substring(index + buildVersionMetadataPrefix.Length)
            let success, timestamp = DateTime.TryParseExact(value, "yyyyMMddHHmmss", CultureInfo.InvariantCulture, DateTimeStyles.None)
            if success then
                Some timestamp
            else
                None
        else
            None
    else
        None

Is there a way to get rid of all the 'else None' statements to have only one?

On one side, I can imagine that for some people the code is more clear with all the None statements spelled out, but on the other side, coming from the C world, I see it as clutter that reduces readability.

There are many cases where you need a series of conditions to be met and all the failed cases go to one place.

If I have a list of conditions that depend on each others' success, how can I make a concise short exit without duplication.

Upvotes: 6

Views: 1278

Answers (6)

imjoseangel
imjoseangel

Reputation: 3926

There is an approach that I really love which is the use of an array in certain scenarios.

Example:

Instead of using something like:

    if (grade >= 90) {
        scale = "A";
    } else if (grade >= 80) {
        scale = "B";
    } else if (grade >= 70) {
        scale = "C";
    } else if (grade >= 60) {
        scale = "D";
    } else {
        scale = "F";
    }

Use an array like:

function calculate(scores) {
    var grade, scale;

    let sum = 0;

    for (let i = 0; i < scores.length; i++) {
        sum += scores[i];
    }

    grade = sum / scores.length;
    scale = {
        [90 <= grade && grade <= 100]: "O",
        [80 <= grade && grade < 90]: "E",
        [70 <= grade && grade < 80]: "A",
        [55 <= grade && grade < 70]: "P",
        [40 <= grade && grade < 55]: "D",
        [grade < 40]: "T"
    };
    console.log(scale.true);
}

In python could be like:

def calculate(scores: list) -> str:
    grade = sum(scores) / len(scores)
    print(grade)
    scale = {90 <= grade <= 100: "O", 80 <=
                grade < 90: "E", 70 <= grade < 80: "A",
                55 <= grade < 70: "P", 40 <= grade < 55: "D",
                grade < 40: "T"}
    return scale.get(True)

Upvotes: 0

user1562155
user1562155

Reputation:

Have you considered using Result<TSuccess, TError>. It is very structuring - making the code rigid and flat - and makes it possible to provide detailed error information for the step that possible fails. It's a little more code, but IMO more readable and maintainable:

let getBuildDate (assembly: Assembly) : Result<DateTime, string> = 

    let buildVersionMetadataPrefix = "+build"

    let extractAttribute (assem: Assembly) = 
        match assem.GetCustomAttribute<AssemblyInformationalVersionAttribute>() with
        | attrib when attrib <> null -> Ok attrib
        | _ -> Error "No attribute found"

    let extractDateString (attrib: AssemblyInformationalVersionAttribute) =
        match attrib.InformationalVersion.IndexOf (buildVersionMetadataPrefix) with
        | x when x > 0 -> Ok (attrib.InformationalVersion.Substring (x + buildVersionMetadataPrefix.Length))
        | _ -> Error "Metadata prefix not found"

    let toDateTime dateString =
        match DateTime.TryParseExact(dateString, "yyyyMMddHHmmss", CultureInfo.InvariantCulture, DateTimeStyles.None) with
        | true, timeStamp -> Ok timeStamp
        | false, _ -> Error "Invalid date time format"

    extractAttribute assembly
    |> Result.bind extractDateString
    |> Result.bind toDateTime

Usage

let optBuildDate = getBuildDate (Assembly.GetExecutingAssembly())
match optBuildDate with
| Ok date -> printfn "%A" date
| Error msg -> printfn "ERROR: %s" msg

Upvotes: 2

Tomas Petricek
Tomas Petricek

Reputation: 243041

The other answers show how to do this using more sophisticated functional programming methods, like using computation expressions or option values. Those are definitely useful and make sense if this is something that you are doing in many places throughout your system.

However, if you just want a simple way to change the code so that the control flow is more clear (without making it more clever), I would negate the conditions. Previously, you had:

if something then 
  moreStuff()
  Some result
else
  None

You can rewrite this by returning None if not something. I think the F# coding convention in this case also allows you to remove the indentation, so it looks more like imperative early return:

if not something then None else
moreStuff()
Some result

With this, you can write your original function as follows - without any extra clever tricks:

let getBuildDate (assembly: Assembly) : DateTime option =

    let buildVersionMetadataPrefix = "+build"
    let attribute = assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()

    if attribute = null || attribute.InformationalVersion = null then None else
    let value = attribute.InformationalVersion
    let index = value.IndexOf(buildVersionMetadataPrefix)
    if index <= 0 then None else
    let value = value.Substring(index + buildVersionMetadataPrefix.Length)
    let success, timestamp = DateTime.TryParseExact(value, "yyyyMMddHHmmss", CultureInfo.InvariantCulture, DateTimeStyles.None)
    if not success then None else
    Some timestamp

Upvotes: 6

Marko Grdinić
Marko Grdinić

Reputation: 4062

open System
open System.Reflection
open System.Globalization

let inline guard cond next = if cond then next () else None

let getBuildDate (assembly: Assembly) : DateTime option =
    let buildVersionMetadataPrefix = "+build"
    let attribute = assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()

    guard (attribute <> null && attribute.InformationalVersion <> null) <| fun _ ->
    let value = attribute.InformationalVersion
    let index = value.IndexOf(buildVersionMetadataPrefix)
    guard (index > 0) <| fun _ ->
    let value = value.Substring(index + buildVersionMetadataPrefix.Length)
    let success, timestamp = DateTime.TryParseExact(value, "yyyyMMddHHmmss", CultureInfo.InvariantCulture, DateTimeStyles.None)
    guard success <| fun _ ->
    Some timestamp

If you can stomach the inelegance of having to write <| fun _ -> on every guard, this is an option worth considering.

Upvotes: 1

Asti
Asti

Reputation: 12667

A readable approach might be use a computation expression builder for Option.

type OptionBuilder() =
    member _.Return v = Some v
    member _.Zero () = None
    member _.Bind(v, f) = Option.bind f v
    member _.ReturnFrom o = o

let opt = OptionBuilder()

You can simulate an imperative style of if-then-return.

let condition num = num % 2 = 0

let result = opt {
    if condition 2 then 
        if condition 4 then 
            if condition 6 then 
                return 10
}

Rewriting your example:

let getBuildDate (assembly: Assembly) : DateTime option = opt {

    let buildVersionMetadataPrefix = "+build"
    let attribute = assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()

    if attribute <> null && attribute.InformationalVersion <> null then
        let value = attribute.InformationalVersion
        let index = value.IndexOf(buildVersionMetadataPrefix)
        if index > 0 then
            let value = value.Substring(index + buildVersionMetadataPrefix.Length)
            let success, timestamp = DateTime.TryParseExact(value, "yyyyMMddHHmmss", CultureInfo.InvariantCulture, DateTimeStyles.None)
            if success then
                return timestamp
}

No more None.

Upvotes: 3

Charles Mager
Charles Mager

Reputation: 26213

Another approach might be to use the Option functions - each of these steps will effectively short circuit if the input from the previous step is None.

let getBuildDate (assembly: Assembly) : DateTime option =    
    let tryDate value =
         match DateTime.TryParseExact(value, "yyyyMMddHHmmss", CultureInfo.InvariantCulture, DateTimeStyles.None) with
         | true, date -> Some date
         | false, _ -> None

    let buildVersionMetadataPrefix = "+build"
    let attribute = assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()

    Option.ofObj attribute
    |> Option.bind (fun attr -> Option.ofObj attr.InformationalVersion)
    |> Option.map (fun infVer -> infVer, infVer.IndexOf buildVersionMetadataPrefix)
    |> Option.filter (fun (_, index) -> index > 0)
    |> Option.map (fun (infVer, index) -> infVer.Substring(index + buildVersionMetadataPrefix.Length))
    |> Option.bind tryDate

Whether this is 'better' is arguable - and definitely a matter of opinion!

Upvotes: 10

Related Questions