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)

  • what kind of warnings? –  Jul 21 '11 at 07:17
  • 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. –  Jul 21 '11 at 07:47
  • are these *compiler* warnings? –  Jul 21 '11 at 08:52
  • what would you rather call them? –  Jul 21 '11 at 09:40
  • 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 '11 at 09: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 '11 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. – JSBձոգչ Jul 21 '11 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 :/ – Matthieu M. Jul 21 '11 at 17:25
  • Consider asking "Which warning can _ALWAYS_ be safely ignored, and why?" –  May 16 '12 at 12:05
  • What I don't get is why eclipse wants to tell me anything but *finished* and warnings/errors. How do people read through this junk in builds that take 40+ seconds? Scrolling/scanning for blue linky thingies seems kind of silly to me. – Erik Reppen Mar 05 '13 at 14:45
  • I used to obsess over clean code for my small pet projects and school assignments. But when I got a job, I was faced with large projects with thousands of warnings. I realized that a company has more important things to do, and it doesn't have the time or resources to dedicate to "cleaning up the code". Most of the warnings are due to upgrading Java versions (once clean code suddenly becomes laden with warnings, especially with the introduction of Java generics). I write clean code when possible, but I learnt to ignore the 13.5k warnings already there. **Fix the errors, ignore the warnings.** – ADTC Feb 24 '14 at 06:58
  • My favourite solution is: either treat a warning as an error, or switch it off. I do not want to be warned about things that I do not consider important, and if I consider a problem important enough that it should cause a warning, then I have to fix it. – Giorgio Nov 29 '14 at 09:38
  • Possible duplicate of [Is it good practice to avoid warnings and notices?](http://programmers.stackexchange.com/questions/307966/is-it-good-practice-to-avoid-warnings-and-notices) – gnat Jun 23 '16 at 11:00
  • i don't think this is useful to do - you're not a "coder as an aritst" you're a "coder as a person executing business desires". You're solving the wrong problem. The real problem is to increase corporate revenue. – bharal Apr 13 '19 at 19:56

9 Answers9

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?

Basic
  • 234
  • 2
  • 10
Ray Toal
  • 1,305
  • 8
  • 11
  • 9
    I'd like to add that you can't make other people care, so be pragmatic and pick your battles well. – Daniel Werner Jul 21 '11 at 06: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. – Joachim Sauer Jul 21 '11 at 06: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 '11 at 06: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. –  Jul 21 '11 at 06: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 '11 at 18:02
  • It just so happens that I recently came across this set of "Java Puzzlers": www.youtube.com/watch?v=wbp-3BJWsU8 (tl;dw, the slides are at goo.gl/FzEjQ). One of the main morals of this whole presentation is just what you are saying: Listen to what the compiler tells you, because compiler warnings often correspond to runtime errors. – Tyler Jul 23 '11 at 03:47
  • I would add: "Sure is nice to rule out all that crap when you have some other confusing problem." – Erik Reppen Mar 05 '13 at 14:44
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 :)

  • 2
    very-well thought through response. Thank you. The example you give will be an error rather than a warning in Java though. :) –  Jul 21 '11 at 07: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! –  Jul 21 '11 at 08: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 '11 at 03:51
  • 1
    @MatrixFrog: Undefined. Can be an arbitrary int, which will cause all sorts of nasties. Quote from [this](http://stackoverflow.com/questions/4090105/a-function-declared-to-return-int-returns-nothing-is-this-undefined-behavior/4090171#4090171) 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.*" –  Jul 26 '11 at 00: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.

Vineet Reynolds
  • 653
  • 4
  • 9
  • 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... –  Jul 21 '11 at 06: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
    Or focus on fixing warnings when you look at code (new classes should have no warnings, any classes you modify should have less warnings). – Brendan Long Jul 21 '11 at 16:23
  • Serious question: How do you know which ones are most likely to result in actual bugs? – Tyler Jul 23 '11 at 03:53
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.

Thorbjørn Ravn Andersen
  • 1,297
  • 10
  • 14
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.

Kane
  • 111
  • 3
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!

WinW
  • 1,003
  • 1
  • 8
  • 19
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 ;)

Mehul Lalan
  • 9
  • 1
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.

Ben Taitelbaum
  • 111
  • 2