Breaking up a large, cohesive class

Question

I have a class that I think is too long. I don't really know what too long means but it's about 2500 lines of code. However, all the methods use at least one or more of the variables so I think it's pretty cohesive. I'm thinking of still breaking this class up into a few smaller classes that would use three of the same variables. Is this bad design or does this represent a pattern?

class MyClass
{
    ...
    MyVar1 myVar1;
    MyVar2 myVar2;

    public void DoStuff()
    {
         ...
         MyPart1 myPart1 = new MyPart1(this,myVar1,myVar2);
         myPart1.DoStuff();

         MyPart2 myPart2 = new MyPart2(this,myVar1,myVar2);
         myPart2.DoStuff();
    }
}

Answer 1

Think "Single Responsibility" (SR). I bet, that a class with 2500 lines of code does at least 20 different things, that could easily be separated.

First, all private methods are usually a new SR class waiting to burst free. Second, That a variable is used many places, does not mean, that someone else (another class) can't hold, and handle it. Third, a big class like that must be close to impossible to test. A SR class is normally very easy.

So the only thing i can recommend it to start cannibalizing your monster class, and let dozens of pretty little one-hit wonders see the light of day :-)

Regards, Morten

Answer 2

Unless the parts you are separating have an easily defined, sensible single responsibility, I wouldn't split it up like that. If you can describe the subdivided class in just one line, and that line isn't "used to manage var1 and var2", you might be on to something.

2500 lines isn't all that big either - sometimes it just takes that much clear and cohesive code to get something done.

Edit: There are some ways you might be able to detect that something is fishy in the class.

• If you are repeating the same or very similar code several times in the class body, you may be able to factor out the repeating part, making the class shorter (and probably more self-descriptive)
• If there are clear sets of member functions that use a particular subset of the variables, and these do not overlap (i.e. member functions 1-10 use var1 , member functions 2-20 use var2 and var3), you may have an implicit double responsibility. Finding exactly what it is and separating it out may not be easy but at least you can figure out what to look at.
• If several of the functions are minor variations on other functions then the interface of the class may be too broad. Could you do the same thing as a member function by calling two other member functions? If so, consider removing the redundant member function, perhaps just documenting how to do it with the other more basic member functions.

Answer 3

You can't generally say that 2500 lines of code is too much for one class.

You can however say that a class that gets used for 10 different actions is quite monolithic. Some people here say that each class should only have one functionality. These people would read the "10" as binary...

Now if you don't see a chance to divide your class in half, maybe start by splitting small functional parts off instead. This way you might get a better view for what's really essential to your classes' functionality.

Start with looking at your methods: If your class has several methods that basically belong to the same scope (e.g. XML-I/O or something like a Play/Pause/Stop/Reset function set) you could create a subclass for these.
If all your methods are on a par with each other (i.e. the opposite of the above), I'd say your class is not too big.

The most important thing though is that you don't lose orientation in your code. Try structuring your class and order your methods as seems best fit. And don't forget to comment this order so you'll get into it easily again...