ModCoderPack / MCPBot-Issues

Issue tracker for MCPBot (https://bitbucket.org/ProfMobius/mcpbot) and MCP mappings.
http://mcpbot.bspk.rs
80 stars 11 forks source link

Class naming convention #814

Closed kashike closed 5 years ago

kashike commented 5 years ago

We're planning to do the following changes to make things consistent, and 1.14 is a good time to do it with the large changes.

Deadline for feedback is Monday (29/04/2019).

Shadows-of-Fire commented 5 years ago

👎 for suffixes, the general feedback from previous questions like this has been to keep prefixes. At least for game objects (anything that goes into a registry), prefixes should remain. The only case where the prefix is not useful for matching two alike-prefixed things together is a block and it's pair itemblock.

The Enum prefix can go die in a fire, it's not implemented consistently anyway. The I prefix for interfaces is inconsistent with normal java interface names, and could be dropped without problems, but it is helpful for distinguishing what is/isnt an interface without having to look up the class.

tterrag1098 commented 5 years ago

Suffixes

e.g. FoodItem, OreBlock

PROs

CONs

Prefixes

e.g. ItemFood, BlockOre

PROs

CONs


Personally I prefer prefixes, but I feel it is necessary to lay out an objective list of pros and cons. If you have any I missed here, feel free to comment and I will add them. But reasons like "it's better" and other subjective things do not belong here.

Sollace commented 5 years ago

@kashike I would still keep the I prefix on interfaces as a way to distenguish them. Otherwise you can't tell if something is a class or an interface, simillar to the confusion that can occur if you name a generic type class Foo.

marvin-roesch commented 5 years ago

I've mentioned this in previous discussions and will reiterate here: neither prefixes nor suffixes are objectively better for IDE discovery. It's entirely up to what "property" of the class you're interested in for you search.

A prefix search like 'Block*' is probably better if you're not entirely certain what class a block is associated with, since you see a neatly aligned list where you can focus on the suffix.

Suffixes, on the other hand, arguably are better when you're looking for anything that is e.g. related to chests. Typing in 'Chest' would immediately yield the associated block, tile entity, GUI and container class, with the important part similarly aligned.

In summary, one might say that prefixes are favourable for "type"-related searches while suffixes benefit "object"-related searches. Hence, what you listed as pro for prefixes above is ultimately subjective and I'd request it to be either removed or changed such that it reflects the concrete search benefit. Then you should also add the pro I outlined above to suffixes, though.

tterrag1098 commented 5 years ago

@PaleoCrafter I updated the list to better reflect the objective benefits.

Commoble commented 5 years ago

if we change prefixes to suffixes will ItemBlock be renamed BlockItem

RCXcrafter commented 5 years ago

I'm indifferent about the prefix suffix thing but I definitely think the interface and enum prefix should not be dropped. the prefix allows you to immediately see what kind of class something is and it's very self explanatory. The Enum prefix could be changed to E if anyone is bothered by the length.

gigaherz commented 5 years ago

Big 👍 from me to change to suffixes. It feels more natural from a programming point of view -- most professional settings use suffixes for this kind of thing.

Also,

Easier to search for by object Easier to search for by type

I don't fully agree with that, it's exactly as easy to enter "block flower" as "flower block" in the symbol search, at least on IDEs capable of substring searches.

Eg, typing "Flower" in Intellij on a 1.12 project I happen to have open right now:

image

The fact the block class starts with Block is irrelevant there -- you get the same overview.

marvin-roesch commented 5 years ago

@gigaherz, it is of course correct that IDEs will search for substrings, but the argument goes beyond the act of finding anything at all.

IDEs still prefer prefix matches over substrings in their search results and you don't want to walk through a long list to get to the actually desired result. Plus, I'd argue that if the top search results share a common prefix, it's a lot easier to parse the important information quickly.

So it's not so much about the search function itself, but the representation of results that differs and favours prefixes (be they "type" or "object").

mcenderdragon commented 5 years ago

I would also say keep the Enumand I prefixed, when reading code it is easier to know what these objects are. About the suffix & prefix thing I am not sure, I personaly use prefixes for everything becuase I like "HelperRendering" more than "RenderHelper" as I can jsut type "Helper" and get all helper classes, but thats just personal coding style.

ChloeDawn commented 5 years ago

I'll be glad to see I prefix gone, it conflicts with the JDK's primary naming pattern let alone what Mojang might be doing

Zidane commented 5 years ago

The only value in keeping Prefixes are when you are looking for a Block but aren't sure what it is but would know it when you see it. Which is of little value in my opinion. Besides any modern IDE does exactly what @gigaherz showed above so Prefix is even more useless.

Now I/Enum prefixing ha some logic behind it. Unless you look at the object's definition you may not know exactly what it is at face value. Even so, it only takes a moment to see it and then that Prefix looses all value. So I would still axe those as well.

That way we can finally start making MCP consistent (at least the class names).

gabizou commented 5 years ago

Personally speaking:

  • Drop the I prefix for interfaces
  • Drop the Enum prefix for enumerations

EnumX prefixing can die in a fire. Along with IInterface prefixing. The prefixing not only is silly, but it's also overly verbose for something as simple as BlockSlab.EnumBlockHalf. We already have the understanding that these enum types are enums from IDE intelligence and to "see whether something is an enum" by looking, well, that's just mostly familiarity of the codebase at that point. Knowing whether something is an interface is also a moot point because of familiarity with the codebase. It doesn't change much if anything whether I'm referring to a BlockState versus IBlockState, they're both interfaces and abstract to the IDE, and me, the developer, I'd know when I'm writing and reading what it is because of syntax highlighting.

In short, I don't need the name to explicitly tell me that it's an interface, otherwise we should be renaming literally everything to be abstract or interfaced or enumed etc. For example, we would/should rename MinecraftServer to AbstractMinecraftServer or AMinecraftServer if we were to want to keep ICommandSender or IRegistry or EnumBlockHalf or EBlockHalf. It's silly to try to keep that sort of consistency as exceptions.

  • Suffixes for everything: FooBlock, FooItem, etc

    • not everything with a Block prefix is actually a block (applies elsewhere too):

    • BlockMatcher is actually a predicate

    • BlockPattern is actually a pattern that can be matched in the world

    • BlockState is actually a state of a block

    • doesn't apply to vanilla, but applies to mods who don't use packages and follow vanilla's class naming: FooBlock, FooItem are both grouped together, rather than BlockFoo [... many others ...] ItemFoo

    • while vanilla separates blocks and items into their own packages, a mod is not required to do the same

    • note that we already use suffixes for major refactors and new classes: see Biome and Feature classes for an example

As for preferring BlockX vs. XBlock, I'm impartial as it's still smurf naming, but it can't be avoided, otherwise we'd have to fully qualify different classes for the same thing (like net.minecraft.tileentity.Note vs net.minecraft.block.Note etc.), so in respect for the packages already defining the grouping, I'd wager it's easier to understand NoteBlock and NoteTile equally as BlockNote and TileEntityNote.

With regards to fixing the classes that are now more generic, they should be just a TypeMatcher, TypePattern, etc.

darkevilmac commented 5 years ago

Personally I prefer prefixes for registry items, I can understand getting rid of the IInterface and EnumEnum though. Its easy enough to figure out if something is an interface or enum just by looking at the icon your IDE puts in front of it.

Either way it looks like a majority of people would prefer to use suffix naming anyway, so my opinion isn't really going to change anything. I'll just continue using the naming I prefer in my own projects and get used to suffix naming in Minecraft itself.

LexManos commented 5 years ago

Just a note. I am going to leave this up to the community, I've instructed @kashike to create a new issue, with each of these choices as a separate comment on that issue. So people can vote using the 👍 and 👎 reactions. As having it all in one comment is a "all or nothing" vote, which is not good. This way we have a documented source for why things were chosen.

As for my 2 cents. EnumX: This is a hold over from back before packages/inner classes were a thing. And it made sense to group enums together as we had a giant single So getting rid of it is perfectly fine with me.

'IInterface': This, I take full blame for, it's a personal coding preference, blame the project I was working on directly before the first pass of class names being a C# one where this is a common pattern. I am perfectly fine with removing them.

Pre-vs-Post smurp: The prefix smurphing, again is a hold over from pre-package days. Where we essentially faked packages by grouping class names together with prefixes. I prefer suffix, as it gets the information I care about to me first. "I know they are Blocks they are in the block package". But I also see the status-quo argument. So I either way works for me. We'll see how the votes go.

Are there any other things that need to be brought up?

The plan is to have the deadline for votes/new class names be Monday. So I can finish the 1.14 update. However the votes go, this will become the new standard. And all legacy names will be converted to follow it. This means large scale class renames. But we will provide a list for people who want to auto-refactor their code.

Note: Anyone who wants to help with new class names for 1.14, #mcpconfig on the Forge discord.

kashike commented 5 years ago

Discussion and voting are moving to separate issues: #815, #816, and #817.