BearsDen989-GT / Gregtech-5u-Bears-Den-Edition

Gregtech 5u Bears Den Edition
Other
6 stars 6 forks source link

GT_Values.java should have more descriptive variable names #72

Closed diregarath closed 5 years ago

diregarath commented 5 years ago

GT_Values.java should have more descriptive variable names.

Single character constants are not as readable.

leagris commented 5 years ago

https://github.com/BearsDen989-GT/Gregtech-5u-Bears-Den-Edition/blob/a8d5a18dc6a706083d8c15cee7fb8e01692d4cbc/src/main/java/gregtech/api/enums/GT_Values.java#L25-L27

Ignoring offensive wording above; Is it a valid point to:

Given original @GregoriusT explicit intended use of single letter names, and besides backward compatibility concerns with add-ons (GT++, GTTweaker…), or any 3rd-party code using GregTech API; Are there other motivations like some memory footprint optimization to keep using the shortest names possible?

diregarath commented 5 years ago

Nope - the compiler will convert the code into the same byte code - short names are just worthless at this point and not good practice. Makes the code harder to read and understand. It is too bad that the API that was exposed has made it harder to move away from 1 letter names. As Bear said, lets not spend too long on fixing 1.7.10 - but if we can make it better - by removing the TF constants - without adding new work - I see no reason not to.

GregoriusT commented 5 years ago

I added those because it's way faster to type T or F and because it doesn't consume so much screen real estate. Also there is plenty of Tables in GregTechs Code, which are made way more readable by having single char width constants rather than 4 to 5 char width constants.

To me, readability also comes with shorter names for stuff, it always irritated me when people say "just give it a long descriptive name, short names are from the olden days", and then had an excuse for function names such as getHippopotoMonstrosesQuipedalioPhobiaValue() that make things more confusing than simple and even result oftenly in going Offscreen before you even entered the Parameters into the ( ).

So year, short names are better for understandability if you choose the right ones. And who cant just look up what T and F mean, they are obvious constants in every text highlighter, and once you know them they make much more sense.

OvermindDL1 commented 5 years ago

@GregoriusT Acting as the devils advocate, this:

doSomething("blah", T, F, T, T, "xxx", T, F, F, MD.bloop);

Is not more readable than this:

buildSomething("blah").setBleep().setVwoop().setRecipe("xxx").setBlah().setMat(MD.bloop).create();

Sure it is longer but any decent java formatter will break it up into multiple lines like this once it hits a certain length:

buildSomething
  .setName("blah")
  .setBleep()
  .setVwoop()
  .setRecipe("xxx")
  .setBlah()
  .setMat(MD.bloop)
  .create();

Of which in either form you can tell precisely what it's doing, saying, can leave out default values with ease, etc... etc..., all well typed, unlike a munged mess of parameters, in addition to being hold 'intermediate' steps of a builder:

SomethingBuilder type3 = buildSomething.setBleep().setVwoop().setMat(MD.bloop);

type3.clone().setName("blah").setBlah().create();
type3.clone().setName("blah2").setBlah(2).create();

And so forth.

\

GregoriusT commented 5 years ago

Yep that one is better indeed, for most cases, heck I use that for the GT6 Material System already. ;P

leagris commented 5 years ago

Ah builder pattern ^^ I wish there was some in GT5 in alot of places

OvermindDL1 commented 5 years ago

I'm not entirely a fan of the builder pattern, at all, but Java doesn't have the facilities for something better that is as descriptive... ^.^;

leagris commented 5 years ago

deserialisation maybe but it is a bulldozer to just pass structured parameters

leagris commented 5 years ago

Fixed by https://github.com/BearsDen989-GT/Gregtech-5u-Bears-Den-Edition/pull/74