Do you look at which variables a method uses before refactoring a large class into smaller ones?

Question

Hi all,

I am interested in exploring the idea that the relationship between methods and the member varialbes they use can give a hint to how the class could be broken up into smaller pieces. The idea is that a group of variables will be closely related to one responsibility and should be contained in one class according to SRP.

For a trivial example, a class such as:

public class Rectangle {
  private int width; 
  private int height;
  private int red,green,blue;
  public int area() { return width * height; }
//just an example, didn't check the api.
  public Color color () { return new Color (red, green, blue); } 
}

Should be refactored into:

public class Rectangle {
  private Dimension size;
  private Color color;
  ...
}

Because the break down would be:

Area: width, height Color: red, green, blue

Since these variables are used in the same method they are clearly related and could be made into a class of its own. I know this example might be too trivial but bear with me and try and think bigger here. If other methods also use these variables they are most likely also related and could also be moved into the new class.

Just for fun I created a little plugin for Eclipse that tries to do this. It will have a break down on methods->variables, variables->methods and also tries to group methods together after which variables they use either directly or indirectly.

Is this something that you do when coding, and is it actually helpful?

Answer 1

Sounds like someone's read about the Law of Demeter and is attempting to take it to extremes. I have to say, I always treat the "decoupling" zealots with a certain wariness, not least because some people really do advocate "zero coupling" in design, while ignoring the fact that without any coupling at all, a system is useless.

Nevertheless, reducing the dependencies in code makes it easier to test and more resilient to other refactoring, plus I find it tends to lead to simpler, more obvious code. A healthy balance is the goal to strive for.

Answer 2

I can't say that this has ever been something that I've thought about while coding. Yes, it could potentially result in some improvement, but I think what you'd find yourself doing in practice is to create a large number of classes that would be even more awkward to deal with.

There may be some merit to it as a "code smell", though. If you see a class with tons of member variables, and most are only used by a handful of methods, that's generally a bad sign.

Answer 3

This seems like a bad idea overall.

First, a class represents a collection of related data and/or methods. It doesn't need to have any methods at all to still be a useful class. For good encapsulation you might want getters and setters; these methods by definition won't access more than one variable. That doesn't imply anything about whether or not those variables belong in the same class.

Finally, a method belongs to the class if it provides a way to manipulate the class, compute some information about itself, construct the class, etc; there are infinite reasons why a method might belong in a class but not access most of the member variables. Then there are static methods; should these by definition be in other classes?

A class should be split up if there are too many concepts in it. But those concepts only loosely map to methods and variables. Counting variable uses is a sure-fire way to overcomplicate your class hierarchy with no benefit.

Answer 4

I think each class should be organized by what it does, and not what variables it will use. I would recommend making each class have a single purpose (single responsibility principle). Instead of breaking things up based on their usage of variables separate classes based on their responsibility or purpose.

In my opinion writing clean code is not only how good the code looks, but how easily it is to read. Code should be grouped logically and have consistent style. Keep in mind some classes are not going to use all variables. In java you have mutator and accessor methods. These methods are used to get or set a single variable.

Example:

public class Rectangle {

    private int lenght = 0;
    private int width = 0;

    public Rectangle (int length, width)
        {
     this.length = length;
     this.width = width;
    }

    public int getLength(int length)
    {
     return length;
    }

    public int getWidth(int width)
    {
     return wdith;
    }


    public void setLength(int length)
    {
     this.length = length;
    }

    public void setWidth(int width)
    {
     this.width = width;
    }
}

I would not break this class into a RectangleLength and RectangleWidth class. These methods serve a single responsibility and that is to represent a rectangle. If this class contained mathmatical functions you might want to break that out into a RectangleGeometry class.

Answer 5

I can imagine a situation where this is a reasonable code smell. The simple setters/getters in our example aren't too persuasive. For example, refactoring only makes a more complicated object that carries the a and b around:

public class A {
  private String a1;
  public void aMeth () {
     print(a1);
  }
}
public class B {
  private String b1;
  public void bMeth () {
     print(b1);
  }
}

public class A { 
  private A a;
  private B b;
  ... insert infinite regress 
}

However, I can see this as a guide where you have sets of variables entangled into big monster classes, where the variable groupings correspond to the conceptual breakdown. Are you thinking more of something like this?

public class Rectangle {
  private int width; 
  private int height;
  private int red,green,blue;
  public int area() { return width * height; }
//just an example, didn't check the api.
  public Color color () { return new Color (red, green, blue); } 
}

Where the rectangle should have a Dimension and Color. It seems to me that if you can clearly find sets of private variables that entangle only with each other, that's a useful thing, as long as you omit the non-entangled variables (i.e If you think of the variables as graph vertices, I think you'd want to detect the connected subgraphs but omit subgraphs of size 1)

Answer 6

I've done this. Even created a table, methods down the first column, instance variables along the top, with an X to indicate which uses which. Then sort - kind of like heatmaps are sorted, but more casually, more manually - to see which variables, and which methods, track together. It works and it's helpful. But it's deep in my bag of tricks; I've used it only rarely. I'll have to check out your plugin; if it were less work to do this, I might do it more.

Answer 7

This is certainly a good refactoring approach. In Working Effectively With Legacy Code, Michael Feathers describes a technique for this in a section called Seeing Responsibilities:

Heuristic #4 Look for Internal Relationships

Look for relationships between instance variables and methods. Are certain instance variables used by some methods and not others?

He then goes on to describe a method where you draw a diagram called a feature sketch, which is a graph showing the methods of a class and internal method calls. From the diagram, you can easily visually identify clusters of interconnected method calls which are candidates for refactoring into a new, more cohesive class.

It would be very cool if your Eclipse plugin could quickly generate a graph as described in the book - I could see that as being a useful refactoring tool.