Small improvements #10

arturhg · 2022-04-26T22:45:04Z

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

The streams created using Files methods are usually more efficient than those created by stream constructors.

Retera · 2022-04-27T02:45:19Z

...com/etheller/warsmash/viewer5/handlers/w3x/simulation/behaviors/CAbstractRangedBehavior.java

-				// moving.
-				this.wasWithinPropWindow = false;
-			}
+			// If this happens, the unit is facing the wrong way, and has to turn before


After the proposed change, the meaning of this comment becomes somewhat lost or perhaps misleading. Would be nice to fix this so that if there is going to be a comment, it continues to be meaningful.

Fixed the comment in 645cc15.

Retera · 2022-04-27T02:57:14Z

core/src/com/etheller/warsmash/util/IniFile.java

@@ -8,7 +8,7 @@
 import java.util.regex.Pattern;

 public class IniFile {
-	private static final Pattern NAME_PATTERN = Pattern.compile("^\\[(.+?)\\].*");
+	private static final Pattern NAME_PATTERN = Pattern.compile("^\\[(.+?)].*");


Per the readme, this pattern (and indeed this entire file) is a near-direct copy of:
https://github.com/flowtsohg/mdx-m3-viewer/blob/0162f72ce23a07975764000d7225b7295d7bc460/src/parsers/ini/file.ts#L25
Is there value to distancing ourselves from the original regex over something so pedantic?

Actually, the only reason for this change was to make it simpler. But, I think, you are right, as this is basically a copy from another place, we can skip it and maybe revisit it and similar parts in the future if needed. I will revert the commit with this change.

Reverted in 7c893a4.

Retera · 2022-04-27T03:06:07Z

core/src/com/etheller/warsmash/datasources/CascDataSourceDescriptor.java

@@ -84,13 +84,8 @@ else if (!this.gameInstallPath.equals(other.gameInstallPath)) {
 			return false;
 		}
 		if (this.prefixes == null) {
-			if (other.prefixes != null) {


This method was computer generated by my IDE. If we change it, then I generate another similar method later, they will become needlessly different. What do we propose as a good solution to this dilemma? Ideally, all the computer generated methods will end up the same.

Different IDEs are generating the same methods differently. And, of course, we cannot force everyone to use the same IDE. The possible solution may be to try to use Lombok or something similar to eliminate the boilerplate code. Also, in that case, we will not be forced to change the autogenerated code every time when we are adding, changing or deleting fields. Meanwhile, I will revert this change.

Reverted in 6612bf8.

Retera · 2022-04-27T03:11:04Z

core/src/com/etheller/warsmash/parsers/jass/Jass2.java

-										if (this.count >= countLimit) {
-											return true;
-										}
+                                        return this.count >= countLimit;


My code preview is displaying this return statement as being indented incorrectly. Are you using a different indentation paradigm than the rest of the Warsmash codebase? If so, shouldn't we either change the entire codebase, or change none of it?

Hmm, strange, I see the same on github code preview, but in IDE I see this:

Can you please try also in your IDE?

Retera · 2022-04-27T03:14:53Z

core/src/com/etheller/warsmash/util/Quadtree.java

-				if (this.southeast.intersect(bounds, intersector)) {
-					return true;
-				}
+				return this.southeast.intersect(bounds, intersector);


Isn't this an exceedingly pedantic automated change the interrupts the original style of the code where there were four identical branches that could be easily observed by a developer to be the same (see image for how this code looked on my PC before the change):

Certainly it is correct either way, but if we were ever going to modify this Quadtree to make an Octtree for example, and copy this to have eight branches instead of four, having one of the branches be needlessly different in order to pedantically avoid "if {x} return" syntax seems pointless from my perspective. Curious to hear your opinion or if there's a good reason in favor of changing it that I'm just not seeing.

I agree with you, having one of the branches be needlessly different in order to pedantically avoid "if {x} return" syntax is pointless. Please check 60abd00, where are refactored Quadtree.java.

Retera · 2022-04-27T03:16:19Z

core/src/com/etheller/warsmash/util/Vector4.java

-			return false;
-		}
-		return true;
+		return !(Math.abs(other.w - this.w) > epsilon);


(Similar case to the Quadtree comment.)

Please check 60abd00, where I refactored Vector4.java.

Retera · 2022-04-27T03:19:27Z

core/src/com/etheller/warsmash/viewer5/handlers/w3x/simulation/CWorldCollision.java

-			case FLOAT:
+				return this.groundUnitCollision.intersect(newPossibleRectangle,
+						this.anyUnitExceptTwoIntersector.reset(sourceUnitToIgnore, sourceSecondUnitToIgnore));
+				case FLOAT:


Again it appears that the automatic format indentation here for case FLOAT: does not match all other indentation of all other case statements in this file. When we are going far out of our way to be pedantic, I think we should update cases like this to match the code around them.

Fixed the formatting, added some more small changes in 57b2b04.

Retera · 2022-04-27T03:22:13Z

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 · 2022-04-27T09:44:44Z

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 · 2022-04-27T22:31:46Z

@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 · 2022-04-27T22:36:08Z

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.

…s and cleanup

berserkingyadis · 2022-04-30T08:20:03Z

@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 · 2022-06-12T11:51:51Z

@Retera Any chance this PR can be merged?

Retera · 2022-06-19T21:14:33Z

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 · 2022-09-26T21:13:18Z

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

Retera · 2023-04-05T03:24:22Z

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.

Merge Conflict Resolution

arturhg added 15 commits April 27, 2022 01:42

Remove redundant character escape

c89a8b1

Remove redundant 'if' statement

74f1d9f

Remove redundant 'else'

19a31c6

Remove redundant 'new' expression in constant array creation

e3db1b5

Remove redundant 'File' instance creation

4cb57aa

Change C-style array declarations to Java-style array declarations

6e7a03f

Sort missorted modifiers

287bdf0

Replace 'size() == 0' with 'isEmpty()'

5382430

Remove redundant field initialization

6ff3abf

Remove unnecessary call to 'toString()'

6c52e76

Remove unnecessary conversion to String

952e1f4

Replace 'String.equals()' with 'String.isEmpty()'

7204723

Use Files.newInputStream() and Files.newOutputStream()

1fafa11

The streams created using Files methods are usually more efficient than those created by stream constructors.

Remove unnecessary concatenation with empty string

504186c

Replace manual array to collection copy with 'addAll()'

0e1079d

Retera reviewed Apr 27, 2022

View reviewed changes

arturhg added 5 commits April 28, 2022 01:18

Refactor Quadtree and Vector4

60abd00

Fix formatting, more cleanup

57b2b04

Update the comment to make it useful again

645cc15

Revert the regex change

7c893a4

Revert autogenerated equals() method changes

6612bf8

arturhg requested a review from Retera April 28, 2022 15:32

Merge branch 'experimental' into small-improvements, some more change…

e2b6745

…s and cleanup

Merge branch 'experimental' into small-improvements

d539a8c

Retera pushed a commit that referenced this pull request Nov 25, 2023

Merge pull request #10 from Glasislundr/abilitybuilder

64a880b

Merge Conflict Resolution

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small improvements #10

Small improvements #10

arturhg commented Apr 26, 2022 •

edited

Loading

Retera Apr 27, 2022

arturhg Apr 27, 2022

Retera Apr 27, 2022

arturhg Apr 27, 2022

arturhg Apr 27, 2022

Retera Apr 27, 2022

arturhg Apr 27, 2022 •

edited

Loading

arturhg Apr 27, 2022

Retera Apr 27, 2022

arturhg Apr 27, 2022

Retera Apr 27, 2022

arturhg Apr 27, 2022 •

edited

Loading

Retera Apr 27, 2022

arturhg Apr 27, 2022

Retera Apr 27, 2022

arturhg Apr 27, 2022

Retera commented Apr 27, 2022

berserkingyadis commented Apr 27, 2022 •

edited

Loading

arturhg commented Apr 27, 2022 •

edited

Loading

arturhg commented Apr 27, 2022

berserkingyadis commented Apr 30, 2022

arturhg commented Jun 12, 2022

Retera commented Jun 19, 2022

arturhg commented Sep 26, 2022

Retera commented Apr 5, 2023

Small improvements #10

Are you sure you want to change the base?

Small improvements #10

Conversation

arturhg commented Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturhg Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturhg Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Retera commented Apr 27, 2022

berserkingyadis commented Apr 27, 2022 • edited Loading

arturhg commented Apr 27, 2022 • edited Loading

arturhg commented Apr 27, 2022

berserkingyadis commented Apr 30, 2022

arturhg commented Jun 12, 2022

Retera commented Jun 19, 2022

arturhg commented Sep 26, 2022

Retera commented Apr 5, 2023

arturhg commented Apr 26, 2022 •

edited

Loading

arturhg Apr 27, 2022 •

edited

Loading

arturhg Apr 27, 2022 •

edited

Loading

berserkingyadis commented Apr 27, 2022 •

edited

Loading

arturhg commented Apr 27, 2022 •

edited

Loading