28

We have here a large legacy code base with bad code you can't imagine.

We defined now some quality standards and want to get those fulfilled in either completely new codebase, but also if you touch the legacy code.

And we enforce those with Sonar (code analysing tool), which has some thousands violations already.

Now the discussion came up to lower those violations for the legacy. Because it's legacy.

The discussion is about rules of readability. Like how many nested ifs/for.. it can have.

Now how can I argue against lowering our coding quality on legacy code?

Kromster
  • 468
  • 1
  • 6
  • 17
keiki
  • 481
  • 6
  • 10
  • 2
    is the discussion about lowering the thresholds of the tool? ...or did I misunderstand it. It's written in a confusing way. – dagnelies Jan 19 '17 at 08:34
  • 13
    [How do I explain ${something} to ${someone}?](http://meta.softwareengineering.stackexchange.com/a/6630/31260) – gnat Jan 19 '17 at 08:39
  • 2
    "the discussion came up to lower those violations for the legacy", "how can I argue against lowering our coding quality" – I am confused. If you *lower* the number of violations, you *increase* coding quality. – Jörg W Mittag Jan 19 '17 at 09:05
  • 4
    The question has some very unclear parts. "lower those violations" - do you mean to lower the actual number of violations (by rewriting the old code), the number of rules tested by Sonar to the legacy code base, or the number of rules of your coding standard you want to follow when changing something in the legacy code? – Doc Brown Jan 19 '17 at 10:00
  • 4
    Also we have a legacy product of terrible code quality. Since it will be replaced by the new product, there is almost no value in cleaning that old code up. That means: we accept a lower standard in the legacy code base. – Bernhard Hiller Jan 19 '17 at 10:10
  • Sorry for the confusion. It's about lowering the results of the tool, which in my opinion is then also lowering the code quality. Because people then tend to not use good code practice for newer code written in legacy code base. – keiki Jan 19 '17 at 10:27
  • 4
    @keiki: still unclear: is the new code base separated enough to have different validation rules for the old and the new code? What about the parts you change in the legacy code base? Is your problem that lifting the changed parts to the higher standard causes too much effort, or is it you cannot separate those parts in Sonar's validation, to make it possible to validate only those changed parts in full? – Doc Brown Jan 19 '17 at 12:46
  • 1
    Best answer for this question: [Refactoring -- Not on the backlog!](http://ronjeffries.com/xprog/articles/refactoring-not-on-the-backlog/). The 'standards' you have are that you make the code better when you touch it and you don't 'hack things in'. You clean the parts you edit often (and in turn benefit from their cleanliness because you are editing them often), and you leave alone the things that work and don't need changing. – Nathan Cooper Jan 19 '17 at 15:20
  • 2
    Oh I promise you, we *can* imagine... – skrrgwasme Jan 19 '17 at 15:44
  • 1
    "And we enforce those with Sonar (code analysing tool), which has some thousands violations already." - are those thousands of errors when someone ran the legacy codebase through the tool, or thousands of errors caught as people tried updating that legacy codebase without applying the standards? Very different situations - enforcing your policies and standards for all work that gets done vs. arbitrarily going back and overhauling the entire codebase just to make it meet the new standard instead of as-needed/updated. – PoloHoleSet Jan 19 '17 at 16:45
  • 1
    "Now how can I argue against lowering our coding quality on legacy code?" Why is it important to you? You haven't explained why it's important to you. Explain it to us. When you do that, then you'll be explaining it to management. – Andy Lester Jan 19 '17 at 17:55
  • "bad code you can't imagine"? ["I don't know, I can imagine quite a bit."](https://imgflip.com/i/lfr4g) – Scott Weldon Jan 19 '17 at 20:27
  • You have a key misunderstanding in your question. Nested ifs and fors affect far more than readability. There are lots of code paths through a deeply nested code block. That makes it hard to change safely, hard to test, and hard to refactor. When you refactor to reduce the nesting, you make all of those things easier. – dcorking Jan 20 '17 at 14:40

8 Answers8

73

Code is just a means to an end. What that end might be varies, but typically it's profit.

If it is profit; then to successfully argue anything you want to show that it will improve profit - e.g. by increasing sales, reducing maintenance costs, reducing development costs, reducing wages, reducing retraining costs, etc. If you can't/don't show that it will improve profit; then you're mostly just saying it'd be a fun way to flush money down the toilet.

Brendan
  • 3,895
  • 21
  • 21
  • 15
    I believe there is another dimension in this discussion that is professionalism, in many profession, if your boss ask you to cut corners, you can refuse to do so based on moral ground (sometimes even the law back you up), I don't understand why software developers should behave differently. – Alessandro Teruzzi Jan 19 '17 at 11:54
  • 21
    @AlessandroTeruzzi You can refuse to do anything on (personal) moral grounds anywhere irregardless of professionalism. Does not guarantee you will keep working in that place though. – Kromster Jan 19 '17 at 13:00
  • Also, codebases often being extremely complicated, it'd be difficult to prove any of the things mentioned even though a seasoned developer might know by experience that cutting corners will get costly in the long run. – mkataja Jan 19 '17 at 13:01
  • 17
    Nothing wrong in this answer, but nevertheless it is IMHO not useful for most real world situations. Improving the quality of a code base will often reduce maintenance cost, especially on the long term, but these effects are seldom quantifiable. Therefore it is often a strategic decision. – Doc Brown Jan 19 '17 at 13:25
  • @Kromster I haven't said personal, professionalism is actually anything but personal. It is a code of conduct that a profession established to guarantee quality and reliability. – Alessandro Teruzzi Jan 19 '17 at 13:28
  • 2
    @DocBrown I tentatively agree, but I think number-of-nested-ifs-driven development, a la OP, isn't an effective approach towards improving a legacy codebase. – brian_o Jan 19 '17 at 15:52
  • @Alessandro Teruzzi: Where is the lack of professionalism or cutting corners here? Are we arguing that the original developers of the code weren't "professional"? What the OP apparently wants is to re-write the code according to his ideas of what constitutes "good" code - which all too often seems to be defined by personal taste or habits acquired as an undergrad, rather than any objective measure. He needs some objective way to demonstrate that it's worth the cost: otherwise, "If it ain't broke..." :-) – jamesqf Jan 19 '17 at 19:02
  • @brian_o: the metric which counts "number-of-nested-ifs" is called cyclomatic complexity, and of all metrics which are used to evaluate code quality, it is indeed one of the more useful ones. So I would not underestimate its value. – Doc Brown Jan 19 '17 at 21:50
  • @DocBrown: Convincing someone that it'll increase profit has very little to do with whether or not it actually will increase profit and nothing to do with accurately quantifying the amount. For a (relatively extreme and ill-advised) example; if you know for a fact that it will definitely reduce profit, you can still use "marketing tricks" and convince someone it will increase profit (even though you know it's a lie). – Brendan Jan 23 '17 at 02:23
44

Me, myself, and I, have been both a producer and maintainer of legacy code. If your tool's generating "thousands of violations" (or even hundreds for that matter), forget the tool, it's inapplicable to the situation...

I assume the original developers are long gone and unavailable for discussion. So nobody's around who understands the why's and wherefore's of the design and coding style. Correcting hundreds or thousands of violations isn't going to be a matter of rewriting a few lines of code here and there. Instead, it undoubtedly requires refactoring/re-functional-decomposition. You try doing that to any large existing codebase, without intimately understanding its current design, and you're bound to introduce a whole new set of bugs/problems/etc. Just a new can of worms even worse than the one you now have (or worse than your tool >>thinks<< you now have).

The only sensible approach to address "thousands of violations" would be to rewrite from scratch. A long and expensive effort, and almost impossible to sell to management. And in this case they're probably right...

Legacy code typically just requires tweaks. Like for y2k, or when stocks went from 256th's to decimal. I did loads of both that cr*p. And lots of other similar stuff. It's typically pretty "pinpoint" in that you can "read through" the sometimes-bad style, bad functional decomposition, bad etc, and locate the collection of places that need to be modified. And then, what happens "between those places", i.e., what's the more high-level flow, can remain a mystery to you. Just make sure you understand the localized functionality you are changing, and then test, test, test for any side-effects, etc, your localized knowledge won't be able to anticipate.

If you're unable to see your way through code like that, then you may not be the best person to maintain legacy code. Some people can start with a blank screen and write beautiful programs, but can't start with a large codebase of other people's code and maintain it. Other people can maintain code, but can't start from scratch. Some can do both. Make sure the right people are maintaining your legacy code.

The occasional times you might want to redesign and rewrite your legacy codebase from scratch is when the business (or other) requirements change to such an extent that seat-of-the-pants "tweaks" just can't accommodate the changed requirements any longer. And at that point, you might as well just start off by writing a new functional requirements document in the first place, making sure all stakeholders are on-board. It's basically a whole new ballgame.

The one-and-only >>wrong<< thing to do is try treating legacy code maintenance the same way you'd go about new development. And that one wrong thing seems to be exactly the path you'd like to go down:) Take my word for it, that ain't what you want to do.

