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?

closed as primarily opinion-based by gnat, Robert Harvey, user22815, BlueRaja - Danny Pflughoeft, Andres F. Jan 19 '17 at 19:30

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 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 8:34
  • 13
  • 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
  • 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
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.

  • 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 says support Monica 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
  • 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
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.

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

  • 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..."

  • 2
    Do you mean Western Digital? Was it this recall? Can you provide any sources to back up your story? – Lightness Races with Monica Jan 19 '17 at 15:29
  • 3
    Pretty sure he means Digital Equipment Corp a faint glimmer of which may still live deep in the black heart of Hewlett Packard. – John Hascall Jan 19 '17 at 18:11
  • 1
    Yes - Digital also known as DEC. – nigel222 Jan 19 '17 at 18:21
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.

  • 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….
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.

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.

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