FindBugs and Code Cleanup

[sentient6]

Yesterday, I ran FindBugs against a portion of the code base, and not surprisingly for a project this large, quite a number of errors and bugs came up. I was wondering if any would mind if I went in and cleaned up some of the more mundane and obvious amongst them in the name of code cleanliness, bug prevention, and the slight possibility of a performance boost. I wouldn't mess with anything that would affect the behavior of the code, just a couple of bugs and implementation details such as accidentally comparing two different types with .equals() and the like.

On a separate note, sorry for popping up every 5 seconds to ask permission to do something.

[DennisBergkamp]

Sure, go ahead
We've used FindBugs a bit already on Forge in the past, but we mostly only really fixed the most blatant and performance degrading ones.

[sentient6]

Ah got it. It does produce a whole mess load of things to consider, but I find a certain Zen appeal in cleaning up the mundane details of code.

[sentient6]

While cleaning some stuff up, I found the following in ComputerUtil_Block2.getBlockers :

Code: Select all

for(int j = 0; j < blockers.size(); j++) {
    if (CombatUtil.canBlock(attacker, blocker, combat)) {
         blocker = blockers.get(j);
         combat.addBlocker(attacker, blocker);
         blockersLeft.remove(blocker);
   }
}


I'm assuming that it should instead have been something like:

Code: Select all

for(int j = 0; j < blockers.size(); j++) {
    blocker = blockers.get(j);
    if (CombatUtil.canBlock(attacker, blocker, combat)) {
         combat.addBlocker(attacker, blocker);
         blockersLeft.remove(blocker);
   }
}


Otherwise, blocker could be any number of seemingly arbitrary cards when the loop is first entered, depending on the previous path through the method. I just wanted to make sure that this behavior wasn't intended so that may changes will not break anything.

[sentient6]

Further digging has come up with the following (non-exhaustive) list of classes that seem to not do much and don't seem to be used by anything. Is there any reason to not delete them from the current code? As I see it, they'll still be available in the old revisions and they're just cluttering up the namespace.

forge.OldDeckIO (not the one in the default package)
forge.Gui_SealedDeck
forge.Gui_SetEditor
forge.Gui_Welcome
forge.IO
forge.KeyListenerTest
forge.ListProperties
forge.Move
forge.Test
forge.TestMove
forge.TestPanel
forge.Time
forge.Wait

[Chris H.]

Thank you for the help. It is nice to have another set of hands on the project. We also have a few data files that are no longer used. It might be best to wait until after the next beta before we remove too much of the old cruft.

It is better to be safe than sorry.

[sentient6]

Thanks. My main focus so far has been trying to make sure I don't accidentally break anything while trying to help.

Well at least I have those classes written down to look at in the future. I checked an they aren't referenced by anything.

[silly freak]

hi! the legacy classes in the default package are theoretically necessary if anyone with a like 2 years old version wants to migrate his decks to the new version. I don't know why there's an OldDeckIO class in the forge package, but it must be me who created it. I think you can delete it safely

[Rob Cashwalker]

Could you post whatever report it comes up with so we can get an idea of the sort of problems need to be addressed?

[sentient6]

No problem. Below is a summary of a run of FindBugs against r5107.

FindBugs Summary | Open

• Check for oddness that won't work for negative numbers (2)
• Class defines equals() and uses Object.hashCode() (6)
• Class doesn't override equals in superclass (2)
• Class inherits equals() and uses Object.hashCode() (31)
• Class is Serializable but its superclass doesn't define a void constructor (5)
• Dead store to local variable (2)
• Exception is caught when Exception is not thrown (5)
• Field names should start with a lower case letter (25)
• Method concatenates strings using + in a loop (9)
• Method ignores exceptional return value (24)
• Method may fail to close stream (9)
• Method might ignore exception (1)
• Method uses the same code for two branches (1)
• Non-serializable class has a serializable inner class (1)
• Non-serializable value stored into instance field of a serializable class (10)
• Non-transient non-serializable instance field in serializable class (1028)
• Non-virtual method call passes null for nonnull parameter (2)
• Nullcheck of value previously dereferenced (1)
• Possible null pointer dereference (1)
• Read of unwritten field (3)
• Should be a static inner class (11)
• Unchecked/unconfirmed cast (5)
• Unread field (1)
• Unused field (2)
• Unwritten field (2)
• Write to static field from instance method (21)
• integral division result cast to double or float (5)
• ==============================================================================
• Overall bugs count: 1215

Most aren't really a big deal and come down to style or good practices. The majority have to do with a bunch of classes claiming to be Serializable while not really being set up to be so. Hope the above is at least mildly useful.

[DennisBergkamp]

hi! the legacy classes in the default package are theoretically necessary if anyone with a like 2 years old version wants to migrate his decks to the new version. I don't know why there's an OldDeckIO class in the forge package, but it must be me who created it. I think you can delete it safely

Yup, I actually removed it in my local copy when I started with the Shandalar stuff.

[friarsol]

Dennis, how's that work coming along?