views:

492

answers:

8

I have a dejavu feeling when reading [What to do about a 11000 lines C++ source file?] post, but I don't think I can start taking action on my own since I do not have the authority to take action. So I think the first step is to convince people in the organization that big lump of code is bad.

I have a similar situation where there is a single class with 11975 lines of code and every time there are new features, there is a high probability that this class will get bigger and bigger.

A: 

Having a huge class isn't bad. If the programmer can manage and understand it, then it is a perfect class. If the programmer is completely lost when he/she tries to change or add something to the class, then it is bad.

With programming, it's all about preference. If they prefer to have a single class of 11975 lines of code, and they can manage it, then it's their choice.

But yes, it is generally bad.

Alexander Rafferty
David Gelhar
by "programmer", I mean the collective programmer. So if thr 2, 3 or 100 people that work on the project all want that kind of structure, so be it.
Alexander Rafferty
Even if it's readable, your compile time will be unacceptably long. Not to mention that you are making all your tools tuned to work with many smaller files harder to use.
Alex B
If it was broken into 120 classes, it would still compile slowly with the same length of code.
Alexander Rafferty
@Alexander but incremental recompilation time would take much less time, which is critical for compile-test cycle.
Alex B
@Alexander Rafferty: Do you need to modify all 120 classes whenever you need to change something?
casablanca
true. I never said I was pro-one-enormous-class. I just said that people have different methods.
Alexander Rafferty
11k is peanuts to any decent compiler.
David Lively
+13  A: 

You have my sympathies. Any class that is that huge is inevitably breaking the Single Responsibility Principle, making it difficult to maintain.

My best advice is to lead by example:

  • Keep your new code small.
  • Refactor mercilessly when changing code to reduce the size of classes and functions.

Your code will shine like a beacon compared to a the legacy code and your team will understand which code is easier to maintain.

Johnsyweb
Not necessarily. Some monolithic tasks are complex and shouldn't be refactored. Line count is not a reliable measure of design quality.
David Lively
I think when you reach ludicrous extremes like 11,000+ lines for a single class that it's a pretty reliable indicator of crap code.
JUST MY correct OPINION
@David: Yeah, but 11,975 lines of code? What single task could your class possibly be doing that requires that much code?
musicfreak
@David Care to give an example? No complex monolithic task need to take over 10,000 lines of code.
Alex B
@David: I disagree with your down-vote justification. Complexity is **a good reason** to refactor! Had you talked about underlying performance characteristics I may have agreed with you... Perhaps you could provide us with an example of your own monolithic tasks by way of justification?
Johnsyweb
@David: I wholeheartedly agree that line count (alone) is not a reliable measure of design quality. An 11 KLOC class has a pretty bad smell however!
Johnsyweb
I agree about the smell. :-) but, never say never.
David Lively
Just to be clear: I agree that *nearly all* tasks should be broken down to smaller components. However, in a task which is predominantly linear (like some kinds of simulations and calculation pipelines), refactoring would consist only of moving code into classes (or, more likely, methods) which are inherently tightly coupled. Moving those methods *outside* of the class in question wouldn't make sense in that situation. Sometimes refactoring for the sake of refactoring, or to keep LOC lower, just increases maintenance complexity. Glad I didn't post that as an answer!
David Lively
@David I agree with that last comment, but its usually the case that you can pull code out of it and the end result is a Lot simpler code.
eglasius
+3  A: 

If this class consists of many smaller methods AND these methods actually belong to the class i.e. they are cohesive to each other and the class, then this is not necessarily bad.

The mere fact that a class is large does not make it automatically bad.

What is more important is avoiding a so called "god class" that contains many methods that have no conceptual relation to each other or the class itself.

Keeping the size of the individual methods down is also worth focusing on because most developers will need to "load" a whole method into their brains at one time, not the entire class. So the more concise the method the easier it is for developers to maintain.

You should gather metrics about the number of support issues that can be traced back to code in this class. Also, how long to developers spend making changes to this class compared to other smaller classes? Once you have these metrics you are in a better position to explain to management why such a large class is bad.

