Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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 handle 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.

Thank you for advice!

share|improve this question
    
If you would make classes like User and Chatroom, would these classes only need a reference to the common data structure or would they reference each other? –  Tobias Willig Aug 10 '12 at 16:05
1  
Please consider renaming your question. The title should shortly describe the problem so that it can be searched for etc. See meta.stackoverflow.com/questions/10647/… –  Petr Pudlák Aug 10 '12 at 16:05
    
There are several satisfactory answers here, you should pick one. –  jeremyjjbrown Jan 13 at 16:53
    
@jeremyjjbrown the question has been moved and I lost it. Picked an answer, thx. –  Matthew Jan 13 at 20:05
add comment

migrated from stackoverflow.com Aug 13 '12 at 14:13

This question came from our site for professional and enthusiast programmers.

7 Answers

up vote 6 down vote accepted

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);
share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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