6
\$\begingroup\$

Very easy question for these who spend some time on analyzing code written by co-workers.

When I write my code, I do my best to write it in a very, extremely readable way. But sometimes I have an impression, that I write it too easy... Simple example:

When I see such a code:

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

the first thing I do is changing it into:

    public Category getCategoryType(Component components){
            boolean hasSensor = component.hasSensor();
            boolean hasInput = component.hasInputUnit();
            boolean isValid = component.isValid();
            boolean isNotSuspended = !component.isSuspended();

            if(hasSensor && hasInput && isValid && isNotSuspended){
                return Category.TYPE_A;
            }
        } 

One of my collegue told me politely, that it is not necessary, because people can read a little more complicated code. But I think, that someone who will read this ode after me, will have a easier task (despite the fact, that I make this function a little bit longer)

Question for you: Do you think, that such changes in code are senseless?

Maybe only formatting stuff is sufficient:

public Category getCategoryType(Component components){
        if(component.hasSensor() && 
           component.hasInputUnit && 
           component.isValid() &&
           !component.isSuspended()){
               return Category.TYPE_A;
        }
} 
\$\endgroup\$
  • \$\begingroup\$ I like your idea of formatting every condition on a new line, but the automatic formatter (yours, or your co-workers') will probably destroy that at some point. \$\endgroup\$ – toto2 Sep 8 '13 at 15:05
  • \$\begingroup\$ @toto2 Absolutely right : ) That's why I use code from the second listing and that's why in some companies each developer is obligated to use the same formatting rules ; ) \$\endgroup\$ – guitar_freak Sep 8 '13 at 17:30
  • \$\begingroup\$ You'll never get consensus on this question, I think. Anyway, if you solve your other problem, then you might not run into this issue. \$\endgroup\$ – 200_success Sep 9 '13 at 20:02
  • \$\begingroup\$ Iff you introduce variables make them final. \$\endgroup\$ – Martin Schröder Sep 10 '13 at 7:47
5
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ +1 "... when a simple re-formatting is good enough." Ditto. Further: Re-written code must be tested - all testing phases in your development process - and ensure that functionality has not changed. And beware of "but it's a simple change" thinking; that is a "famous last words" kind of things. Professionalism is a harsh mistress. \$\endgroup\$ – radarbob Sep 9 '13 at 2:00
4
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I am all in favour of DI but I don't think that's the way to do it in this context. The signature contains 4 booleans which is confusing and you can bet that somebody is going to call the method with the arguments in the wrong order - and reading a statement like getComponentCategoryType(true, false, false, true) does not improve readability IMO. \$\endgroup\$ – assylias Sep 10 '13 at 7:33
  • \$\begingroup\$ This is not DI ..and first thing I thought when I read my answer was "this looks like it needs something analogueous to EventArgs, an interface that wraps allthe args in a single param... but that would be overkill it seems. You're right, 4 bools isn't a very nice signature, I meant to say the method doesn't need to know everything about the component :) \$\endgroup\$ – Mathieu Guindon Sep 10 '13 at 10:37
1
\$\begingroup\$

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();
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ +1. The more I look at it, the more I find component should be able to determine its category. Looks like feature envy. \$\endgroup\$ – Mathieu Guindon Sep 11 '13 at 0:38
1
\$\begingroup\$

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;
        }
}
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.