In the situation of a large class with a small number of large methods I would use refactoring techniques described in Martin Fowlers book, along with unit tests to ensure no new bugs are introduced.

Once you've refactored a huge method down to a number of simpler understandable methods and have successful unit tests, simply demonstrate this to your colleagues.

At this point, if your colleagues disagree or cannot see a general improvement in the understandability and maintainability of the refactored code, you are dealing with a political/personality problem. In that case it's up to you if you want to continue working in such an environment.

Ash
The first time through, I read it as 'need to download a whole method....' :)
Chubsdad
+10  A: 

11975 (or is it 12000 already :)) lines of class implementation. Is it bad? Cerainly looks so. But...

It depends. Typically classes which implement the Mediator pattern or the Facade pattern tend to be very large. They have very complex routing / communication protocol. "God" like classes also tend to be very huge and monolithic. In short, A class may actually be large but that may not be a problem.

So instead of focussing on the LOC as a parameter, focus on the functionality / design of the class. Is the class violating Single Responsibility Principle? LOC alone cannot be the only deciding factor to conclude if a class is really huge. Look at other metrics e.g. Cyclomatic Complexity.

However if you conclude that this is really bad in your project's context, you must have a strong reason to convince the relevant stakeholders. For example,

a. Does this class have many bugs?

b. Do this class bug fixes always introduce regression defects? (High coupling, Low cohesion?)

c. How long do defects take to be fixed in this class? How does this compare with other modules?

d. Generate a few UML diagrams for this module to illustrate the important issues (e.g. coupling).

e. Read as much on metrics / consult your Quality / Metrics / QA team and generate sufficient data points. An example of OOAD metrics is here but I am sure there are plenty.

EDIT 2 (some minor changes)

f. Get initial support from some key stakeholders in your sphere of control. Next, get, someone who can present these facts well!. I have seen many things fail at this stage!.

Good luck!

Chubsdad
I think the people here will probably do http://google.com/search?q=Mediator+pattern , http://google.com/search?q=Facade+pattern , http://google.com/search?q=Cyclomatic+Complexity , and http://google.com/search?q=Single+responsibility+principle when you mention it to them (even I do that). At this point, you can probably guess, there is no QA team for the codes written. The QA is by running the apps continuously overnight / during weekend.
afriza
+4  A: 

Tell them to summarize what the class does\represents in a minute.

nitroxn
Wow, this works surprisingly well. For new classes, a short description in the header allows me (for now, only me) to focus on (1) what the class should be responsible for; and (2) what functions/functionalities should go to the class.
afriza
+1  A: 

Here's my initial reaction:

  1. Are there no places to refactor to reduce the SLOC? Have you tried refactoring first, before anything else?
  2. Is it handling more than one responsibility or as another poster stated is it a Facade?
  3. Are there many pre-processing macros built into the code thereby making it "smaller" than it really appears to be?
  4. Is there any SQL-type query statements being constructed? I've seen these take up quite a large amount of space and quickly add to the SLOC without being necessarily indicators of poor code quality.
  5. How has this code been assembled and under what time pressures? What is the history of the code? I've only seen code like this (and produced some myself) when the schedules have been nigh borderline insanity. Perhaps it was a "...created in 6 days, the 7th to rest" sort of monstrous rush but never fixed.
  6. Are the other developers actually developers?

There are no suggestions here except refactor what you can. Find whatever functionality violates D-R-Y and then scrub it clean.

wheaties
A: 

Convincing anybody to change existing implementation (especially if "it works") is not easy.

I would start with asking opponents to do the trivial task (from my experience with such extreme classes, I believe there are few large enough methods) like adding something to one of huge methods. It should teach them one lesson: finding the right spot will be really painful. Then, you could ask: "imagine you supporting this class, is it going to be an easy task?".

You may try different approach, but it will probably fail: ask them to write unit test for this code. Or simply ask them to think of any...

Paweł Dyda
A: 

Basically those people are not having knowledge on oops? If you do OOAD before starting the project is the better Idea to avoid this, from my experience I can say that every programmer don`t have knowledge on oops, so obviously they cant write a better code.

This is where the difference comes with a programmer and analyst-

Sandeep Manne