How to split large, tightly coupled classes?

Question

I have some huge classes of more than 2k lines of code (and growing) that I would like to refactor if possible, to have some more light and clean design.

The reason it is so big is mainly because these classes handle a set of maps that most methods need to access, and methods are very connected to each other.

I will give a very concrete example: I have a class called Server that handles incoming messages. It has methods like joinChatroom, searchUsers, sendPrivateMessage, etc. All of these methods manipulate the maps such as users, chatrooms, servers, ...

Maybe it would be nice if I could have a class handling messages concerning Chatrooms, another handling all about Users, etc. but the main problem here is that I need to use all of the maps in most of the methods. That's why for now they are all sticked in the Server class as they all rely on these common maps and the methods are very connected to each other.

I would need to create a class Chatrooms, but with a reference to each of the other objects. A class users again with a reference to all the other objects, etc.

I feel like I would be doing something wrong.

Answer 1

From your description, I would guess that your maps are purely bags of data, with all the logic in the Server methods. By pushing all chatroom logic into a separate class, you're still stuck with maps containing data.

Instead, try to model individual chatrooms, users etc. as objects. That way, you will be passing around only specific objects required for a certain method, instead of huge maps of data.

For example:

public class User {
  private String name;
  ...

  public void sendMessage(String message) {
    ...
  }
}

public class Chatroom {
  // users in this chatroom
  private Collection<User> users;

  public void add(User user) {
    users.add(user);
  }

  public void sendMessage(String msg) {
    for (User user : users)
      user.sendMessage(msg);
  }
}

public class Server {
  // all users on the server
  private Collection<User> users;

  // all chatrooms on the server
  private Collection<Chatroom> chatrooms;

  /* methods to handle incoming messages */
}

Now it's easy to call a few specific methods to handle messages:

Want to join a chatroom?

chatroom.add(user);

Want to send a private message?

user.sendMessage(msg);

Want to send a public message?

chatroom.sendMessage(msg);

Answer 2

You should be able to create a class which hold each collection. While Server will need a reference to each of these collections it only need the minimal amount of logic which won't involve accessing or maintain the underlying collections. This will make more obvious exactly what the Server is doing, and separate out how it does it.

Answer 3

When I have seen big classes like this I have found that there is often a class (or more) in there trying to get out. If you know of a method that you think might not be related to this class then make it static. The compiler will then tell you of other methods that this methd calls. Java will insist they are static too. You make them static. Again the compiler will tell you of any method that it calls. You keep doing this over and over again until you have no more compile failures. Then you have a load of static methods in your big class. You can now pull these out into a new class and make the method non static. You can then call this new class from your original big class (which should now contain fewer lines)

You can then repeat the process until you are happy with the class design.

Martin Fowler's book is a really good read so I would recommend this too as there are times you cannot use this static trick.

Answer 4

Since most of your code is existing, i would suggest making use of helper classes to move out your methods. That will help easy refactoring. So, your server class will still contain the maps in it. But it makes use of an helper class say ChatroomHelper with methods like join(Map chatrooms, String user), List getUsers(Map chatrooms), Map getChatrooms(String user).

Server class will hold an instance of ChatroomHelper, UserHelper etc thus moving the methods to its logical helper classes. Wtih this you can leave the public methods in server intact, so any caller doesnt need to change.

Answer 5

To add to casablanca's insightful answer - if multiple classes need to do the same basic things with some type of entity (adding users to a collection, handling messages etc), those processes ought to be kept separate as well.

There are multiple ways to do this - by inheritance or composition. For inheritance, you can use abstract base classes with concrete methods that provide the fields and functionality to handle users or messages for example, and have entities like chatroom and user (or any other) extend those classes.

For various reasons, it's a good general rule to use composition over inheritance. You might use composition to do this in various ways. Since handling users or messages are functions central to the responsibility of the classes, it can be argued that constructor injection is most appropriate. This way, the dependency is transparent and an object cannot be created without having the required functionality. If the way users or messages are handled is likely to change or be extended, you ought to consider using something like the strategy pattern.

In both cases, be sure to code towards interfaces, not concrete classes for flexibility.

All that being said, always consider the cost of added complexity when using such patterns - if you're not going to need it, don't code it. If you know you're very probably not going to change how handling of users / messages is done, you don't need the structural complexity of a strategy pattern - but in order to separate concerns and avoid repetition, you should still divorce common functionality from the concrete instances that use it - and, if no overriding reasons to the contrary exist, compose the users of such handling-functionality (chatrooms, users) with an object that does the handling.

So, to sum up:

1. As casablanca wrote: Separate and encapsulate chatrooms, users etc.
2. Separate common functionality
3. Consider separating individual functionality to divorce data-representation (as well as access and mutation) from more complex functionality over individual instances of data or aggregates thereof (for example, something like searchUsers could go in a collection-class, or a repository / identity-map)

Answer 6

Look, I think your question is too generic to answer since we don't really have a full description of the problem so offering a good design with so little knowledge is impossible. I can, just as an example, address one of your concerns about the possible futility of better design.

You say that your Server class and your future Chatroom class share data about users, but this data should be different. Server probably has a set of users connected to it, while a Chatroom, which itself belongs to a Server, has a different set of users, a sub-set of the first set, of only the users currently logged into a specific Chatroom.

This is not the same information, even if the data types are identical.
There are many many advantages to a good design.

I have not read the aforementioned book by Fowler, yet, but I have read other stuff by Folwer and it was recommended to me by people I trust, so I feel comfortable enough to concur with the others.

Answer 7

The need to access the maps doesn't justify the mega-class. You have to separate logic in several classes, and each class must have a getMap method so other classes can access the maps.

Answer 8

I would use the same answer I provided elsewhere: take the monolithic class and divide its responsibilities among other classes. Both DCI and the Visitor Pattern provide good options for doing so.

Answer 9

In terms of software metric, big class is bag. There are unbounded papers proving this statement. Why is that ? because big classes are harder to understand than small classes and it's need more time to modified . Moreover, Big classes are so tough when you do testing . And big classes are very hard for you when you want to reuse it because it's very likely contain undesired stuff.