OSM2World icon indicating copy to clipboard operation
OSM2World copied to clipboard

Different types of Refactoring done: (First Time)

Open margin5094 opened this issue 1 year ago • 1 comments

Pull request title: Refactoring I have done refactoring in certain class on the basis of their functionalities.

Description' Refactoring name and place where done:

  1. Rename variable: lat, long, ele is change to latitude, longitude, and elevation that makes more sense.
  2. Extract Method: writeRows extracted from append to make append class less complex.
  3. Decompose conditional: condition for contain method is decomposed into two different method to easily understood.
  4. Extract Class: TraingleFromMode class extracted with methods trianglesFromVertexList, trianglesFromTriangleStrip,triangleVertexListFromTriangleStrip to make it more cohesive.
  5. Replace conditional with polymorphism: ProgramAction Interface made to replace condition in OSM2World.java.
  6. Pull-up method: length() method pull-up into interface Vector3D as VectorXYZ and VectorXZ were having it without implementing it.

Testing Code is compiling and all test-cases are passing like before.

This is my first pull-request. Please let me know if I need to make any further changes.

margin5094 avatar Apr 07 '23 01:04 margin5094

Welcome and thank you for your pull request!

This seems a mixed bag of changes – I agree with some of them, but not all of them. I acknowledge that these things tend to be a matter of taste, so let me explain my thinking on the ones I'd rather not merge:

  • (1) While I do generally agree with valuing clarity over brevity, my impression is that short forms such as "lat" for "latitude" are sufficiently common that they do not significantly affect clarity. And they are used a lot in calculations throughout the code, so the brevity is useful.
  • (2,3) I'm not too eager to split off separate methods which are only used in one location, as you did with writeRows or allMinCoordinatesAreLessThanOrEqual. After all, having the code within a single method communicates some useful information: It's immediately obvious that this code is only used in this location, and changing it won't therefore affect more than one caller. So I would only extract a method if there are clear benefits, such as when this part of the code performs a mentally distinct concept. This is not really the case here.
  • (5) The CLI classes can indeed use some reorganizing, but I already have plans to do this as part of an overhaul of the CLI interface.

I would be willing to merge the changes 4 and 6, preferably with 1 commit per refactoring because these don't have a lot to do with each other.

For change 4, I would prefer a separate top-level class such as TriangleModeUtil. It should be a utility class (following the usual conventions, e.g. a private constructor) and should contain all, not just some, of the relevant methods. That is:

  • trianglesFromVertexList
  • trianglesFromTriangleStrip
  • triangleVertexListFromTriangleStrip
  • triangleNormalListFromTriangleStripOrFan
  • trianglesFromTriangleFan
  • triangleVertexListFromTriangleFan

Also, something I've noticed across your changes is that there are numerous comments in your code which describe the refactorings being applied. In my opinion, code comments should generally document the code, not the changes to the code. So if you choose to submit a new PR, please keep that in mind. :)

tordanik avatar Apr 12 '23 14:04 tordanik