51

I work on a huge project (more like a tangled up combination of dozens of mini projects that can't be easily separated due to poor dependency management, but that's a different discussion) in Java using eclipse. We've already turned off a number of warnings from compiler settings and the project still has over 10,000 warnings.

I'm a big proponent for trying to address all warnings, fix all of them if possible, and for those that are looked into and deemed safe, suppress them. (Same goes for my religious obsession with marking all implemented/overriden method as @Override). My biggest argument is that generally warnings help you find potential bugs during compile time. Maybe out 99 of 100 times, the warnings are insignificant, but I think the head scratching that it saves for the one time that it prevents a major bug, it's all worth it. (My other reason is my apparent OCD with code cleanness).

However, a lot of my teammates don't seem to care. I occasionally fix warnings when I stumble across them (but you know it's tricky when you touch code written by a co-worker). Now with literally more warnings than classes, the advantages of warnings are very much minimized, because when warnings are so common-place, nobody will bother looking into all of them.

How can I convince my teammates (or the powers that be) that warnings need to be addressed (or suppressed when fully investigated)? Or should I convince myself that I'm crazy?

Thanks

(P.S. I forgot to mention what finally prompted me to post this question is that I sadly noticed that I'm fixing warnings slower than they are produced)

14
  • 5
    All kinds: rawtype, unused import, unused variable, unused private methods, unnecessary supress, unchecked, unnecessary cast, unnecessary condition (always be true or false), unannotated overriden methods, reference to deprecated class/methods etc.
    – RAY
    Jul 21, 2011 at 7:47
  • 1
    « I want to start applying static “code” analysis to game maps, and treating warnings as errors. » Recent quote fro John Carmack. If you are crazy, you are to of you. Actually, we are three of us.
    – deadalnix
    Jul 21, 2011 at 9:55
  • 1
    @RAY: These are warnings from Eclipse's code analysis, I'm not sure if you could get all of them from vanilla javac.
    – yatima2975
    Jul 21, 2011 at 10:45
  • 4
    but you know it's tricky when you touch code written by a co-worker -- if your workplace has these kinds of territoriality issues, I consider that a big red flag. Jul 21, 2011 at 15:19
  • 1
    @deadalnix: being a C++ guy, I only have one way to do things -Wall -Wextra -Werror (ie, activate most of the warnings available, treat them all as errors). Eclipse C++ is nigh unusable though :/ Jul 21, 2011 at 17:25

9 Answers 9

37

You can do two things.

  1. Point out that warnings are there for a reason. Compiler writers don't put them in because they are mean-spirited. People in our industry are generally helpful. Many warnings are helpful.

  2. Gather up a history of spectacular failures arising from ignored warnings. A web search for "Pay attention to compiler warnings" returns some anecdotes.

Your obsession with @Override is not an "obsession." That is a Good Thing. Ever misspelled a method name?

7
  • 9
    I'd like to add that you can't make other people care, so be pragmatic and pick your battles well. Jul 21, 2011 at 6:37
  • @Daniel: sad, but true. If they don't care, even if they know (or at least believe) that you're right, then this is not a task with a rosy outlook. Jul 21, 2011 at 6:40
  • 2
    There are plenty of times when warnings are in fact just warnings. You can be vigilant, but I'd often prefer to spend that time writing test-cases that are much more specific to your codebase.
    – hayesgm
    Jul 21, 2011 at 6:46
  • I think the OP is looking to get coworkers to stop being dismissive. Granted many warnings do not have to be addressed, nor should they all be! But being blase and allowing 10,000 of them to creep into the codebase is not a good thing. Warnings should be looked at, considered, and if not applicable, they can be turned off or a Suppress annotation applied.
    – Ray Toal
    Jul 21, 2011 at 6:53
  • 3
    I recently tidied up some warnings in an open source project and found a clear bug that was reported via a warning: if (error = 0) instead of if (error == 0). Also, lots of warnings also makes it much easier to find compiler errors without having to wade through reams of warnings.
    – Hugo
    Jul 21, 2011 at 18:02
12

Relevant reading here. C++, but still relevant. I especially like this example (code comment is mine):

int searchArray(int to_search[], int len, int to_find)
{
    int i;
    for( i = 0; i < len; ++i )
    {       
        if ( to_search[i] == to_find )
        {
            return i;
        }
    }
    // should be returning designated 'not found' value (0, -1, etc)
}

A lot of the time, a warning will mean the difference between the application crashing with an error which will actually help you track down a design flaw and fix it (such as safe typecasting) or the application making assumptions and then actually behaving in an incorrect or erroneous manner (which would be the case with unsafe typecasting). Similarly, they also point out cruft or dead code which can be removed (unreachable code blocks, unused variables), which can be considered code optimisation, or unaccounted-for cases which will show up in testing, like the above example for C++. Note that this example results in a compile-time error in Java.

So, fixing warnings has (at least) several advantages for management:

  1. Less chance of the code behaving improperly with user-inputted data which, to the higher-ups concerned about the image of the company and how it handles its clients' data, should be a Big Deal(tm). Crashing to prevent a user from doing something silly is better than not crashing and letting them do it.
  2. If they want to reshuffle personnel, people can come into a warning-free codebase (and not freak out or complain as much..). Maybe this will reduce the amount of grumbling that goes on, or opinions on the maintainability or quality of Team X's contribution to Project Y.
  3. Optimising code by removing compiler warnings, while not giving any significant improvements to running time, is going to improve numerous scores when it comes to testing:
    • Any code coverage scores will likely increase.
    • Testing boundary and invalid test cases will likely succeed more often (due to the abovementioned safe typecasting, among other things).
    • When a test case does fail, you don't have to think about whether it's due to one of the compiler warnings in that source file.
    • It's easily arguable that zero compiler warnings is better than thousands. No warnings or errors speaks about the quality of code, so you can argue that code quality improves the less warnings there are.
  4. If you have no compiler warnings, and one or more are introduced, it's a lot easier to know who caused those to appear in the codebase. If you have a "no warnings" policy, that would help them identify people who aren't doing their job properly, perhaps? Writing code isn't just about making something work, it's about making it work well.

Note that I'm still just a junior dev who knows little about project management, so if I've said something wrong please correct my thinking and give me a chance to edit before you downvote me out of existence :)

