search

atlarge-research / opencraft

Other
5 stars 2 forks source link

Reduce the complexity of the BoundingBoxes.java class - [merged] #177

Closed jdonkervliet closed 4 years ago

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 14:44

Merges feature/refactor-block-bounding-boxes -> development

This merge request reduces the cyclomatic complexity of the BoundinBoxes class. It contained a really big getBoundingBoxes method that is now split up.

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 14:45

added 1 commit

Compare with previous version

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 15:39

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 99

Is there a better name for this variable? Seems redundantly long and non-descriptive.

Also, it might be better to create your own 'Size' or 'Dimensions' class for storing the doubles instead of storing them like this. It would improve readability in later parts of the code (e.g. in the createBoxFromDimensions method).

You could use the Map interface for the definition here as well.

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 15:40

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 111

Missing whitelines at the start (and inbetween?) this codeblock.

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 15:43

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 149

I do not really like all these numbers like this. Is there really no alternative?

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 15:44

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 98

Can't we just call this one gates?

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 15:48

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 504

It might be neater to just put this in a separate method, instead of chaining method calls and computation like this.

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 15:51

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 537

If these have the same bounding box, shouldn't they also have their own set?

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 15:54

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 418

As this test is a lot cheaper, it might be better to put it first in the if-else statement.

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:04

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 99

I could store this in a Dimensions class in the constants package.

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:18

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 149

Not really, all measurements in minecraft are given in 1 /16.0th's and mostly are mostly design driven

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:18

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 98

Sure

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:19

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 537

You are right

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:19

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 418

Oke

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:47

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 504

Done

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:48

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 99

changed this line in version 3 of the diff

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:48

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 149

changed this line in version 3 of the diff

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:48

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 98

changed this line in version 3 of the diff

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:48

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 504

changed this line in version 3 of the diff

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:48

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 537

changed this line in version 3 of the diff

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:48

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 418

changed this line in version 3 of the diff

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 16:48

added 1 commit

Compare with previous version

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 17:06

added 1 commit

Compare with previous version

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 17:07

added 1 commit

Compare with previous version

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 17:47

Commented on src/main/java/net/glowstone/entity/physics/BoundingBox.java line 10

Missing whiteline at start of class.

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 17:47

Commented on src/main/java/net/glowstone/entity/physics/BoundingBox.java line 12

Can these values change? If not, they should be final.

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 17:48

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 637

It might be neater to use full-words like location instead of loc.

jdonkervliet commented 4 years ago

In GitLab by @swabbur on Jun 9, 2020, 17:49

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 637

The name getRemainingBoundingBoxes is a little ambiguous. Are you getting the remaining bounding boxes, or the correct box from those remaining?

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:26

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 637

The latter one. Do you know of a potential name suggestion?

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:33

Commented on src/main/java/net/glowstone/entity/physics/BoundingBox.java line 12

changed this line in version 6 of the diff

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:33

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 637

changed this line in version 6 of the diff

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:33

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 637

changed this line in version 6 of the diff

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:33

added 1 commit

Compare with previous version

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:33

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 637

done

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:33

Commented on src/main/java/net/glowstone/entity/physics/BoundingBox.java line 12

Done

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:34

added 1 commit

Compare with previous version

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:34

Commented on src/main/java/net/glowstone/entity/physics/BoundingBox.java line 10

Fixed

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:35

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 637

I changed it to getNonGeneralBlockBoundingBoxes is that oke?

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 9, 2020, 18:36

added 1 commit

Compare with previous version

jdonkervliet commented 4 years ago

In GitLab by @JimVliet on Jun 9, 2020, 20:07

Commented on src/main/java/net/glowstone/entity/physics/BlockBoundingBoxes.java line 637

Actually, NonGeneral seems worse to me. The docs states that it returns all the bounding boxes that are not edge cases or cases that require special functions. Which seems really weird in the first place. If you were to name it based on the first part, something like getSimpleBoundingBoxes or getDefaultBoundingBoxes would make sense. But if you were to name it based on the second part, something like getUniqueBoundingBoxes or getSpecialBoundingBoxes would seem to make sense.

Anyway let me know if I am completely wrong here, since I am net very familiar with this code.

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 10, 2020, 10:21

added 1 commit

Compare with previous version

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 10, 2020, 10:24

resolved all threads

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 10, 2020, 10:34

merged

jdonkervliet commented 4 years ago

In GitLab by @julian9499 on Jun 10, 2020, 10:34

mentioned in commit 13179fe5f80e7fa464523e37c97ffd081134504e