No trivial god-class refactoring

Question

Consider you have the following code:

class UserContainer
{
  List<User> user;
//some methods to get specific users, for example users, which are higher than 1,70meters

}

The User have a lot of different responsibility and features, so in the example the user is our god-class. There a lot of methods which are used from outside, from clients

So now let us refactor the God-Class into smaller units.

like it is described here: http://scg.unibe.ch/download/oorp/OORP.pdf (PDF-Page 281, chapter 9.3 Split Up God Class).

So there we should refactor the godclass into classes which bundles high-cohesive features/fields. The old User class is near the end a facade-class with more than 100 methods.

In the final step you should remove the facade/old-god-class itself.

Ok, how should that work?^^ There is no answer in that paper how to do so, if you have something like i have in the example.

So maybe i want to get from the UserContainer all users which are 1,70meters high and then i want to do some special behaviour with that user, some behaviour which was before my refactoring in these 100 and more methods. But if had it refactored now, and deleted the god-facade class in the last step -> so there is no user anymore........ äh how?!^^

So the question is easy: How should i remove/refactor a good-class with its 100 and more methods (+ removing in the last step a possible facade)?

One more point, which is not adressed in the paper, how you would instantiate the new classes and wire them up?

Edit:

I want to add some additional notes to make things clearer:

The UserContainer is not the god-class, it's only a class with one Responsibility, so maybe i should rename it to UserHeightFilterContainer like Flatter used in his answer.

The problem then is not how to sprout and split the class User like Kain0_0 described in his answer, if we still have the facade UserFacade at the end, because then i could still have

List<UserFacade> user

in my UserContainer or UserHeightFilterContainer

but if i now want to destroy the facade UserFacade then i dont have something left which i could use in the List or rather UserHeightFilterContainer. And i want to know how to destroy that facade with the constraint to continue using the UserHeightFilterContainer

Let' say i want to get all Users which are 1,7meters high from that container and then i want to call some User-behaviour which is then not anymore in the user because i sprout and splitted it^^

Answer 1

Sprout and Split

Take a look at your god class again. You know its doing to much because you can see X responsibilities that its doing. Start by trying to isolate those X responsibilities within the class.

This can be pretty easy for some functions and variables as they are only ever used by one responsibility move these into a one section of the class to live right next to each other.

On the other hand some functions are just layers of interwoven responsibilities. Time to apply Sprout. Refactor the function to extract one responsibility at a time. Hopefully you have unit/module level tests that will tell you if you've broken the class. If not write some first. Then start pulling out behaviours into supporting functions. Each time you pull out one of these behaviours into a new function:

• scan the whole of the class to see if it doesn't already exist
• look at the other functions to see if it can't replace behaviours there too
• move the function close to related behaviours

Similarly shared variables can be problematic. If by now after sprouting out all the reasonable behaviours that you can, you still have variables shared between two or more groups of functionality, it can be tricky to handle.

• If the variables are readonly you can consider wrapping them in a config class.
• If the variables are only updated by one group of functionality, then it probably belongs with that functionality. Although somehow those other sections of code need that information. Try seeing if that information could not be returned as part of those sections calling behaviours in this section. In the worst case add a new wrapping function.
• If the variables are updated by two different groups of functions.
  • It might belong more strongly to one section than the rest. In which case see if you can't effect the writes via other behaviours already in that section. For example a class that writes to a terminal tracks the current line and column. Instead of having a new behaviour directly update those variables, it instead calls a MoveCursor(x,y) which updates those variables, and any other related behaviour.
  • Or you might be missing an abstraction. Try and push these variables down and wrap them in behaviours. For example a parser might push tracking the current token down into a couple of behaviours like Next(), Peek(), Skip() wrapping the TokenStream stream and a Token? currentToken.

Finally review the code with a fresh head. You might want to collapse two sections together as they are actually the same responsibility. You might want to delete a section as its clearly dead code. You might want to further refine a section breaking it further down. Keep pushing and pulling at the code till the sections are coherent, and useful.

