building kolMafia

capitol

capitol

New member
Hi

I'm in the process of setting up my developing envirorment in order to play around with some stuff, and i noticed that the code for net.java.dev.spellcast.utilities seems to be duplicated both under the src and the lib folders, thus making it impossible to mark both of them as sources in intellij.

Is there some reason behind this, that i don't understand?

best regards
Alexander Kjäll
 
capitol said:
Is there some reason behind this, that i don't understand?
Click to expand...
Yes. The reason is that the source tree you have created is not *the* source tree. I have screwed up things like that by mapping my SVN client inappropriately.
 
Yes, it was my checkout that had gotten corrupted somehow.

The issue resolved itself when i deleted the checkout and instead made a clone of the repo over at github: https://github.com/mkszuba/kolmafia

I have a vision of improving the user interface of the chat a bit, and i'll start working on that soon, but right now i'm just reading through all code, and i noticed a number of minor code improvements and performance gains that could be made, so i started commiting fixes for those to my own fork: https://github.com/alexanderkjall/kolmafia

It's mostly about using the function contains() instead of indexOf() != -1, and replacing StringBuffer with the faster StringBuilder.

I can create a patch if you feel that the changes are good, or you can just merge my changesets if you want to. It would also be great to know if you're not interessted in those kind of patches.
 
Performance patch good, fracturing code base bad.

Also, can you actually verify that performance is improved?

Here's how Java implements the contains function:
PHP: 
public boolean contains(CharSequence s) {
    return indexOf(s.toString()) > -1;
}
 
Last edited:
Yeah, I totally agree on the fracturing of the codebase, I want to submit patches and get them accepted, that i choose git was just so that i could make local commits before I submit them as patches, it really helps when you don't have commit access yourself.

Regarding speed, that only applied to the part of the statement about StringBuffer vs. StringBuilder, to quote the javadoc: "This class is designed for use as a drop-in replacement for StringBuffer in places where the string buffer was being used by a single thread (as is generally the case). Where possible, it is recommended that this class be used in preference to StringBuffer as it will be faster under most implementations."

The reason i would like to replace str.indexOf("second str") != -1 with str.contains("second str") is that it's a bit clearer and makes it faster for new persons to read the if statements.
 
The reason those weren't done before is that Java 1.4 compatibility was being maintained, and those were added in 1.5. In general, I expect things like that will get changed as people come across that code while making other changes.
 
capitol said:
The reason i would like to replace str.indexOf("second str") != -1 with str.contains("second str") is that it's a bit clearer and makes it faster for new persons to read the if statements.
Click to expand...

I definitely agree with you on this one. The performance difference will be infinitesimal and the code will be much easier to read.

Also, regarding your StringBuilder, what lostcalpolydude said.

My understanding was that StringBuilders worked out faster when you are looping through code to build a string, but I guess there's no harm in moving over to the recommended implementation :)
 
Not that I know much about tiny performance improvements, but I assumed the compiler would turn indexOf("foo") != -1 and contains("foo") into the same thing.
 
capitol said:
using the function contains() instead of indexOf() != -1
Click to expand...
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.
 
Here is a patch with some changes:

http://frikod.se/~capitol/StringBufferAndVariableBoxingCleanup.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/kolmafia/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/java/library/j-jtp04223/index.html#N100A7 and http://www.ibm.com/developerworks/java/library/j-jtp1029/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/kolmafia/commit/74fd30dbd8800b80eb7009d8715f68242cd20f63
https://github.com/alexanderkjall/kolmafia/commit/4f480a1df3276d6a2d6d88821c9f9799e90ea631
https://github.com/alexanderkjall/kolmafia/commit/37699c5d7253f27198046263767afa74e3003991
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?

PHP: 
-    if ( this.formSource.indexOf( "adventure.php" ) == -1 )
+    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.
 
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.
 
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.
 
You must log in or register to reply here.
Back
Top