Alex, This is a great letter! I cannot compete. I agree with the main point that no change should be done until the proper understanding of consequences is achieved.
Nevertheless, from my perspective automatic tools are pretty good for locating problematic places, and each place usually worth deletion or simplification. I agree that if a part of the code is kept for purpose, automatic tools can destroy it. From the other side, if we check your example, the following commenting solves the problem. >// int i; >// i=someMethod() TODO Fix the bug in someMethod() I have even a stronger suggestion - we should remove the commented code leaving only a message for human beings. I was convinced once that it is always better to remove all dead and commented code. There are many guys who will spend time traversing dead code, while usually only one guy benefits from keeping it. His tangible benefit is measured in saved efforts from retrieving a previous revision from a version control system. I suggest that we better save the time for many readers instead of the time of one lazy writer. I'm ok with leaving a human readable message in a place of a bug or exceptional behavior. For example, in our case I would write: >// TODO someMethod() behaves weirdly in case of OutOfMyResourceError. >// Fix the behavior. The example about conditional statements is even better. I remember that reordering "if" statements sometimes decreased my code size to quarter. Here goes the rule number two. If an automatic tool corrupted a part of my code, I would either reconfigure the tool, or stop using it for this part of the code. With best regards, Alexei Fedotov, Intel Java & XML Engineering >-----Original Message----- >From: Alex Blewitt [mailto:[EMAIL PROTECTED] >Sent: Saturday, October 21, 2006 1:59 AM >To: harmony-dev@incubator.apache.org >Subject: [OT] Automated fixes considered harmful > >There have been a few messages recently regarding build warnings that >complain about e.g. local variables not being used or missing >annotations, and work going on to 'fix' them. > >However, in most cases, the only thing that is being 'fixed' is that >the compiler warnings no longer appear. Realistically, it's not a >problem in the code; in fact, the code works fine with those warnings >in place. Arguably, therefore, the code is being fixed to suit the >compiler, rather than the compiler being configured to be more useful >(i.e. less verbose). > >Of particular beef are the kind of automated clean-ups that IDEs such >as Eclipse will offer. These are mostly incredibly dangerous, and >offer no significant benefit to Harmony as a whole. Some of them are >benign (for example, updating the list of imports) and some of them >aren't (deleting private methods, unused variables etc.). The problem >is that unless you've got a good idea of the code at any one point, >performing automatic transformation of the codebase is not a sensible >plan. > >Whilst the fact of the matter is true that it doesn't affect code >execution (because private methods *aren't* being called, or a local >variable *isn't* being read), it doesn't make sense to do it >necessarily. For example, one could argue that removing all the >JavaDoc from the source files has no effect on the code, and has the >added advantage that the size of the source code is smaller -- but >that doesn't make it a sensible thing to do. > >In particular, there's a lot of areas in the code that are pretty >dynamic, and are still evolving. For example, I could be writing >something like: > >int i; >i=someMethod(); > >and then want to commit it to the repository. Let's say I discover a >bug in someMethod(), so I instead do: > >int i; >//i=someMethod() //TODO Fix the bug in someMethod() > >Now, automated tools will complain that i is being unreferenced >(possibly even uninitalised too :-) and volunteer to take it out. The >problem is then, after I've committed and worked on my code, I have: > >int i; >i=someMethod() // Fixed bug > >and someone else has committed Auto-Delete, so that when I update it >becomes: > >i=someMethod() // Compiler error, i doesn't exist > >The problem is that the person who ran Auto-Delete only saw a snapshot >of the source code. Yes, *at that point in time* the variable wasn't >being defined, but by removing it, you've effectively removed it from >versions in the future, too. It's even worse with a private method, >because then you've lost a lot more than a simple variable >declaration. > >The same thing is true of the Auto-Generics that some people have >become fond of. Really, get over it: generics don't save anything that >you wouldn't have in code anyway (they're pretty much all compile-time >restrictions anyway; there's still no type safety at run-time like >there is in C#). Worse, Generics can only be used with Java 5 onwards, >so by introducing generics into a library you're preventing that same >library being used on anything older than Java 5. Granted, most of >Harmony is working towards the Java 5 goal, but I'm sure there will be >reusable parts of the codebase that other users would otherwise be >able to use on older VMs, such as some of the tools (keytool, >jarsigner) or of course, the pack200 libraries that I'm involved with >at present. > >I've also no idea what IDE suggested this one, but in the following: > >if(condition) { > // some code > return value; >} else { > // some other code > return otherValue; >} > >is absolutely *not* the same as > >if (condition) { > // some code > return value; >} >// some other code >return otherValue; > >These two may execute the same, but they have very different >semantics. In the first example, it's impossible to go from the if >block to the else block in the future. However, in the second example, >it may be that you can wrap some extra conditions around the return >statement. In this case, they have different semantics: > >if (condition) { > // some code > if (anotherCondition) > return value; >} else { > // some other code > return otherValue; >} > >and > >if (condition) { > // some code > if (anotherCondition) > return value; >} >// some other code >return otherValue > >The point is that code has been written in one way (or other) for a >reason. The code may well evolve in the future (or be evolving as you >are currently applying automated fixes). If you know that the variable >is not only unused now, but always will be unused in the future, then >fine, remove it. However, if all you know is the variable is unused >now, then automated fixing can be detrimental to the health of the >codebase. > >Alex. > >--------------------------------------------------------------------- >Terms of use : http://incubator.apache.org/harmony/mailing.html >To unsubscribe, e-mail: [EMAIL PROTECTED] >For additional commands, e-mail: [EMAIL PROTECTED] --------------------------------------------------------------------- Terms of use : http://incubator.apache.org/harmony/mailing.html To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
