Document logisim.data package

[cosineblast]

This PR (draft) contains style fixes for the class Value.java. It contains/will contain fixes for the incorrectly braced if-else statements, one logic change (to get rid of an empty brace warning) and javadocs for all public methods.

Small question:
While documenting the public functions, I encountered this 'controls(' function. It doesn't seem to be used anywhere in the project, and i'm not particularly sure of what it does. Is it ok to remove it?

[MarcinOrlowski]

I do not see the point of this PR. While LSe' code style is far from perfect, it is somehow consistent. The type of changes introduced in this PR add no value and can be made easily with formatting tools, not to mention there's no point of changing just single file. I'd vote to reject this one.

[cosineblast]

Thanks for the feedback. I have implemented these changes mostly to reduce the amount of style warnings coming from the checkStyleMain build task in this file, however I can implement changes to more source files if that's desired. Would changes to all logisim.data classes be welcome in a single PR?
I can also provide more valuable modifications such as adhering to final vars and enhanced switch statements/switch statement expressions as well.

@MarcinOrlowski would such improvements to the PR be welcome?

[R3dst0ne]

@MarcinOrlowski would such improvements to the PR be welcome?

Since @MarcinOrlowski introduced PRs with similar scopes (#1853, #1854, #1856, #1857), they should be welcome.

Adding documentation to the project is one of the best things you can do. Unfortunately, LSe is lacking severely in this area.

[MarcinOrlowski]

@cosineblast When we first introduced linters years ago, we opted for CheckStyle with Google's built-in coding style as a base, however, we made several exceptions to the rules. This wasn't due to quality in LSe's code only, but rather because not all rules in Google's style guide were aligned with what we wanted to enforce. It's important to note that there's no one-size-fits-all coding style, and LSe is no exception. It's also worth pointing out that we adapted our code style policy to existing code style in some portions (like brackets) because it was already widely spread across the code base. While I personally also prefer the style with brackets, there's a reason we allow bracket-less branches and I would most likely write new code the same way here for sake of consistency. Our project's code style is evolving as we make incremental improvements through refactoring and any attempts to enhance code quality are welcome, but the reason I am objecting here is that this PR introduces changes without necessarily improving anything. Adding brackets is a trivial operation and hasn't been done for years, because the current code style is simply consistent. Will that be changed in future? Maybe, but I dare to say the "evolution" over "revolution" is better choice, and since we use no preprocessor nor anything that would unfold one-liner into bigger multi-instruction code block ruining the logic, adding brackets here only bloats the code without any tangible benefit:

The PRs that @R3dst0ne linked are not directly relevant here. What he failed to notice is that those PRs aim to reduce verbosity and cut down lines of code, not the opposite.

If you have some spare cycles to contribute to the LSe project, you're more than welcome. However, more beneficial refactoring tasks would include using final var, replacing meaningless single-letter variable names with self-documenting ones, adding properly constructed JavaDoc blocks (please consult this guide first), reviewing or updating translations if you're fluent in any of the supported languages, or possibly addressing some bugs. Adding brackets should rather be at the very bottom of that priority list, if included at all.

[cosineblast]

This makes sense. I'll be updating the PR in the following days to provide more meaningful changes (i.e documentation and final vars) to this and other classes in the logisim.data package. Although I don't mind having this one as closed and opening another one when these improvements arrive.

[maehne]

@cosineblast: Thanks for your willingness to contribute! Every PR is welcome, which helps to improve Logisim-evolution! You don't need to close this PR to rework it, but rather mark it as draft and inform us, when you would appreciate some feedback. Please, keep PRs topical and if possible at a manageable size for review. Avoid forces pushes once people have made review comments to code sections. Otherwise, GitHub looses track, which files didn't change since the last review.

[cosineblast]

Alright, I will do so.

[cosineblast]

I have updated this pull request to provide documentation for com.bchurch.logisim.data.
The documentation was mostly derived from the method names, implementation, and how they are used by
implementors and callers. Throughout the process, I have encountered a few things that
may benefit from changes and/or clarifications. I would like to address those if possible,
either in this pull request or another one.

• In Location.java:44, the create the method computes Math.round(x / 5), where x is an integer.
  This call is either unecessary or incorrect, as x / 5 is always an integer.
  Should one adjust it to just x / 5 or Math.round((float) x / 5.0)?

• In Location.java:330, the compare method of the Horizontal comparator returns the difference of
  the hash codes of the two locations.
  In that branch, it doesn't make a lot of sense for me to return the difference of their hashCodes,
  as it is guaranteed that they will have the same x and y components, thus the .equal method returns true and
  and their hashCodes have to be the same.
  Should one make it so that this method returns zero instead?

There are also a few unused methods.

• The attribute class has a parse(Window, String) overload which takes a java.awt.Window as an argument.
  Currently, this method is not utilized in the project, and all it does is ignore the source argument and call
  The original parse(String) method.
  Should one remove or deprecate this method.

• In Value.java:389, there is this controls method.
  It doesn't seem to be used anywhere in the project,
  And it is not very easy to figure it out what it does.
  Should one remove it?