search

Retera / WarsmashModEngine

An emulation engine to improve Warcraft III modding
GNU Affero General Public License v3.0
192 stars 37 forks source link

Small improvements #10

Open arturhg opened 2 years ago

arturhg commented 2 years ago

Remove redundant character escape Remove redundant 'if' statement Remove redundant 'else' Remove redundant 'new' expression in constant array creation Remove redundant 'File' instance creation Change C-style array declarations to Java-style array declarations Sort missorted modifiers Replace 'size() == 0' with 'isEmpty()' Remove redundant field initialization Remove unnecessary call to 'toString()' Remove unnecessary conversion to String Replace 'String.equals()' with 'String.isEmpty()' Use Files.newInputStream() and Files.newOutputStream() Remove unnecessary concatenation with empty string Replace manual array to collection copy with 'addAll()' Some other changes and cleanup

Retera commented 2 years ago

Hey! Above I added some review comments. From what I can tell, it seems that you ran an autoformatter on this codebase, but using format style settings that are different than the autoformatter that I had setup in my IDE to run on the code constantly while I was writing it. Do you have a reference point or IDE formatter settings that would cause future author contributions to be consistent with the automatic format you are suggesting here?

berserkingyadis commented 2 years ago

I think a good idea would be having a intellij code style file in the repo and collectively sticking to it to minimize formatting commits. Even if it's the default intellij style.

arturhg commented 2 years ago

@Retera Hey, thanks for comments! I am using Intellij Idea default codestyle settings. But in order not to force everybody to use it, we can try to use CheckStyle or similar thing. In that case we will be able to put CheckStyle config into repository, people will install CheckStyle plugins for their particular IDEs and we will have unified codestyle.

arturhg commented 2 years ago

I think a good idea would be having a intellij code style file in the repo and collectively sticking to it to minimize formatting commits. Even if it's the default intellij style.

Yes, agree. Even if not the completely default codestyle of Intellij Idea or some other one, it will be still helpful to have it in repository. Especially in case, when people are using different IDEs.

berserkingyadis commented 2 years ago

@Retera does it makes sense to squash all this into 1 commit? GIthub may have that funcitonality by now if you wanna go that way

arturhg commented 2 years ago

@Retera Any chance this PR can be merged?

Retera commented 2 years ago

Hey, sorry it's been a while on this. Prior to merging I would like to make sure that I read through each and every line that changed again. It may sound like a sort of personal problem on my part, but in the past I had contributions from other authors in a different open source project that I cared about greatly where I took for granted that the contributors also cared and I did not carefully read their contributions. Eventually it became a code soup so horrendous that I branched off of the version from before their pull requests and stopped following the main branch because I simply did not have time to reconcile their changes against the original intention of the program.

When we do something like an extensive automatic reformat, that increases the amount of work for me to do reviewing and I have had it at the back of my mind to take a look at that for sure, but I just haven't gotten around to it yet.

In the meantime until I get around to that, if you happen to have any specific gameplay needs for the game engine, feel free to make a pull request for specific features that I might be able to review more quickly and maybe I'll be able to get that done sooner. For example, I was on a call with the user CanFight and you may have seen he made a tiny pull request that changed like 6 or so lines to add "Grid" hotkeys, and because I was talking to him (watching him type the code at the time via the screenshare call), I was able to approve that pull request very quickly because it was much faster for me to review.

arturhg commented 1 year ago

@Retera Hi, may it will be easier for you, if we will start with single file scope pull requests?

Retera commented 1 year ago

Sure. That seemed to be working for other recent pull requests by bearU369. It creates less for me to look through, so I can get through it more quickly.