How do you deal with out-of-control code?

[Borg]

I would like to get some ideas for solving problems that occur on large coding projects. I'll just describe one for now but I have plenty of other examples. Please feel free to offer your suggestions, describe problems that you would like to find a solution for or derail it off in another direction. :tongue2:

Some background. I've been working for the past year on a large Java web project that has been under continuous development for over 10 years. The program has seen many developers of various skill levels come and go over the years. Management has been pretty much the same group the entire time and can be set in their ways sometimes. The code is easily in the millions of lines with over 80,000 files in the repository (yes, the term bloatware comes to mind). Obviously, I can't fix everything in the code that has been wrought over the last ten years but perhaps there are ways to gently guide the program in the right direction. I'm not looking to blast how others code - I'm trying to find ways to point them in the right direction. That includes myself if I'm doing something that I shouldn't.

First, an example from the Java If Else Indentation thread. Several developers have their Eclipse IDE set to automatically limit the length of a line to 80 or less characters. When they save the file, it results in deeply nested brackets getting turned into junk like the bolded if statement in the made-up example below:

    private void someRandomMethod(String sql) throws SomeException {
        try {
            if (someBoolean) {
                ResultSet resultSet = null;
                Connection connection = null;
                Statement statement;
                try {
                    connection = getConnection();
                    statement = connection.createStatement();
                    boolean success = statement.execute(sql);
                    if (success) {
                        resultSet = statement.getResultSet();
                    }
                    else {
                        throw new Exception("Statement failed to execute.");
                    }
                    ResultSetMetaData rsmd = resultSet.getMetaData();
                    int columnCount = rsmd.getColumnCount();

                    while (resultSet.next()) {
                        String data = String.valueOf(resultSet.getString(
                            "SOME_LONG_TABLE_NAME"));
                        [B]if ((isOneTypeOfData ||
                            hasPassedChecksInThisLongMethod(firstVar,
                                secondVar, "Some random text", data)) && data
                            != null && (checkWhatThisMethodSays() ||
                            checkThisIfTheFirstDidntPass(data)) && (anotherBool
                            && todayIsNotTheFirstOfTheMonth)) {
                        }[/B]
                        else{
                            ...
                        }
                    }
                }
                catch (Exception e) {
                    e.printStackTrace();
                }
                finally {
                    ...
                }
            }
            else {
                ...
            }
        }
        catch (Exception e) {
            e.printStackTrace();
        }
    }

I wrote the code in the if statement so that it would specifically break weird according to the 80 character limit. Also, the original if statement may not have even started out being too long but grew as various changes or fixes were implemented. The question is, how do you try to guide people away from ingrained coding styles that clearly leave a mess for the next poor guy that has to try to understand it? Meetings? Rants? Bribery?

 

[Simon Bridge]

If it is your project, you can just set the coding style you want as a rule - any code submitted that breaks the rule gets returned for correction. Coders who disagree are free to take their work elsewhere.

You could see it as an automaton problem to identify the kinds of things you don't want to appear... have a script identify and maybe correct the offending bits.

If it is not your project, you must appeal to the project administrators.

Whatever you do is going to be hard work.

 

[AlephZero]

In my experience "gentle guidance" won't work.

Find a way to make your code repository automatically reformat everything that is checked into it to conform to the project standards (and if you have to bend your personal favorite standards a bit to make that happen, just live with it). If you can't do that, at least get it to reject every check-in that doesn't follow the standards.

People will soon get tired of reformatting everything they check out from the repository to match their own whims and fancies, and accidental oversights will get fixed automatically.

Of course this will cause some "short term" disruption (especially if there are some bugs in the reformatting code!) - the best time to start doing this is at the start of the project, not ten years in.

