XNSIO
  About   Slides   Home  

 
Managed Chaos
Naresh Jain's Random Thoughts on Software Development and Adventure Sports
     
`
 
RSS Feed
Recent Thoughts
Tags
Recent Comments

Archive for the ‘Code Smells’ Category

Multiple Returns or Lower Cyclomatic Complexity: Which Coding Style Do You Prefer?

Sunday, March 27th, 2011
public String execute() {
    String resultStatus;
    User user = getUser();
    if (loggedIn(user)) {
        String key = getParameter(KEY);
 
        if (!isEmpty(key)) {
            String id = findSourceIdFor(key);
            Record record = new Record(user, key, id);
            record.save();
            resultStatus = "Successful";
        } else {
            logger.severe("Invalid key"); 
            resultStatus = "Invalid Key";
        }
    } else
         resultStatus = "User Not Logged In";
 
    return resultStatus;
}

OR

public String execute() {
    User user = getUser();
    if (!loggedIn(user))
        return "User Not Logged In";
 
    String key = getParameter(KEY);
    if (isEmpty(key)) {
        logger.severe("Invalid key");
        return "Invalid Key";
    }
 
    String id = findSourceIdFor(key);
    new Record(user, key, id).save();
    return "Successful";
}

Personally I like the second code sample. I’m a big fan of the guard clause pattern. Even though the second code sample has multiple return statements (which some people hate), it has much lower cyclomatic complexity.

What heuristics do you use to decide Long Method Smell?

Friday, March 25th, 2011

I find myself using the following heuristics:

More details: Long Method Smell: When is a method too big?

Single Responsibility Principle Demystified

Monday, October 19th, 2009

Lately, I’ve sensed some confusion in developers around the Single Responsibility Principle (SRP). Its easy to say every object or abstraction should have a single responsibility. But it’s important to understand what do we mean by responsibility.

So when we talk about SRP, what does single responsibility really mean?

Some developers suggest each public method on a object is exposing a responsibility. If we go by this definition of responsibility we’ll end up with every object having exactly one method. Which does not feel right.

Some developers suggest that each object should do exactly one thing. What do they mean by one thing? When I look at a class or a method, how can I tell if its doing one thing or more than one thing? This definition of SRP is even more vague than the original definition.

For example, if we have a class Foo, do you think, adding the toString() method violates SRP? Do you think toString() is the responsibility of the Foo class? Most developers would agree that toString() really belongs on the Foo class and it does not violate SRP. toString() method usually needs access to the internal instance variables and hence this method really belongs on the respective class.

Lets say, we agree that toString() method should belong on the Foo class. Now going by the same argument, can we add toXML() method on the same class? If no, why not? If yes, when will you stop?

Basically this is a slipper slope. Trying to understand SRP by defining Responsibility does not work for me. Instead I look at it from the “Axis of change” or “Single Reason for Change” lens.  If I look at a class Foo with a method bar() on it; if the class Foo and the method bar() change for 2 different reasons then I think it violates SRP. Irrespective of how many methods it has and how many things it does. (It could be violating other design principles).

Going back to our example, I think adding a toString() method does not violate the SRP coz I don’t see any reason why only the toString() method needs to change and the rest of the class remains unchanged. If we add or remove instance variables, its most likely that both the class and the toString() method will change.

Now what about toXML() method? I think the toXML() method does not belong on the object, coz I see 2 reasons for which the toXML() method can change.

  1. When we add/remove any instance variables
  2. When the XML representation/format changes.

However there is a trade-off here. If you don’t put any method on the class, then how do other objects get the data out (other than breaking encapsulation. Even doing so will increase the coupling.)? On the other hand, if you add such methods, then any changes in the format/representation will effect this object as well. Which does not seem right. So how do you design something which does not violate SRP, does not break encapsulation and also adheres to Open Closed Principle?

One way to solve this puzzle is to add a to() method which takes a Formatter object, so we end up with a to(Formatter collector) method. Depending on what format (XML, YAML, etc) one needs to pass the appropriate Formatter type. For example if you want the XML representation of this Class, you pass a XMLFormatter object to the to() method. The class which has the to() method is responsible to adding what ever data it needs to expose and the Formatter object is responsible for the representation and formatting. This way if the XML schema changes, there is only one places to go make the change. If the instance variables change there is only one place to make the change. (Again this is not 100% true, but this solution is a step forward from the first solution as far as SRP goes).

So look for Reasons to Change, as a way to understand SRP.

BTW, In Uncle Bob’s SRP paper (pdf), he defines SRP as:

There should never be more than one reason for a class to change.

Frankly I find this a little too restrictive. I think there can be more than 1 reasons for the class to change. As far as all those reasons affect the whole class (all or most of the methods) the same way, I think we’ve achieved high cohesion and we can be happy for now.

Also its worth highlighting that SRP applies at multiple levels, right on top at a project level all the way down to your individual lines of code.

Conceptual Integrity

Sunday, February 1st, 2009

Conceptual Integrity is the principle that anywhere you look in your system, you can tell that the design is part of the same overall design. This includes low-level issues such as formatting and identifier naming, but also issues such as how modules and classes are designed, etc.

Imagine a Library where:

  • Few classes threw an exception
  • Few other classes returned you an integer error code
  • And rest had void methods, but you had to call a method HasErrors() & GetErrors()

This lack of inconsistency can lead to poor communication and complex client code. Having a consistent style of design is the very essence of Conceptual Integrity.

While developers focus a lot on High Cohesion and Low Coupling, they seem to underestimate the importance of Conceptual Integrity. Some times despite high cohesion and low coupling, the system might not have conceptual integrity. This is because the overall style, themes, mood, does not tie it all together.
For example according to the Pragmatic Programmer, in computer languages, Smalltalk has conceptual integrity, so does Ruby, so does C. C++ doesn’t: it tries to be too many things at once, so you get an awkward marriage of concepts that don’t really fit together well.

    Licensed under
Creative Commons License