I recently encountered a situation in our codebase where a different team created a ‘god class’ containing around 800 methods, split across 135 files as a partial class.
I asked the other team about this. While my gut reaction was to nuke it from orbit, they insist that it’s a good design, a common practice, and that it promotes ‘modularity’ and ‘ease of implementation’ because new developers can bolt on functionality with almost no knowledge of the rest of the system.
Is this actually a common practice or in any way a good idea? I’m inclined to take immediate steps to bring this beast down (or at least prevent it from growing further) but I’m willing to believe I’m wrong.
7
This is not in any way a good idea.
Partial classes exist to help the form designer. Using them for any other reason (and arguably for their intended purpose, but that’s a different matter) is a bad idea, because it leads to bloated classes that are hard to read and understand.
Look at it this way: If a god class with 800 methods in one file, where you know where everything is, is a bad thing–and I think everyone would agree that it is–then how can a god class with 800 methods spread across 135 files, where you don’t know where everything is possibly be a good thing?
5
So the question was:
Is this actually a common practice or in any way a good idea?
Among the currently available answers is a resounding no. So there is few other things I could chime in about it such as; even procedural code can be made modular and including everything but the kitchen sink is not how you achieve modularity. Which one giant class split into partials does do, it all adds up into one big mess.
However that question is a red herring to the real critical part what has been missed; as the OP also has the following to say about the situation:
(…) While my gut reaction was to nuke it from orbit, they insist…
(…) I’m inclined to take immediate steps to bring this beast down (or at least prevent it from growing further) but I’m willing to believe I’m wrong.
HOLD YOUR HORSES!
This here is something that would make my figuratively speaking monocle drop into my figurative cup of tea.
I’ve been in similar situations and let me do tell you: DON’T fall for the urges to do so. Sure, you can drop the Nuke Hammer of Justice on the team but before you do so please humour yourself with the following bit of conundrum:
What will happen if you tell the team that their code sucks and to sod off?
(…or something like that but less offensively, it doesn’t really matter because they will be offended regardless what you do if you decide to go full force on them)
What is the current situation with the code base? Is it working? Then you’ll have big problems explaining to their clients that their code essentially sucks. No matter what reasons they have: as long as it is working, most clients don’t care about how the code is organized.
Also put yourself in their shoes, what would they do? Let me amuse you with the following very possible outcome:
Team Member #1: “So there is this guy telling us that we’ve been doing it wrong for the past years.”
Team Member #2 and #3: “What a jerk.”
Influential Team Member #4: “Don’t worry about it, I’ll just go to the management and report it as a harrasment.”
See what Influential Team Member #4 did there? He went to the management and reduced your karma in the company. He might be American-Italian, telling everyone to not worry about it, but then I’d be racist about it.
Painting the offending team into a corner with letting them admit that they did it wrong for so long is also a bad idea and lead to the same thing. You’ll lose respect and some office politics karma.
Even if you managed to get a bunch of people to sign off on this, in order to “teach the team a lesson”, remember that you’re doing this to fairly smart people who apparently get some things done. Once the code will be rewritten/refactored/dealt/whatever’d with and problems arise that you will be held accountable as you were the firestarter.
Being adversarial about situations like this is mostly a lose/lose game as it risks to become a vicious circle of blame games. This is a suboptimal outcome for this situation. Even if you win, you’re suddenly handed over the mess that someone else did.
There are Other (Much Mature) Ways
I was in a similar situation once, but then I took an arrow in the knee. So after a while with that sudden career altering arrow in my mind I got a book Driving Technical Change by Terrence Ryan. It lists several patterns of skeptics, the kind of people don’t act on good ideas. They are all most likely applicable in the OP’s case:
- The Uninformed – They genuinely did not know there are other ways or simply didn’t understand. Junior developers usually fit this category but not necessarily. (To be honest: I’ve met junior developers that would be more brighter than this.)
- The Herd – They knew better techniques than using partial classes but didn’t know that they were allowed.
- The Cynic – They just like to argue that having partial classes is better than your idea just for the sake of arguing. It’s easy to do that in a crowd instead of standing in front of a crowd.
- The Burned – For some odd reason they don’t like to create new classes, most likely they perceive it as too difficult.
- The Time Crunched – They’re so busy that they can’t bother to fix their code.
- The Boss – They think: “Well it works; so why bother?”
- The Irrational – Ok, they’re just completely crazy.
The book goes on with a list of strategies and so on, but in the OP’s case it’s a matter of persuasion. Going head strong with facts on the anti-pattern is not enough.
If it is in your interest to increase code quality, at least give the offending team chances to reiterate and rectify their own mess. Personally I would try to sway them by listening and asking leading questions, let them tell their story:
- What exactly is their god class doing?
- Have they run into any problems? Do they have any bugs? Suggest sane fixes without telling them to do so.
- If you a user of this god class as an API: Are there simpler ways to use the code than to use the whole thing? Suggest simpler examples, see how they react.
- Can you switch out some functionality with another, without having to write the function?
- Are they in need of some training? Can you convince management to give it do them or have lunch meetings about patterns and practices?
… and so on. Give them small suggestions so they can move forward. It takes time and will need some office politics grade elbow grease but patience and hard work is a virtue right?
1
The MSDN Partial Classes and Methods page suggests two situations for using partial classes:
1. To split a giant class into multiple separate files.
2. To have a place to put automatically generated source code.
2 is heavily used by Visual studio (e.g., for aspx files and for winforms). This is a reasonable use of partial classes.
1 is what your team is doing. I would say that using partial classes is a perfectly reasonable way to achieve modularity when you have giant classes, but having giant classes is itself unreasonable. In other words, having a giant class is a bad idea, but if you’re going to have a giant class, partial classes are a good idea.
Though if you can intelligently split a class into separate files for different users to work on, can’t you just split it up into separate classes instead?
1
So in the end, they are doing normal procedural development without any piece of OOP. They have one global namespace with all methods, variables and whatnot.
Either persuade them they are doing it wrong or get the hell out of there.
A utility class contains methods that can’t sensibly be combined with other methods to create a class. If you have 800+ methods, split into 135 files, it seems likely someone has managed to find some common methods…
Just because you have one utility class, doesn’t mean that you can’t have another utility class.
Even if you do have 800 mostly unrelated methods, the solution isn’t one big class and a lot of files. The solution is a namespace, some sub namespaces and a lot of little classes. This is a bit more work (you have to come up with a name for the class as well as the method). But it will make your life easier when it comes time to cleanup this mess, and in the meantime it will make intellisense easier to use (ie easier to discover where a method is to use it).
And no, this isn’t common (more the number of files than the number of free functions).
I don’t like this at all. However, maybe the partial classes made sense to the developers during development because it allowed concurrent development of this large number of methods, specially if there is not much dependency between them.
Another possibility is that those partial classes were built to amend an automatically generated core class. If this is the case, one should not touch them, otherwise the custom code would be whipped-out when re-generating.
I am not sure that anyone can maintain this with ease without some study. Now if you kill it, the number of methods will not go down. The number of methods and complexity, to me, is the real problem. If the methods really belong to the class, then having 1 huge file is not going to make your life easier, specially in maintenance, in fact, it may be more error-prone to expose all this code for silly mistakes such as wild card search and replace.
So be careful and justify the kill decision. Also, consider what testing will be required.
Its perfectly fine from a practical design perspective assuming the 135 files actually group methods together similar to what traditional classes would do. It’s only an average of 6 methods-per-file. Its definitely not the most popular or traditional way to design a project. Trying to change it will just create a bunch of problems and ill will for no real benefit. Making the project conform to OO standards will create as many problems as it solves. It is a completely different matter if the multiple files don’t actually provide any meaningful separations.
4
Beyond the answers already pointed out, partial classes can be useful in a layered design where you want to organize functionality that cross cuts your class hierarchy into separate layer-specific files. This allows one to use the solution explorer to navigate per-layer functionality (by file organization) and the class view to navigate per-class functionality. I would prefer to use a language that supports mixins and/or traits, but partial classes are a reasonable alternative if used sparingly, and shouldn’t be a replacement for good class hierarchy design.