(But if your automatic code build-and-test system isn't good enough to compare the software before and after you reformat everything, the best plan is probably forget about the whole idea, and start job-hunting!)

 

[phinds]

In my experience "gentle guidance" won't work.

Find a way to make your code repository automatically reformat everything that is checked into it to conform to the project standards (and if you have to bend your favorite guidelines a bit to make that happen, just live with it). If you can't do that, at least get it to reject every check-in that doesn't follow the standards.

People will soon get tired of reformatting everything they check out from the repository to match their own whims and fancies, and accidental oversights will get fixed automatically.

Of course this will cause some "short term" disruption (especially if there are some bugs in the reformatting code!) - the best time to start doing this is at the start of the project, not ten years in.

(But if your automatic code build-and-test system isn't good enough to compare the software before and after you reformat everything, the best plan is probably forget about the whole idea, and start job-hunting!)

Excellent advice. I couldn't have said it better.

 

[D H]

In my experience "gentle guidance" won't work.

I agree with this entirely.

Find a way to make your code repository automatically reformat everything that is checked into it to conform to the project standards (and if you have to bend your personal favorite standards a bit to make that happen, just live with it).

I disagree with this wholeheartedly. Borg has a ten year old system comprising ~1 MSLOC. It's almost a certainty that there is old legacy stuff in that code base that is absolutely atrocious. There's also going to be some old legacy code that, other than having lines that are 300+ characters long looks OK. The automatic reformatting will change that moderately OK code into incredibly bad looking code.

Leave the legacy code alone. If it needs to be changed, that's maybe the time to make it comply. Note that I said maybe. If a bug report results in a change to one line, as a reviewer I'd want to see a diff that shows that that one line is the only line that was changed. Let those old lines of code rest in peace.

New code and changes to code that is already in compliance, yeah, now your auto reformat makes sense. It also makes sense when some piece of garbage legacy code needs extensive modifications. But there has to be a way to check in code that bypasses the auto reformat.

 

[Borg]

Thanks for all of the advice.

Leave the legacy code alone. If it needs to be changed, that's maybe the time to make it comply. Note that I said maybe. If a bug report results in a change to one line, as a reviewer I'd want to see a diff that shows that that one line is the only line that was changed. Let those old lines of code rest in peace.

That's pretty much what we do right now. I suspect that there is also a ton of dead code that doesn't get used and there is definitely a lot of duplicated code. It's hard to know what can be deleted because the architecture that is in place tends to obfuscate what's getting accessed. Debugging and adding new features are also a pain because of this.

New code and changes to code that is already in compliance, yeah, now your auto reformat makes sense. It also makes sense when some piece of garbage legacy code needs extensive modifications. But there has to be a way to check in code that bypasses the auto reformat.

The code doesn't get auto-formatted when it's checked into the repository. It's just a few people who have their IDEs set up to do it whenever they save a file. Most people are ordering their code in a logical manner and not grouping things in an odd, unreadable manner so that their check-ins are fine - If someone does go to 90 or even 100 characters on a line, it's usually still readable. I think that the few whose IDEs are reformatting, may not even realize what is happening in deeply nested blocks. They work on a file, make changes to several sections, save it, test it and check it in if the test worked (some don't test but that's a different problem). I think that it's a case of not reviewing every line that's being checked in, otherwise they would see the problem.

 

[Grep]

The code doesn't get auto-formatted when it's checked into the repository. It's just a few people who have their IDEs set up to do it whenever they save a file. Most people are ordering their code in a logical manner and not grouping things in an odd, unreadable manner so that their check-ins are fine - If someone does go to 90 or even 100 characters on a line, it's usually still readable. I think that the few whose IDEs are reformatting, may not even realize what is happening in deeply nested blocks. They work on a file, make changes to several sections, save it, test it and check it in if the test worked (some don't test but that's a different problem). I think that it's a case of not reviewing every line that's being checked in, otherwise they would see the problem.

That's a pet peeve of mine, right there. Really annoying when if causes merge conflicts for no good reason, for one. As you say, people should really look at a diff between their code and the repository to see exactly what they're checking in before they do so. I remember someone who was using Visual Studio who kept changing our spaces to tabs (standard said use spaces). Tip to other devs: Don't be that person. Be very careful when you check things in. There's really no excuse for that kind of thing.

 

[harborsparrow]

The code that you posted looked easy-to-read to me--just boiler plate database access code--up until the bolded part:

<code>
if ((isOneTypeOfData ||
hasPassedChecksInThisLongMethod(firstVar,
secondVar, "Some random text", data)) && data
!= null && (checkWhatThisMethodSays() ||
checkThisIfTheFirstDidntPass(data)) && (anotherBool
&& todayIsNotTheFirstOfTheMonth)) {
}
else{
...
}
</code>

Now that huge conditional statement made me say bad words. Over time (I'm now late 50's and have been at this a very long time), I decided always to break those tests out into a long series of tests, and document each one. It has a payoff--readability for others. And besides, the compiler implementations can be tricky--certain tests may never occur if a previous test failed, and that isn't always what you want.

The question is, why is someone trying to build so much business logic into one long, run-on conditional statement? The business logic should be clear. It should jump out of the page at you. One way to keep readability, once you've indented way over, is to call a subroutine right there, and put all the business logic in it, hence jumping the indentation back over to the left side of the page for that part of the code.

I do laud the goal of keeping code to 80 characters or less so that it can be printed. Sometimes, printing out a hunk of legacy code and going at it with yellow markers and such is the only way it can be deciphered, later.

I don't know if it's helpful to you, but I'd ask people, in a code inspection scenario, to untangle the conditionals, making each test separate, and I'd expect to see comments on why each test is being dong. The boiler plate DB access code does not need commenting.

 

[Simon Bridge]

This conversation reminds me of the codeless code... this page:
http://thecodelesscode.com/case/113

 

[Borg]

This conversation reminds me of the codeless code... this page:
http://thecodelesscode.com/case/113

Nice site. Thank you for the enlightenment.