4
  • 2
    very-well thought through response. Thank you. The example you give will be an error rather than a warning in Java though. :)
    – RAY
    Jul 21, 2011 at 7:44
  • @RAY: Ah, fair enough. It's been a year since I've done any Java dev, so I was bound to overlook something. I'll edit my post to mention that. Thanks!
    – user31713
    Jul 21, 2011 at 8:32
  • It's been a while since I did C++. What does that function return if you reach the comment at the end? Is it 0? Undefined behavior?
    – Tyler
    Jul 23, 2011 at 3:51
  • 1
    @MatrixFrog: Undefined. Can be an arbitrary int, which will cause all sorts of nasties. Quote from this answer: "C++03 §6.6.3/2: Flowing off the end of a function is equivalent to a return with no value; this results in undefined behavior in a value-returning function."
    – user31713
    Jul 26, 2011 at 0:56
7

I've worked on a project with similar characteristics in the past. This one in particular was originally written in Java 1.4. Once Java 5 with generics came out, one can imagine the number of warnings thrown by the compiler for every usage of the Collections API.

It would have taken quite sometime to get rid of all the warnings, by beginning to use generics. That is a factor that must be accounted for, but when you have to convince someone (especially your manager) that it needs fixing, you will need hard data, like

the time spent on bugs in legacy or non-standard compliant code (the standard is your project's coding standard) that could have been avoided.

You could keep presenting a bill that demonstrates how you are loosing time and money by ignoring "certain" warnings, and someone will get the idea. The key part is that not all warnings are worth looking at, at least not right away; in simpler words, you will need to establish the priority of the warnings that need addressing immediately. To begin with, consider the ones pertinent to bugs that you are seeing in your project.

You could also make notes in the bug tracker, when you fix bugs, that the said bug could have been avoided by not ignoring a warning (or perhaps by running PMD or FindBugs if you have a CI or build system). As long as there are a sufficient number of bugs that can be fixed by heeding compiler warnings, your point about looking at compiler warnings would be valid. Otherwise, it is a business decision, and usually it is not worth the time spending to fight this battle.

1
  • Yes, exactly. I think the initial explosion of the number of exceptions happened during the 1.4->1.5 transition, and since then, nobody paid any attention to warnings anymore because there are just too many...
    – RAY
    Jul 21, 2011 at 6:41
2

If you succeed and your team decides to observe the warnings, you should take a step-wise approach. No one can and will 10000 warnings in one time. So you can select the most important ones (those that are bugs with high probability) and disable the less significant ones. If they are fixed, increase the warning level again. Additionally, you can start with FindBugs, which warns on code that is almost always a bug.

2
  • 2
    Or focus on fixing warnings when you look at code (new classes should have no warnings, any classes you modify should have less warnings). Jul 21, 2011 at 16:23
  • Serious question: How do you know which ones are most likely to result in actual bugs?
    – Tyler
    Jul 23, 2011 at 3:53
1

I totally agree with you, it's a best practice to clean up the compile warnings as more as possible. You mentioned your team is using Eclipse as development tool. Eclipse is a very good tool to help you clean up the code and make the code style consistence.

  1. define 'Save action' for your each Java project, such as formatting code, unused local variables, organizing the imports(I think nobody has objections on such kind of cleaning up).
  2. define project specific compile options for each project. For example, compile level(if your code need be compatible with 1.4 to clean the warnings related to generic), assignment has no effect(e.g. 'x=x') and so on. You and your teammate can make an agreement for those items, and Eclipse will report them as 'Error' in your development phase after you have agreement for the compile error.

Eclipse will create some properties files for those preference under .settings folder, you can copy them to other Java projects and check them in to SCM as part of source code.

You can check out the code of Eclipse to see how Eclipse developers do it.

1

You have two ways of getting rid of all the warnings (and I agree it can hide a subtle bug):

  1. Convince the whole team that this is the best thing to do, and have them fix it.
  2. Convince your boss that this is the best thing to do, and make it mandatory. Then they need to fix it.

I believe that 1 is not feasible in your case any more. Also this will probably take so long due to the number of warnings that this will show in the productivity output so the management will need to know anyway.

So, my suggestion is: Take it up with the boss, convince him this is a ticking bomb, and have it made official policy.

1

I would just say them that most of these are insignificant warnings and it is essential that we remove them from the list. So that we will not miss the real significant warning in the crowd, as it happens!

1
  • Exactly. It's important to get rid of them because they're insignificant.
    – Tyler
    Jul 23, 2011 at 4:55
0

You will need lots of patience in trying to convince them, removing warnings when doing pair-programming will help others pick up the habit. Suggest them to read Effective Java 'Item 24 : Eliminate unchecked warnings' (for generic collection). And probably ignore many instances when people do not follow your advice ;)

0

An effective approach here would be to setup a Continuous Integration server that automatically builds the project and runs the tests whenever anyone checks in code. If you're not already using this, you should be, and it's not very hard to convince others of the benefits of doing so.

  1. Convince management / team leads of the benefits of doing this (preventing bad builds from being deployed, finding bugs early, especially those that accidentally affect other parts of the software, maintain regression testing, testing early and often, etc.)

  2. Install CI server with all tests passing (if you don't have tests, write a quick one that passes, so that everyone sees that it's green).

  3. Setup emails or other notifications about the status of each build. It's important here to include who checked in the code, what the change was (or a link to the commit), and the status+output of the build. This is an important step, as it causes team-wide visibility both for successes and for failures.

  4. Update the CI server to make tests fail if there are any warnings. This will be seen by management and team leads as increasing the discipline of the team in a systematic way, and no one wants to be responsible for all the failure emails going out.

  5. (optional) get nice and geeky with this, by adding some visible dashboards, lava lamps, flashing lights, etc. to indicate the status of the build and who broke it / fixed it.