John Forkosh
  • 821
  • 5
  • 11
  • 2
    This answers a question "Should the legacy be rewritten". But OP is not asking how to fix the warnings or if rewrite should happen. The question was about quality standard - basically a requirement for every change not to increase count of validation warnings. – Basilevs Jan 19 '17 at 15:05
  • 9
    This directly addresses the question, which is *not* about rewriting. The OP wants to argue for 'treating legacy code maintenance the same way you'd go about new development'. This answer says "WOAH! You shouldn't do that!" May not be the response the OP or you wanted to hear, but completely responsive to the question. – Jonathan Eunice Jan 19 '17 at 16:29
  • 1
    @Basilevs (and @ JonathanEunice). What Jonathan said. And note that I started off by directly saying "forget the tool, it's inapplicable to the situation", which directly addresses the question, although negatively. But like the saying goes, if you're going to criticize, offer constructive criticism. So I couldn't just say, "don't do that". I also had to suggest what to do instead (and that got a little more long-winded than I'd anticipated when I started writing it). – John Forkosh Jan 19 '17 at 22:54
  • 1
    When working with legacy code projects, I sometimes like to design an interface layer that sits between old and new code. That allows a clean division between the old and new parts, and makes it easier to troubleshoot the new parts than if they were glommed in with a bunch of code I don't understand. – supercat Jan 20 '17 at 13:33
  • @supercat If that can be done, it's certainly a nice approach. But recalling various of my y2k remediation projects, you just had to "dive into" the middle of existing code and mess around with it. No possible (re)factoring into old/new possible (without >>massive<< rewrite). For example, rather than add two bytes to date fields for a four-byte ccyy-formatted year, requiring wholesale updates to massive databases, just interpret yy>50 as 19yy, and yy<=50 as 20yy. But the only sensible implementation for that involves lots of little changes every place yy is used. – John Forkosh Jan 20 '17 at 21:32
  • @JohnForkosh: Yes, but rather than insert code in each such place to do the computation, I'd be inclined to refactor `date1 > date2` into `sc17tDateGreater(date1, date2)`, confirm that the code still worked as it had before, and then change the `sc17DateGreater` function so it could easily be made to use old or new behavior. All new functions would be marked with a prefix that didn't appear anywhere in the old code, and changes to the old code would be--to the extent possible--limited to calling functions at the new interface layer. – supercat Jan 20 '17 at 22:08
  • @supercat Yes, sometimes. And sometimes not, e.g., when I had to reformat reports that printed a literal "19" followed by yy. And sure, we typically wrote functions (or just #define'd macros) like you describe, primarily to encapsulate the yy>50 so that 50 could be easily/globally changed (to a larger number) at some future date. But that's trivial. The real job is carefully looking everywhere to find and understand all the little here-and-there places such tweaks are needed. And it's often not as cut-and-dried as you might think. Not really challenging, more like "death by a thousand cuts." – John Forkosh Jan 22 '17 at 11:13
13

The question is not how good or bad that code is. The question is how much benefit you get from how much investment.

So you have a tool that finds thousands of things that it doesn't like. You have the choice of ignoring the tool's results, or to invest work to change thousands of things. That's a lot of stuff. People will not be focused, not while making the changes, not while reviewing them, and this will not be properly tested because it takes ages to figure out how to test (or just to try out) every change, so there will be bugs. So until you found and fix those bugs, you invested lots of time and money with overall negative result.

On the other hand, tools can find things that they not just "don't like", but that are bugs or potential bugs. If the legacy code is still in wide use, then the right tool can actually find bugs. So turning compiler warnings on one by one, checking whether they are useful (not all are useful) and fixing those warnings can be worthwhile.

gnasher729
  • 42,090
  • 4
  • 59
  • 119
  • 2
    I once came to a project which was done quickly (ignored compiler warnings etc.). The project was a mess, there was a bug. A number was overflowing. The team was searching for 2 weeks. I set up the compiler environment with all warning flags on (the count went from 500 to multiple thousand warnings). I went through all the warnings. After just 3h I had 0 warnings. For some reason the code worked. But we didn't know why. I committed the code at every change to my branch. After a bit of searching I found it. An implicitly declared function, which made C garble the return value. – CodeMonkey Jan 19 '17 at 12:07
7

If it ain't broke don't fix it

This practically ought to be tattooed on every software developer and manager so that they never forget it.

Yes, it breaks all modern coding conventions. Yes, it's written in FORTRAN-77 with a wrapper in C so it can be invoked from Python. Yes, those are unstructured GOTOs. Yes, there's one subroutine three thousand lines long. Yes, the variable names only make sense to a Croat. etc. etc.

But if it has been tested in anger over a decade or more and has passed, leave it alone! If the modification needed is small, then keep it as small and as local as possible. Anything else runs the greater risk of breaking something that did not need fixing.

Of course, sometimes you find that it really is an unmaintainable kluge and that the only way to accomodate the "small" modification is to rewrite from scratch. In which case you have to go back to whoever is asking for the change and tell him what it will cost (both in dollars or man-hours for the rewrite, and in blood sweat and tears for the inevitable new bugs).

A long time ago there was a rash of failures of one of Digital's disk drives. They ended up recalling and replacing all of them at gord knows what cost. Apparently there was a blob of glue holding a filter inside the HDA. The engineering manifest said of the glue "Non-parametrically specified, NO SUBSTITUTES ACCEPTABLE". A bean-counter "saved" under a cent per disk drive by sourcing some other glue. It gave off a vapour inside the HDA which killed the drive after a couple of years in service. It wasn't broke until he fixed it. Doubtless "it was only a tiny change..."

nigel222
  • 255
  • 1
  • 3
5

There is definitely no point to reduce the quality level for legacy code. There is also no need to fix warnings immediately. Just ensure that all your "new" changes in every component of your project are following high quality standard. I.e. no commit should increase warning count ever.

It is an uniform and dead simple approach.

It allows old legacy code to work as is (because no unnecessary modifications are done to it). It ensures new code is of high quality. No additional cost is involved.

Basilevs
  • 1,221
  • 9
  • 11
  • I might take exception with the word **ever** at the end of your first paragraph. If, say, a few dozen lines of changes are needed in the middle of a several-hundred-line function whose style is raising warnings, I'm still going to try and follow the existing style, as long as it's not flawed to the extent of unreadability. I want to make life as easy as possible for the next poor guy who comes along and has to munge through the stuff. Mixing styles for no other reason than to satisfy some tool's standards is itself bad style. Instead, follow the original standards/style as much as feasible. – John Forkosh Jan 22 '17 at 11:30
  • I've said commit, not line or function. Supposedly you clean-up enough mess around in you edit, to have a budget for a.problem place. – Basilevs Jan 22 '17 at 12:05
3

Risk control….

  • The code in the large legacy codebase has been tested at least by real life usage of the software.
  • It is very unliky the code in the large legacy codebase is covered by automated unit tests.
  • Hence any changes to the cold in the large legacy codebase is high risk.

Cost….

  • Making code pass a code checker while you are writing the code is cheap
  • Doing so at a later state is much more costly

Benefit

  • You hope that keeping to a coding standards leads to a better design of code.

  • But it is very unlikely that having someone spend many week changing code just to remove warnings from a coding standard checking tool, will result in an improvement to the design of the code.

  • Simple code is clearer and tends to be easier to understand, unless the complex code is split into short methods by someone that does not understand it….

Recommendation….

  • If a section of code is shown to have lots of bugs in it, or needs lots of changes to add additional functions then spend the time 100% understanding what it does.
  • Also spend the time adding good unit test to that section of code, as you have a proven need for them.
  • You could then consider refactoring the code (you now understand) so it passed the new code checking too.

Personally experience….

  • In other 20 years of working as a programmer, I have never seen a case where blindly changing old to remove all output from a new coding standard checker had any benefits apart from increasing the income of the contractors that have been instructed to do so.
  • Likewise when old code has to be made to pass code reviews (TickIt/ISO 9001 done wrong) that required the old code to look like the new coding standard.
  • I have seen many cases where using coding standard checking tools on new code have had great benefits by reducing ongoing costs.
  • I have never seen a company fail to the costs of maintaining old code that is used in a product with lots of customers.
  • I have seen company fail due to not getting the income from customers by being too slow to market….
Ian
  • 4,594
  • 18
  • 28
0

Is the discussion about lowering the thresholds of the tool? ...or did I misunderstand it. It's written in a confusing way. If it is not, please discard my answer.

IMHO it would a good idea to reduce the tool's sensibility. Why? Because it would highlight the most hairy pieces of code. The alternative is producing reports with thousands of violations, becoming useless by its sheer size.

...other than that, just talk about it with the people involved and hear out their reasons too.

dagnelies
  • 5,415
  • 3
  • 20
  • 26
0

I would try to separate the legacy code from the new clean code. In e.g. Eclipse you could do this by putting them in different projects which use each other. 'clean'-project and 'legacy'-project.

This way you can analyze both projects in sonar with different quality standards. The standards should only differ in the importance of the rules. The rules itself should be the same. This way you can see in the 'legacy'-project what needs to be 'fixed' to meet the standards of the 'clean'-project.

Whenever you managed to lift some legacy code to the higher standards you can move it to the 'clean'-project.

In every day work you cannot accomplish to lift every class you touch to the 'clean'-project standards because of the lag of time. But you should try to get there in little steps by trying to improve the legacy code a bit every time you touch it.

MrSmith42
  • 1,041
  • 7
  • 12