CivClassic / NameLayer

Allows the creation of configurable player made groups in Minecraft. Built for Paper 1.16.5
BSD 3-Clause "New" or "Revised" License
1 stars 19 forks source link

MojangNames + Permission Sorting #35

Closed Protonull closed 3 years ago

Protonull commented 3 years ago

Added persistent Mojang name caching, plus changed the permission GUI to sort permissions alphabetically. The permission GUI diff looks rather scary, but if you look here it's exactly the same as before but inside a sorted for each instead of a for loop.

Maxopoly commented 3 years ago

I think we need to talk about your lamda usage in general. That lambda chaining to replace the loop is an abomination. Yes you can use lambdas to do simple processing of collections, but please don't just replace any and every loop with it just because you can.

List<PermissionType> perms = new ArrayList(PermissionType.getAllPermissions());
Collections.sort(perms, Comparator.comparing(PermissionType::getName));
for(PermissionType perm : perms) {

}

It's much easier to read and easier to maintain imo

Protonull commented 3 years ago

"Abomination," really?

Anyway, the permission sorting was changed that way not "because lambdas", but because I assumed that one-off Stream sorts, particularly in Java 11, were faster and more efficient than duplicating, sorting, and for looping it. Apparently I'm wrong there. This was the only reason I chose to do it that way because it's not like I particularly enjoy having the code be so indented on my tiny screen.

The only other lambda'd iterations are this and this: the former of which I hope you agree is not objectionable, but I do agree the latter is not exactly the most obvious. It was a full function like the former but Intellij screamed at me to simplify it. Anyway, both of them use forEach in lieu of a for loop because save and load are both called asynchronously and interact with a synchronised map, so I deferred to forEach because I assumed that the map's own iteration methods would be better at ensuring thread-safety.

Also, side note, It's really not me just replacing everything with lambdas just because I can, and there might be a little of the pot calling the kettle black here considering your JukeAlert rework :P

Protonull commented 3 years ago

After further reading, yes, replacing the forEach's in MojangNames with for loops would likely require encapsulating synchronize blocks. While I agree that lambdas shouldn't be used just because you can, it's equally silly to go out of your way to avoid lambdas when it's clearly a superior option for the use case.

That being said, I will change the permission sorting back since it's likely the Stream overhead is making the sorting slower.

Maxopoly commented 3 years ago

Nice