Medo42/LateralGM — comment on commit 8cd7280437a6d381be2ad9dbabb6a29edf5ab3fa
File:  line 0
By Medo42:

Please note that several changes were only done for my personal benefit and while looking through warnings generated by FindBugs, since I did not anticipate all my changes being integrated into LGM anyway, so feel free to revert anything you don't like. However, since we are already discussing this, let explain what I think about these issues.

First, I understand that it is easy to miss strings which are not localized yet. However, I'm not sure if having all these ignore hints in the code and and having to put up with useless warnings (to the point of changing perfectly sensible code to get rid of them) seems quite annoying. I don't know a good solution that avoids both issues, but maybe there is a tool that only reports non-i18n strings in new code, so that old code does not get warnings and does not need ignore comments?

About the dead code, this is very much a matter of opinion and I don't want to force mine on your development process, I just want to explain it. I am a believer in keeping changes atomic and not keeping half-finished things in the main codebase. That means dead or commented-out code in the repository is useless and can be thrown away. There can be exceptions of course, but they should be marked with a TODO and a description of what is supposed to happen there (bonus points if an issue or change request id is mentioned). Otherwise, incomplete development should happen in a feature branch. The problem is that half-finished things tend to be forgotten and some people don't want to remove them even after they become obsolete because they think it still might be useful to someone, and everyone reading the code later will attempt to figure out what this was about, wasting effort.

Overriding the equals with super.equals(): I wasn't sure myself whether to do this or not. The incentive came from FindBugs, and on the one hand the argument isn't very strong, but on the other the code doesn't hurt anything. Here's the point: Since the superclass has implemented equals(), PropertyMap does not use object identity, but instead relies on the equals definition of the superclass. However, since PropertyMap adds extra fields, it's not clear to a reader whether these are not included in the equals test by design or whether the programmer just didn't realize that the class doesn't use object identity. By adding an explicit equals method, it becomes clear that the extra fields are not compared on purpose.