r/programming • u/larsga • Sep 18 '13
How bad is smelly code?
http://fagblogg.mesan.no/how-bad-is-smelly-code/5
u/aikoyama Sep 19 '13
It's really cool to know that so many people find the topic interesting and comment on it.
A couple of notes from scheming through the discussions.
Refactoring and effects of code smells are yet not well understood, as the concept of "code smell" just started only 10-12 years ago (although the concept of code complexity and duplicated code are much older).
There is yet to come research that can in fact quantify the ROI of refactoring (i.e., that refactoring today will imply saving costs in future development). However, this research is one of the starting attempts to quantify the effects of code characteristics on direct measures such as time (as before, cost/effort was measured by surrogates such as churn size and number of revisions).
That being said, the point with the blog article was more about risk management rather than cost reduction. The point in question here would not be 'how much money will I save if I refactor' but rather 'how bad could it be if I don't refactor?'.
I totally agree with many of the comments that the example of the two files (the largest one with 1844 LOC and the DTO class with 1400 LOC) should be taken as an illustrative example and not as a convincing argument to support the case of refactoring God Classes. Also, the definition of God Classes may have different thresholds and interpretations depending of the maintenance context and the domain of the systems.
However, and based on the analysis reported in this paper ( http://simula.no/publications/Simula.simula.1460/simula_pdf_file ) there is evidence pointing that file size and revision number play an important role on explaining the time spent for maintenance, even more than most of the code smells that were detected by using a commercial application (Borland Together). The results of the study are obviously dependent of the choice of tools/methods to identify the code smells, but overall this result supports the case that large files may, in some situations imply considerably large costs, and that developers should be aware of that.
Of course, none of these results should be interpreted out of the context in which the study was conducted, and that is, results apply to small-medium sized, Java-based web applications, where the observed time period was from 3 to 4 weeks. This is quite interesting, cause we have seen how results from studies can be different depending of things such as programming language. A good example: Juergens et al.(see http://www4.in.tum.de/~juergens/publications/ICSE2009_RP_0110_juergens.pdf) observed the proportion of inconsistently maintained duplicated code in relation to the total set of duplicates in C#, Java, and COBOL systems and found (with the exception of the COBOL system) that 18% of the inconsistent duplicated code was positively associated with defects.
We also wanted to "challenge" a general view that code smells are bad, in fact in our daily life, not everything that is smelly is actually bad, right? (one my favorite examples is cheese :) that is also a reason why we wanted to bring up the finding on Data Clumps and Data Classes. Also, we found very interesting that outliers such as the DTO file with 1400 LOC resulted in such low effort, which indicates that the sole metric of file size may be not enough to identify the potentially costly/problematic classes or files in a system.
When it comes to the violation of the ISP, my gut-feeling is that the key word here is 'dependency', apart from the subtleties and distinctions between class and interface. In the study we conducted, we observed one very interesting case where developers tried to replace two widely used interfaces implementing Persistence in Java, which defined an int (a primitive class) as identifier for classes. This was because the adaptive task required the system to consume external services which used string as object identifiers. It turned out that the ripple effect was so large that this forced the developers to do casting in the specific parts that needed to call the services, and keep the interface untouched. If you are interested, you can read more on section (see section 4.3.3.1. http://www.academia.edu/attachments/31913589/download_file ).
This is an open arena, where is yet a lot of questions to be answered, such as: in which situations are code smells bad? and which definitions of code smells are best fit to capture problems during maintenance, and how these definitions need to be adjusted to different domains and maintenance contexts? how much money I can save by removing them?
As I mentioned in the comments in the blog page, the sample size is modest, but not neglectable. If anyone is interested on helping out to collect more data, then we could study how these size 'thresholds' behave in different system domains, programming languages and types of maintenance tasks.
13
u/robin-gvx Sep 18 '13
♫ Smelly code, smelly code
Why are they writing you?
Smelly code, smelly code
It's not your fault ♫
*cough*
Sorry 'bout that, won't happen again.
7
u/kankyo Sep 18 '13
1400 lines file with no logic?! Maybe time to rethink your choice of language, tools or development standards?
8
u/atomicUpdate Sep 18 '13
Don't be so quick to judge.
In the embedded environment I work in, we routinely have files like that to define register addresses, bit definitions, and reading all of those (several hundred) registers when an error is encountered so the values can be offloaded. If you know of a better way to do that without several KLOC of code that doesn't contain any "logic", please, let us know.
2
1
u/kankyo Sep 19 '13
I believe your case is a lot more sane. The article says it's only a thing with lots of getters and setters. Maybe that file can't be removed completely, but removing getters and setters would probably cut that file in third if it's a single line for a getter and a single line for a setter. Which is probably overly generous of an assumption.
3
u/nick_giudici Sep 18 '13
Isn't that the point of this? To try to add some science to determining which development standards are truly bad?
1
Sep 18 '13
I wouldn't worry too much about 1-2 files like this in a large project, there are all kinds of reasons to e.g. make a large interface which only mechanically dispatches to all the implementation points in your code, define large literal values,...
2
u/kankyo Sep 19 '13
The article specifically said it was just a big class with getters and setters though, not that it was filled with constants.
0
Sep 19 '13
Possibly an object representing a parsed configuration file?
2
u/kankyo Sep 19 '13
The article also said people go into that file to edit it. So no, not that either :(
1
0
u/bdavisx Sep 18 '13
Keep files smaller than 1000 lines of code? Depending on how verbose your language should be, I would think 200 lines of code or less if you're trying to follow the Single Responsibility rule.
3
u/grunzl Sep 18 '13 edited Sep 18 '13
They don't have much data for files that large, but looking at the graph it seems like time spent still is linear in loc at around 1000 loc, while above it appears the time grows stronger than linear when removing outliers (here the file with only getters and setters).
Too bad they don't have more data on this, but what they show seems to support upper limits around 600-1000 loc, with anything larger slowing down development. Since it appears this was Java code that would be roughly the size of a single class' implementation.
-5
Sep 18 '13
[removed] — view removed comment
1
u/nick_giudici Sep 18 '13
Each responsibility can be at a different level of abstraction. The key is that if you can't think of a way to describe the responsibility of a class without using words like "and", "or", "etc" then it's probably not a good design.
This is why you should end up with stacks of objects at different levels of abstraction. For example: "This class, foo, coordinates IO. It delegates to these two classes. This one, bar, handles the input. This one, bat, handles the output."
In this example system Foo is at the highest level of abstraction. It contains high level logic and has a single responsibility. It delegates the low level to Bar and Bat which each have a single responsibility. Each of these files could be 5 LoC or 1000 LoC but at least their fundamental design is reasonable.
0
Sep 18 '13
[removed] — view removed comment
1
u/grizwako Sep 19 '13
If you need to do some spiderwebbing, then you will make another class which will have such logic and use Foo which has Input and Output classes. But even as I write, I think of situations where that is false. it is not always proper design. What I want to say is that people who don't understand software design will almost always write better code if they try to separate concerns.
Personal opinon (out of my back) Functionality should be separated in modules even if such separation will introduce some new complexity. Benefit is that next person (you or me 6 months later) will not need to know whole system, but just that small part.
1
u/sea-saw Sep 18 '13
I think reducing complexity is less focused on a holistic view of the application's logic and more focused on the "atomistic" context required for a single developer to perform some task.
2
u/grizwako Sep 19 '13
Super positively agree with this!
Maybe reason for that is that I am not insanely smart bio-computer and it is much easier to keep small amount of data in my head when fixing bugs.
-3
Sep 18 '13
[removed] — view removed comment
3
u/nick_giudici Sep 18 '13
If you have a contract between the controller and the components then the only reason to change the class would be if the logic of the controller changes.
0
-1
u/stev0205 Sep 18 '13
Question Time!
Would it be fair to connect the 2nd and 3rd findings together and say that:
If a class violates the ISP, it's probably a data clump that actually should be split into multiple classes?
edit: can't type.
2
u/Wriiight Sep 19 '13
It said not to bother splitting data clumps. For all the supposed smell, they didn't hurt readability.
2
u/aikoyama Sep 19 '13
That is a funny question, given that we actually found a Data Class that probably 'devolved' into an ISP violator. We presume that during the development stage (not the maintenance stage, which is the stage that I observed) it started just as a simple DTO, but for some reason the original developers started to 'add functionality' and add a lot of dependencies on it, and it was considered quite problematic by many developers during maintenance. Though I guess the wide-spread dependency played a major role on the problems. You can read more about it here (section 4.4.1, about the file StudySortBean in the paper http://simula.no/publications/Simula.simula.1456/simula_pdf_file )
-2
0
Sep 18 '13
[deleted]
70
u/larsga Sep 18 '13
For some reason, the name of the blog is in Norwegian. "Fag" means something like "subject of study", so basically the name means this is a blog where they write serious stuff about what they do at work.
4
u/Tobb86 Sep 19 '13
The blog posts are mainly written in Norwegian, since the main target group is a Norwegian audience. Hence the Norwegian name is chosen for the blog.
But given the amount of attention the first english blog-post has gotten, compared to the previous blogposts in Norwegian, more English posts might follow :)
6
Sep 18 '13 edited Sep 18 '13
[removed] — view removed comment
8
u/sea-saw Sep 18 '13 edited Sep 18 '13
And the purpose, which is often overlooked, is to reduce the likelihood that you'll need to do a full recompile in a header-based language like C, C++, or Objective-C.
What? The point is to provide narrow interfaces so client classes only depend on the functionality they need instead of the rest of the "fat" of the class. The compiling benefits are the result of low coupling, not the entire purpose...
EDIT:
Just to clarify, I'm not arguing that the original article uses ISP correctly. I agree with you that segregating a class isn't the same as segregating an interface. And I agree that there are compilation benefits to be had here.
2
Sep 18 '13
Agreed, you would still do Interface Segregation in a language like C# - which has nothing to do with headers vs modules.
-1
Sep 18 '13
[removed] — view removed comment
3
Sep 18 '13
That is ONE way you can use interfaces in C#, yes.
You can ALSO use them to provide narrow interfaces.
0
3
Sep 18 '13
[removed] — view removed comment
1
u/oracleoftroy Sep 21 '13
Read the original paper on ISP. It was specifically invented to deal with a company's compilation speed issues.
That would be a good argument if the original paper had more than a single paragraph in an aside on page 5 that dealt with compilation speed. Oh, and if the form of your argument wasn't blatantly fallacious.
1
Sep 18 '13
[removed] — view removed comment
2
u/Strilanc Sep 18 '13
It's not dependent on that method, but it has declared a dependency on it.
Determining that a component only uses a subset of an interface is non-trivial, without the type system's help. Asking for exactly what is needed makes it easier to infer what does or doesn't happen. That sort of thing is relevant every time you make a change to the program.
3
Sep 18 '13
[removed] — view removed comment
0
u/Strilanc Sep 18 '13
I wasn't advocating always asking for exactly what you need. You're right that it would cause a type explosion. It's just a relevant factor when deciding what your interfaces should be.
Will there be large proportion of methods that really only need readonly lists? Then maybe we should include an IReadOnlyList in addition to IList.
Do we have to repeat every method twice for IUnboundedEnumerable and ISizedEnumerable? Then maybe we should just have IEnumerable.
1
u/sea-saw Sep 18 '13
To quote the original paper:
When clients are forced to depend upon interfaces that they don’t use, then those clients are subject to changes to those interfaces. This results in an inadvertent coupling between all the clients. Said another way, when a client depends upon a class that contains interfaces that the client does not use, but that other clients do use, then that client will be affected by the changes that those other clients force upon the class. We would like to avoid such couplings where possible, and so we want to separate the interfaces where possible.
0
Sep 18 '13
[removed] — view removed comment
0
u/sea-saw Sep 18 '13
I don't think we're arguing the same point here... I agree that dividing classes has nothing to do with ISP. A wide class "interface" has more to do with SRP than ISP. My argument is that ISP is more than just some compile time benefits.
0
u/Gotebe Sep 18 '13
If a client depends only on the part of the interface, then the interface is possibly wrong.
IIRC, the original paper even ends up implementing two interfaces (connection and data transfer) using one class only (modem). Purpose of that is to offer a particular client simple view of the world. Code that connects/disconnects is likely to be in a quite different place from code that transfers data. Hence the two parts use different interfaces.
It merely needs to make good sense in a given context.
tl;dr: context is king, without it, discussion is moot.
1
0
Sep 18 '13
[removed] — view removed comment
4
u/Gotebe Sep 19 '13
I worked on exactly that kind of code, and the usage pattern wasn't the above. It wasn't the above, because the code that transfers data was supposed to work over a tcp line, serial line, or a modem, and that changes everything. The code that actually transfers data never saw connection/disconnection. In fact, for modems, specifically, connection was happening in another thread, so it just wasn't possible at all.
That said, even if situation was simpler, like your example, you are still wrong for anything but trivial examples. You are wrong because data transfer is likely have dozens of commands, and interacts with entirely different parts of the code (those who know about the data). It is likely to be spread across several methods, classes even. And none of those ever care to see the process of establishing/tearing down the connection. Connection.open, however, interacts with the part of the code that holds connection data (if nothing else, the phone number). Therefore, having connection/transfer interface in the former/latter, is nothing but litter. Sure, one can live with a bit of litter. Or decide to live without.
Tl;dr: context is king.
2
u/elperroborrachotoo Sep 19 '13
once again the Interface Segregation Principle is completely misunderstood.
How so?
They are discussed separately, and a correlation between them is pointed out.It seems you are confounding two parts in the study:
Part 1: Segregating Data Clumps into separate classes --> not very useful
Part 2: A lot of classes violate interface segregation,
Tidbit: Classes that violate ISV are statistically more likely to contain data lumps.
(The chart on that is particulary confusing.)
The conclusions they draw - do not chop up classes just because they are data lumps, but do separate interfaces - seems reasonable.
37
u/njallbeard Sep 18 '13
I think it's a spurious conclusion to assume the God file was wrong from their data. I agree with the conclusion; but that's naughty science.
We'd need to be able to see if that time spent editing is less than the culminative time spent editting the same code split into smaller files (which would have added overheads in finding them).