By now you should have a well modularised god class. Its now time to apply Split.

Grab one of these sections of code and extract it into a new class. Add interfaces, and collaborator properties, etc... to link it all back up. Keep splitting the class up into separate classes one section at a time. At the end of this you have a God Facade and a graph of collaborating objects backing it up.

Time to kill the Facade, or not. There is no reason why a facade should not exist. It simplifies the backing object graph, so use a pinch of practicality here. If you do decide to remove it, its pretty simple. You just inline the facades implementation where it is being used. EG:

class Facade
{
   void DoX() { X.DoesTheWork(); }
}
class Something
{
   void Foo
   {
       facade.DoX();
   }
}

becomes:

class Something
{
   void Foo
   {
       X.DoesTheWork();
   }
}

Repeat for each call site, then delete the Facade class.

Answer 2

The User have a lot of different responsibility and features, so in the example the user is our god-class. There a lot of methods which are used from outside, from clients
So now let us refactor the God-Class into smaller units.

If you already see the "different responsibilties", then you are able to list those responsibilities. This is the indication on what you need to split your god class on.

Note that "responsibility" has a subjective measure of granularity. If we were talking 5 methods in total, "handles user list filtering" would be an appropriately scoped responsibility. If we're talking 100 methods, you need to divide them further and therefore focus on more specific responsibilities, e.g. "filters users by height", "filters users by age", ...

so in the example the user is our god-class.

I'm not quite sure that's correct. Your example implies that UserContainer is the god class, not User.

In the final step you should remove the facade/old-god-class itself.
Ok, how should that work?^^ There is no answer in that paper how to do so, if you have something like i have in the example.

Let's run with my previous example, and say that you subdivide your UserContainer into specific filtering: height, age, last name.

Instead of a UserContainer, you'd have separate UserHeightFilterContainer, UserAgeFilterContainer and UserLastNameFilterContainer classes, each representing the now divided responsibilities. Therefore, any old code that used to call e.g. UserContainer.GetUsersTallerThan() would be changed to UserHeightFilterContainer.GetUsersTallerThan().

Assuming you subdivided all the methods of UserContainer, there would no longer be any method calls to it, so the class can safely be removed.

So maybe i want to get from the UserContainer all users which are 1,70meters high and then i want to do some special behaviour with that user

Fetching the data and performing operations on it are two very different things, and you'll find that most if not all codebases separate these into two distinct responsibilities.

In my above example, the UserHeightFilterContainer is used as the queryable data store (i.e. repository). You haven't really specified what operations you'd want to perform on this data, but using the example of e.g. sending an email summarizing all these tall users, you'd end up with a MailSender class doing something like:

public class MailSender
{
    public void SendLengthReportTo(string emailAddress)
    {
        var tallUsers = _userHeightFilterContainer.GetUsersTallerThan(1.7);
    var sb = new StringBuilder();
    sb.AppendLine("These are all users above 1.7m:");

    foreach(var tallUser in tallUsers)
    {
        sb.AppendLine($" > {tallUser.Name}");
    }

    SendMail(
        emailAddress,       // recipient
        "Tall user report", // subject
        sb.ToString()       // mail body
    );
}

}

This is just a simple example, but notice how the above code focuses solely on how to format/send the email, and it delegates the responsibility of filtering the appropriate users to the underlying UserHeightFilterContainer.

But if had it refactored now, and deleted the god-facade class in the last step -> so there is no user anymore........ äh how?!^^

There's a big difference between UserContainer ceasing to exist, and User ceasing to exist. In the above example, and implied in your code example/explanation, UserContainer may cease to exist, not User.

One more point, which is not adressed in the paper, how you would instantiate the new classes and wire them up?

That is too broad of a question to be tacked on to this one. Look up inversion of control and more specifically dependency injection if you want to learn more on how this is generally approach in professional clean codebases.