Some stuff about Web and .NET development
RSS icon Email icon Home icon
  • Bad smells in code by Martin Fowler

    Posted on August 9th, 2010 Thibaut No comments

    At the company I’m currently working for, there’s a big poster on the wall with the following headline : “if you see something like this, please raise your hand to improve code quality”. Below is the content of that poster – a list of things to avoid when coding – from Martin Fowler’s refactoring book.

    • Duplicated code : the same code structure in two or more places is a good sign that the code needs to be refactored: if you need to make a change in one place, you’ll probably need to change the other one as well, but you might miss it
    • Long methods : longs methods should be decomposed for clarity and ease of maintenance
    • Large class : classes that are trying to do too much often have large numbers of instance variables. Sometimes groups of variables can be clumped together. Sometimes they are only used occasionally. Over-large classes can also suffer from code duplication
    • Long parameter list : long parameter lists are hard to understand. You don’t need to pass in everything a method needs, just enough so it can find all it needs
    • Divergent change : software should be structured for ease of change. If one class is changed in different ways for different reasons, it may be worth splitting the class in two so each one relates to a particular kind of change
    • Shotgun surgery : if a type of program change requires lots of little code changes in various different classes, it may be hard to find all the right places that do need changing. Maybe the places that are affected should be all brought together into one class
    • Feature envy : this is where a method on one class seems more interested in the attributes (usually data) of another class that in its own class. Maybe the method would be happier in the other class
    • Data clumps : sometimes you see the same bunch of data items together in various places: fields in a couple of classes, parameters to methods, local data. Maybe they should be grouped together into a little class
    • Primitive obsession : sometimes it’s worth turning a primitive data into a lightweight class to make it clear what it is for and what sort of operations are allowed on it (eg creating a date class rather than using a couple of integers)
    • Switch statements : switch statements tend to cause duplication. You often find similar switch statements scattered through the program in several places. If a new data value is added to the range, you have to check all the various switch statements. Maybe classes and polymorphism would be more appropriate
    • Parallel inheritance hierarchies : in this case, whenever you make a subclass of one class, you have to make a subclass of another one to match
    • Lazy class : classes that are not doing much useful work should be eliminated
    • Speculative generality : often methods or classes are designed to do things that in fact are not required. The dead-wood should probably be removed
    • Temporary field : it can be confusing when some of the member variables in a class are only used occasionally
    • Message chains : a client asks one object for another object, which is then asked for another object, which is then asked for another one, etc. This ties the code to a particular class structure
    • Middle man : delegation is often useful, but sometimes it can go too far. If a class is acting as a delegate, but is performing no useful extra work, it may be possible to remove it from the hierarchy
    • Inappropriate intimacy : this is where classes seem to spend too much time delving into each other’s private parts. Time to throw a bucket of cold water over them !
    • Alternative classes with different interfaces : classes that do similar things, but have different names, should be modified to share a common protocol
    • Incomplete library class : it’s bad form to modify the code in a library, but sometimes they don’t do all they should do
    • Data class : classes that just have data fields, and access methods, but no real behavior. If the data is public, make it private !
    • Refused bequest : if a subclass doesn’t want or need all of the behavior of its base class, maybe the class hierarchy is wrong
    • Comments : if the comments are present in the code because the code is bad, improve the code

    Share/Save/Bookmark

    Comments are closed.