Should conditionals be embedded in the function whose execution is contingent on them?

Question

I have a large data structure that is about to be persisted to the database. Before that can happen I have to validate it and then update a bunch of it's properties based on various specific conditions.

Which style is more readable, having a bunch of if/else statements which determine which functions will execute under specific conditions or a simple list of functions to execute which contain within themselves the condition under which they should execute?

The first gives me indentation hell and nested ifs, the second looks clean but gives the impression that the list of functions always executes and requires viewing each function to see the circumstances under which each function will run.

Example of if/else style:

function myFunc(myThing){
    if(myThing.myProp === 'someValue'){
        doStuff();
        if(myThing.myProp2 === 1){
            doAThing();
        } else {
            doADifferentThing();
        }
    } else {
        doOtherStuff();
    }
}

Example of conditions nested in functions:

function myFunc(myThing){
    doStuff(myThing);
    doAThing(myThing);
    doADifferentThing(myThing);
    doOtherStuff(myThing);
}

function doStuff(myThing){
    if(myThing.myProp === 'someValue'){
        //Do stuff
    }
}

function doAThing(myThing){
    if(myThing.myProp === 'someValue' && myThing.myProp2 === 1){
        //Do a thing
    }
}

function doADifferentThing(myThing){
    if(myThing.myProp === 'someValue' && myThing.myProp2 !== 1){
        //Do a different thing
    }
}

function doOtherStuff(myThing){
    if(myThing.myProp !== 'someValue'){
        //Do other stuff
    }
}

These examples aren't perfect, but hopefully they convey what I'm asking.

Possible duplicate of [Elegant ways to handle if(if else) else](http://softwareengineering.stackexchange.com/questions/122485/elegant-ways-to-handle-ifif-else-else) — gnat, Feb 07 '17 at 17:20
I think there's not enough information to answer. If `myFunc` means "create an invoice" and `doStuff` means "add sales tax", then `doStuff` should be the function which checks whether or not the location is a location that has sales tax. If `myFunc` means "make breakfast for this person" and `doStuff` means "add cream to the coffee", `myFunc` should be the function which checks whether or not the person likes cream in their coffee. — Tanner Swett, Feb 07 '17 at 19:59

score 6 · Accepted Answer · answered Feb 07 '17 at 17:13

The second approach is more readable and will probably be more maintainable in the long run.

The content of myFunc is more understandable: looking at it, I can very quickly get a high-level idea of what's happening here, I don't need to start digging through nested conditions right away.

The other functions are simpler and more self-contained. If something changes for one condition, you don't need to untangle a mess of nested spaghetti code - go straight into the function, change it, you're done! It even lends itself to having these smaller functions be abstracted away to other modules in the future, if necessary.

The only downside I can see to the second approach is that there will probably be more typing involved, but I think the trade-off is more than worth it.

If you're worried that the second option

gives the impression that the list of functions always executes and requires viewing each function to see the circumstances

you could change the names from doAThing to checkPropAndDoAThing or doThingIfProperty. Obviously, if you want to do that, you would need to come up with short, concise names for the properties in the functions, for example: formatNameIfTooLong. Or, add a more detailed description to the function's documentation so that your IDE can pop-up the description with the details of the condition within. This will only work if your IDE supports it, but it's still probably a good idea to document it.

What if we use a switch case on the property and then call the function? — ihimv, Feb 14 '17 at 12:17

score 2 · Answer 2 · answered Feb 07 '17 at 17:51

As a general rule, you want to separate the decision about whether to do something ("decision logic")from the what you do ("action code").

This lets you check the decision logic separately from the action code. It lets you stub the action code, and check all of the decision logic, and look for sequencing surprises, without ALSO having to mock up enough of the environment to test the action code simultaneously.

At the same time, this lets you farm out the action code, and develop and test it concurrently.

Yes, in your case, you get an if-then-else indentation spaghetti platter. I would argue that this is not a Bad Thing, as it warns the reader that there is a Business Logic mess at the heart of all this. Business Logic problems are usually above the software engineer's pay grade; they have to be addressed by corporate management.

Note: There's a very interesting tidbit in a book I read many years ago. A guy attending graduate business school was having hell with an accounting/auditing class, fighting with a digital computer simulation. He finally broke into the chemistry building, liberated a bunch of glassware, and built an ANALOG computer model, using water flows through valves to simulate the money flowing through the simulated company, and was able to get his answers that way. The side-effect was that seeing the convolutions of the analog model, in the profusion of pipes, valves, tees, and stuff, gave him significant insight into how messed-up the simulated company really was.

You want to make those messes visible.

You make a good point re: testability. – Legion Feb 07 '17 at 18:02 — Legion, Feb 07 '17 at 18:02

score 0 · Answer 3 · answered Feb 07 '17 at 18:54

I would be inclined to make use of the fact that JavaScript is actually an object oriented language and of polymorphism.

If you make a base tester object that runs the tests and that has either pure virtual methods for each or that simply has stubs for each that return Fail.

Then for each of the variant types in your data structure you can define test classes that inherit from the base tester and customises the individual tests as necessary.

Then your tests will be able to use the test classes for the variants as appropriate, you should get good clarity in your test code and good maintainability.

Should conditionals be embedded in the function whose execution is contingent on them?

3 Answers3