1

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^^

7
  • Why do you want to "destroy the facade" completely? Why not leave it as it is, or let the class User further exist as a class with reduced responsibilities? Please clarify. – Doc Brown Mar 17 at 6:35
  • because its the last step in that refactoring proccess to remove it, see the pdf. I want to see how that can be done. I dont think that it is good having a facade with 100-1000 delegation-methods?^^ You mentioned a reduced responsibility User class, where are the other responsibilties gone? How can i access them? If i want to be object oriented. – Robin Kreuzer Mar 17 at 15:07
  • "Object oriented" does not mean any method which involves a User must be a member of User. You have to distinguish between methods which you want to keep inside the User class (good candidates are those who work with user attributes exclusively, and need almost nothing else), and methods (inside new refactored classes) which just get a User as parameter. If you want less abstract advice, you need to show us an example for a responsibility of the user you want to move somewhere else (the "height" property of a user should probably be kept there, that's not a good example). – Doc Brown Mar 18 at 6:38
  • ... or, if you want to discuss real code, try to ask about it on codereview.stackexchange.com – Doc Brown Mar 18 at 6:43
  • 1
    I can only repeat what Robert Harvey responded to that other question of yours: "The only way this is answerable is if we work through a code example." – Doc Brown Mar 18 at 21:02
6

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.

9
  • i edidted my question (after edit^^) to focus it more in the direction after sprout and split. Maybe you can extend your answer a little bit towards that, how to remove the facade which itself is a part of other domainobjects, a direct linking seems not to work. But by no means, your answer right now is still great, maybe you can give some more details/examples in that point (If the variables are updated by two different groups of functions) in particular the secound subpoint "Or you.... in behaviours" – Robin Kreuzer Mar 15 at 22:53
  • I've refined my answer a little, but moving the goal posts isn't great. :( – Kain0_0 Mar 16 at 0:49
  • i dont think having moved the goal post, but highlighted it more, after i saw the answers. I assumed the refactoring till the facade-step was still given; i dont precised this more in my last sentecenses in my origin post, because it seems that the steps destroying the facade is not possible and so that the whole refactoring process must be directed in a other direction. But i should have to make a few things on the right positions more clear; so now the impression has arisen that i moved the goal post. – Robin Kreuzer Mar 16 at 2:20
  • The client Something in your post is my UserContainer with its list, and this client has its clients too. now i still want to get all users with 1,7meters high from that UserContainer and call the doesTheWork, if the user is still there as a Facade, no problem, if not, then i have problem And for that case i search/ask for a solution. Maybe in that case we have to rethink the whole refactoring proccess. – Robin Kreuzer Mar 16 at 2:26
  • The data still lives somewhere in some form. Even after you have split it up it still exists, and is still related. You might have to look in this list and access that property, and this other property over here in another part of the object graph, but that is part of the process of refactoring. Chances are though that you will still have a User object - that is an object that you go to to determine stuff about the user. Its just now going to be composed of orthogonal objects describing permissions, contact details, and whatever other user related information you have. – Kain0_0 Mar 16 at 2:41
3

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.

1
  • hey, i edited my question to make unclear things clearer and give it more focus. Maybe you can include there some ideas into your answer. But its a good answer to show clearly how a general god-class refactoring can work till the facade. And in purposes in which the god-class wasn't really included in other domain-object, if the idendity of the god class itself is relevant. – Robin Kreuzer Mar 15 at 23:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.