Long conditionals in ifs [closed]

Readability is extremely important, particularly in large projects with many programmers involved with the same code. What you do, using local Boolean variables to clarify potentially complex conditions, is a very good way of improving readability that I try to imprint on my co-workers.

That said, in your example, if I wrote that method from scratch, I would do it similarly to your first re-write. However, if I just happened upon the existing code, I would do just about exactly as your second re-write. It is good enough, and I do not really like to change other peoples code unless it is either incorrect, or directly violates the coding standard (which it incidentally does two-fold: The conditional line is too long, and our standard also tells us to place sub-conditions on separate lines).

So, in my humble opinion: You have it right - I like how you do it, and you should take pride in writing readable code. But do not waste time on re-writing working code when a simple re-formatting is good enough.

Does the method getCategoryType need to know everything about a Component? Maybe its logic can be satisfied (and tested) with only this:

public Category getComponentCategoryType(boolean hasSensor, boolean hasInput, boolean isValid, boolean isSuspended) { if(hasSensor && hasInput && isValid && !isSuspended) { return Category.TYPE_A; } } 

I'm not really into java so maybe this is violating a convention I don't know about, but I find this signature makes the method/function self-documented as far as its inner logic is concerned.

A bit like constructors that statically declare dependencies in dependency injection, a signature like this declares everything the method needs to know in order to give you a category type - by taking in a Component, all you know is that it takes a Component and spits out a Category.

And you can test the logic without needing to instanciate a Component - if that object were a 3rd-party thing you had no control over, having it as a depedency could make the method very hard to test.

Of course if the method is already in use in 2000 places and changing the signature would be a major breaking change, I see no problem with the last listing you provided.

If this is used in more than one place I would put validation logic either in Component, or some sort of ComponentRules class, so then your code will be something along the lines of

if (component.IsCategorizable()) DoWork(); 

Or

if (ComponentRules.CanBeCategorized(component)) DoWork(); 

I think we can avoid using local variables if possible and can directly use the passed-in parameter

public Category GetCategoryType(Component component) { if((component.hasSensor()) && (component.hasInputUnit) && (component.isValid()) && (!component.isSuspended())) { return Category.TYPE_A; } } 

Going against the grain of most answers, I am guessing that there are a lot of Component types . If there were more than say 20 types than I would either want to maintain

• Code like you have found
• Code that parses config file / database table with the rules
• Some kind of Map that contains the rules

The last thing I would want to find is code diarrhea with hundreds or thousands of lines.

I'm going to throw my hat in the ring and say the format of the last example is the easiest for me to read(like your colleague says).

It's easy to see that everything is coming from component and I can read the conditions in a straight line: it has a sensor, it has an input unit, it is valid, and it is not suspended. And that's exactly how I would read it going through the code the first time. Someone who comes across this code in the future shouldn't have any trouble figuring out what is going on.

With the local booleans, I have to verify that each boolean is coming from component and that the booleans created are the ones being used in the if(...). In this example it's trivial to see that is the case, but in the case of multiple ifs, elses, or other logic in the function...it may not be so trivial.

It also introduces another vector for producing bugs either in the creation of the function, or the function's maintenance in the future. Again, the example is trivial, but it's easy to imagine cases that aren't trivial.

Having seen your other related post, I would also pair this with having a separate function for each category(x3ro suggested something like isCat24(), etc), and within that function following the format of the last example you gave(one condition per line, no locals created).

Your conditionals then start to look like:

if(isCatA(component)) return Category.TYPE_A; if(isCatB(component)) return Category.TYPE_B; ... //Based on x3ro's suggestion: public boolean isCatA(Component c) { return c.hasSensor() && c.hasInputUnit && c.isValid() && !c.isSuspended(); } 

And if I want to verify each set of conditions, I can just look up one function per category and see what they're doing.

Finally, you might want to consider making a function in Component called getCategory(). All of the logic shown uses one Component, and uses no information from anything outside of that Component. It seems like it should be encapsulated by the Component class, and anyone who wants to know the category can call the getter for that information.