Here is a patch with some changes:
It's a bit to large to be really usable in my opinion, but there is also a commit history here: https://github.com/alexanderkjall/ko...commits/master
I split the commits to one commit per file, in hindsight that might not have been the best way to do it, if i would make something like this again i think it would have been better to group the commits based on the type of change that was done.
The changes can be broken down into the following categories:
*) StringBuffer changed to StringBuilder, and calls to append chained instead of doing string concatination:
Discussed above, strictly due to performance reasons.
*) use .contains() instead of .indexOf()
Also discussed above, done because of readability.
*) removed the final qualifier on private methods
This is also a readability change, as private methods can't be overridden the final keyword doesn't have any effect. People sometimes say that it affects performance, but according to these sources: http://www.ibm.com/developerworks/ja...ex.html#N100A7 and http://www.ibm.com/developerworks/ja...029/index.html .
*) removed variable boxing and unboxing
There is some calls to .intValue() on Integers and wraping of int with new Integer(someInt), these are redundant as java handles these conversions transparently.
*) strings compared with "" instead of using equals
These might indicate bugs, but most likely not, but it's a bif of an unsafe coding practice to compare strings using the != operator.
*) restructured imports
These was done by my editor and i didn't notice at first, so it became a bit hard to revert. Feel free to ignore and not import (but it removed about 4k lines of imports :P )
*) Added generics to avoid some casts
Here are these changesets
This avoids some casts and makes the code a bit easier to read in my opinion.
I'll start trying to make some improvements to the chat that i have been thinking about, if i manage to improve something i'll submit another patch
return as last statement in a function returning void
Those are all over the place. I'm pretty sure they exist so that if another block is added, it won't be executed when it shouldn't be. Does it actually hurt anything?
Ignoring the indexOf => contains change, you removed the spaces between ( and ", changing the code style used.PHP Code:
- if ( this.formSource.indexOf( "adventure.php" ) == -1 )
+ if (!this.formSource.contains("adventure.php"))
This looks like it just removed some blank lines (which were there on purpose) and replaced a single import with a wildcard import. There are occasional unused imports (which I mostly fix when I happen to be editing the file anyway), but I have a feeling most of the 4k removed lines are in this category of blank lines that should not be removed. I see other cases like this where a large number of imports are collapsed by a wildcard import. I don't know if there's a reason for doing it one way versus the other.
This is another case where continue isn't needed now, but removing it is likely to cause issues if something else is added there later.
Good point about the style change, that's handled by my editor, but I'll make sure to fix it so that I don't break style.
I also agree on the other points, didn't really think about the usecase when another block is added.
Regarding the imports, yes, it's mostly just reordering and replacing code blocks with * and stuff like that, I didn't intend it to happen really and let me see if I can strip it out from the patch somehow.
Will maybe take some time to make the changes
Personally, I find wild card imports decrease readability and comprehension. Explicit imports help me focus on what is being brought in and used as opposed to what might be brought in. So I am all in favor of alphabetizing import lists and eliminating unused ones and not in favor of rolling things up into wild cards. I suppose I should check and see whether the defacto code standards support my preference or not ;-)
As someone who has broken applications by overzealous following of recommendations from lint like programs and compiler warnings, I am a bit leery of such changes and would thus encourage you to make such changes in small chunks or when they occur in code you are already visiting for some other purpose.
I agree with frono. I do not want wildcard imports.
Ph'nglui mglw'nafh Cthulhu
R'lyeh wgah-nagl fhtagn.
Another +1 on no wildcards. The only time I'd ever use them is (in 1.5+) doing a static import on a library class.
ok, i'll strip out all changes to imports