Page 2 of 2 FirstFirst 1 2
Results 11 to 19 of 19

Thread: building kolMafia

  1. #11
    Developer Veracity's Avatar
    Join Date
    Mar 2006
    Location
    The Unseelie Court
    Posts
    11,389

    Default

    using the function contains() instead of indexOf() != -1
    Originally Posted by capitol View Post
    I'm fairly certain that this is not a performance improvement; the caller compares the return value to "true" rather than to "-1".

    I guess it's more readable, if you have never looked at a Java program before.
    Ph'nglui mglw'nafh Cthulhu
    R'lyeh wgah-nagl fhtagn.

  2. #12
    Junior Member
    Join Date
    Jan 2010
    Posts
    22

    Default

    Here is a patch with some changes:

    http://frikod.se/~capitol/StringBuff...gCleanup.patch

    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
    https://github.com/alexanderkjall/ko...5f68242cd20f63
    https://github.com/alexanderkjall/ko...9f9799e90ea631
    https://github.com/alexanderkjall/ko...7afa74e3003991
    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

  3. #13

    Default

    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?

    PHP Code:
    -    if ( this.formSource.indexOf"adventure.php" ) == -)
    +    if (!
    this.formSource.contains("adventure.php")) 
    Ignoring the indexOf => contains change, you removed the spaces between ( and ", changing the code style used.

    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.

  4. #14
    Junior Member
    Join Date
    Jan 2010
    Posts
    22

    Default

    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

  5. #15
    Developer fronobulax's Avatar
    Join Date
    Feb 2009
    Location
    Central Virginia, USA
    Posts
    4,126

    Default

    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.
    You just vehemently agreed with me
    Originally Posted by Veracity View Post
    I agree with frono.
    Originally Posted by Veracity View Post

  6. #16
    Developer Veracity's Avatar
    Join Date
    Mar 2006
    Location
    The Unseelie Court
    Posts
    11,389

    Default

    I agree with frono. I do not want wildcard imports.
    Ph'nglui mglw'nafh Cthulhu
    R'lyeh wgah-nagl fhtagn.

  7. #17
    Developer roippi's Avatar
    Join Date
    Aug 2010
    Posts
    2,663

    Default

    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.

  8. #18
    Junior Member
    Join Date
    Jan 2010
    Posts
    22

    Default

    ok, i'll strip out all changes to imports

  9. #19

Similar Threads

  1. Building from the SVN?
    By bluestars in forum Unofficial Builds
    Replies: 130
    Last Post: 06-13-2016, 01:48 AM
  2. 10605: Provide GUI for building Simple deeds.
    By RSS Bot in forum Latest SVN Changes
    Replies: 0
    Last Post: 02-24-2012, 05:50 PM
  3. Bug Macro building doesn't check for Funkslinging
    By Yvain in forum Bug Reports
    Replies: 9
    Last Post: 09-15-2010, 09:29 AM
  4. Need help building/making script work.
    By hippymon in forum Scripting Discussion
    Replies: 3
    Last Post: 06-12-2007, 01:46 AM
  5. MOVED: Building from the SVN?
    By efilnikufecin in forum Community Support
    Replies: 0
    Last Post: 01-20-2007, 10:26 AM

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •