A very opinionated code review of Sponge, a Minecraft modding framework

Gąska

@pie_flavor okay, so I'm making a quick look at that Sponge. The first thing that strikes me immediately is. Extremely. Extremely. Nested Namespaces.

org.spongepowered.api.event.cause.entity.damage.source.common. That's nine nested namespaces. NINE.
org.spongepowered.api.event.entity.living.humanoid.player. EIGHT.
And it's not just events - for example, org.spongepowered.api.extra.fluid.data.manipulator.immutable. Eight nested namespaces. And it contains only two classes. Its sister namespace, org.spongepowered.api.extra.fluid.data.manipulator.mutable, also contains two classes. The parent namespace contains zero. The grandparent namespace also contains zero. The great-grandparent, org.spongepowered.api.extra.fluid, contains six.

That's already 2 architectural WTFs:

Extremely granular namespaces. So granular that they've lost their purpose.

An explosion of classes. Every single tiny thing gets its own type. This is bad. Very bad.

Edit: this is so bad that I've had to get exact numbers. If I haven't made mistake copy-pasting, the whole API has 1882 separate classes, split over 211 different packages. For comparison, the entire System.Windows namespace in WPF has about 1850 classes, and 36 descendant namespaces.

Turns out it's Open Source! We don't have to constrain ourselves to API reference, we can look at code as well! So let's open some random files and see what's inside!

\https://github.com/SpongePowered/SpongeAPI/blob/stable-7/src/main/java/org/spongepowered/api/effect/sound/SoundCategory.java

/*
 * This file is part of SpongeAPI, licensed under the MIT License (MIT).
 * (snip)
 */
package org.spongepowered.api.effect.sound;

import org.spongepowered.api.CatalogType;
import org.spongepowered.api.util.annotation.CatalogedBy;

@CatalogedBy(SoundCategories.class)
public interface SoundCategory extends CatalogType {

}

Empty interface. Lovely.

\https://github.com/SpongePowered/SpongeAPI/blob/stable-7/src/main/java/org/spongepowered/api/effect/sound/SoundType.java

/*
 * This file is part of SpongeAPI, licensed under the MIT License (MIT).
 * (snip)
 */
package org.spongepowered.api.effect.sound;

import org.spongepowered.api.CatalogType;
import org.spongepowered.api.GameRegistry;
import org.spongepowered.api.Sponge;
import org.spongepowered.api.util.ResettableBuilder;
import org.spongepowered.api.util.annotation.CatalogedBy;

/**
 * Represents a sound that can be heard on clients.
 */
@CatalogedBy(SoundTypes.class)
public interface SoundType extends CatalogType {

    /**
     * Creates a new {@link Builder} for building SoundTypes.
     *
     * @return A new builder
     */
    static Builder builder() {
        return Sponge.getRegistry().createBuilder(Builder.class);
    }

    /**
     * Creates a <i>new</i>SoundType from the given ID. To fetch existing types,
     * use {@link GameRegistry#getType(Class, String)}.
     *
     * @param id ID of the sound
     * @return A new SoundType object
     */
    static SoundType of(String id) {
        return builder().build(id);
    }

    /**
     * Builds a SoundType, primarily for sending custom sounds to the client.
     */
    interface Builder extends ResettableBuilder<SoundType, Builder> {

        /**
         * Builds a new instance of a {@link SoundType}.
         *
         * <p>Note: If no domain (indicated by the string before ':') is present
         * in the id, the default "minecraft" domain will be used.</p>
         *
         * @param id ID of the sound
         * @return A new instance of the sound type
         */
        SoundType build(String id);

    }
}

Boring boilerplate that looks like it's repeated a lot and could be easily generalized.

\https://github.com/SpongePowered/SpongeVanilla/blob/stable-7/src/main/java/org/spongepowered/server/chat/ChatFormatter.java

This one is quite large, so I'll be doing short snippets.

    private static final String DEFAULT_SCHEME = "http://";

    private static final String SCHEME = "https?://";
    private static final String IP_ADDRESS = "(?:\\d{1,3}\\.){3}\\d{1,3}";
    private static final String DOMAIN = "(?:[a-z\\d](?:[a-z\\d-]*[a-z\\d])?\\.)+[a-z](?:[a-z\\d-]*[a-z\\d])?";
    private static final String PORT = "\\d{1,5}";
    private static final String PATH = ".*?";

    private static final Pattern URL_PATTERN = Pattern.compile(
            "(?:" + SCHEME + ")?(?:" + IP_ADDRESS + '|' + DOMAIN + ")(?::" + PORT + ")?" + PATH + "(?=[!?,;:\"']?(?:[§\\s]|$))",
            Pattern.CASE_INSENSITIVE);

If only they could use something like java.net.URL to do the work for them... Oh wait they can.

        Matcher matcher = URL_PATTERN.matcher(s);
        if (!matcher.find()) {
            return null;
        }

        ITextComponent result = null;

        int pos = 0;
        do {
            int start = matcher.start();
            int end = matcher.end();

            String displayUrl = s.substring(start, end);

Someone should show them Matcher.group(), it would blow their mind.

            if (pos < start) {
                if (result == null) {
                    result = new TextComponentString(s.substring(pos, start));
                } else {
                    result.appendText(s.substring(pos, start));
                }
            }

            // ...

            if (result == null) {
                result = new TextComponentString("");
            }

Either microoptimization, or they're not realizing they're always creating empty TextComponentString anyway. Dunno which is worse.

Okay that's all I have for today. I'll look for some truly WTF code tomorrow.

pie_flavor

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

Empty interface. Lovely.

Does there need to be anything there? SoundCategory is a distinct subset of CatalogType but doesn't have any extra properties beyond what the CatalogType interface provides.

Boring boilerplate that looks like it's repeated a lot and could be easily generalized.

Not really. Only a SoundType.Builder can build a SoundType and have the necessary fields for a SoundType. Same goes for every other ResettableBuilder. Unless you want to introduce reflective overhead on what is basically a constructor and rely on arbitrary string constants.

If only they could use something like java.net.URL to do the work for them... Oh wait they can.

It is well known that URL and URI are total shit and not standards compliant.

Someone should show them Matcher.group(), it would blow their mind.

Fair.

Either microoptimization, or they're not realizing they're always creating empty TextComponentString anyway. Dunno which is worse.

Wat? How are they always creating an empty one?

Okay that's all I have for today. I'll look for some truly WTF code tomorrow.

Here, I'll save you a bit of trouble:

Remy Porter / Sep 22, 2016

As The World Ternaries

Ah, the ternary operator. At their worst they’re a way to obfuscate your code. At their best, they’re a lovely short-hand. For example, you might use the ternary operator to validate the inputs of a function or handle a flag.

Although Remy got it wrong - it went from four lines to the other four, it wasn't eight to start with.

dkf

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

If only they could use something like java.net.URL to do the work for them... Oh wait they can.

Unless they're planning to download from that URL, java.net.URI is better. It doesn't have a retarded-broken equality test for starters…

Gąska

@pie_flavor said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

Empty interface. Lovely.

Does there need to be anything there? SoundCategory is a distinct subset of CatalogType but doesn't have any extra properties beyond what the CatalogType interface provides.

But why interface? What benefit is there to having an interface, as opposed to, I don't know, enum?

Boring boilerplate that looks like it's repeated a lot and could be easily generalized.

Not really. Only a SoundType.Builder can build a SoundType and have the necessary fields for a SoundType. Same goes for every other ResettableBuilder. Unless you want to introduce reflective overhead on what is basically a constructor and rely on arbitrary string constants.

Or a generic Builder<T> interface that will work just the same but won't have to be redeclared for each and every resource type. Even better, you can move the builder() method into it, with a single implementation for all builders, and then SoundType becomes empty interface, which can be converted to enum too!

If only they could use something like java.net.URL to do the work for them... Oh wait they can.

It is well known that URL and URI are total shit and not standards compliant.

So is that regex. Why do a shitty job yourself when the standard library has already done it?

Either microoptimization, or they're not realizing they're always creating empty TextComponentString anyway. Dunno which is worse.

Wat? How are they always creating an empty one?

Well, not always empty, but they're always creating one. If they just created the empty one in the beginning, they could replace three if/elses with just one line each.

dkf

@dkf OK, now I've read through the code, I see that they're not using URL and shouldn't be doing so, since what that code is doing is mainly going through a chunk of text and putting it in a GUI while making substrings that look like URLs into things that work like a link. The code could be slightly neater, but only slightly; it would still need to be nearly as awful as it currently is.

The one thing that they get completely wrong is this bit:

if (new URI(url).getScheme() == null) {
    url = DEFAULT_SCHEME + url;
}

That should have been written as (with an appropriate default):

uri = DEFAULT_URI.resolve(uri).toString();

dkf

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

So is that regex. Why do a shitty job yourself when the standard library has already done it?

They aren't parsing stuff that should be a URL. They're looking through user text for stuff that might be a URL. Very different use-case.

Gąska

@dkf ah, fair point. Still, they don't need to use nearly as many lookaheads in their regex.

dkf

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

Still, they don't need to use nearly as many lookaheads in their regex.

They only use one: "(?=[!?,;:\"']?(?:[§\\s]|$))". The rest are non-capturing sub-expressions; those are pretty cheap and not a problem.

Gąska

@dkf okay, I see. I wasn't familiar with that syntax.

dkf

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

I wasn't familiar with that syntax.

A very common statement when faced with an advanced RE. (I always have problems with mid-RE anchors; I just can't seem to remember them right. It doesn't help that they're not in any way a standardised syntax form…)

Deadfast

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

@dkf okay, I see. I wasn't familiar with that syntax.

I don't blame you. Looks like a cat ran over the top row of the keyboard and I say that as someone who does actually use regex on occasion.

anonymous234

@dkf said in A very opinionated code review of Sponge, a Minecraft modding framework:

I always have problems with mid-RE anchors; I just can't seem to remember them right. It doesn't help that they're not in any way a standardised syntax form…

Terse languages bad.
Verbose languages good.

Gąska

@anonymous234

Special characters bad.
Letters good.

boomzilla

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

But why interface? What benefit is there to having an interface, as opposed to, I don't know, enum?

Using the language's typing? I dunno...I expect @antiquarian will be along soon to make some snide Ada remark or @Captain will post some cryptic Haskell code.

Gąska

@boomzilla said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

But why interface? What benefit is there to having an interface, as opposed to, I don't know, enum?

Using the language's typing? I dunno...I expect @antiquarian will be along soon to make some snide Ada remark or @Captain will post some cryptic Haskell code.

While it is a valid solution in some other languages sometimes (e.g. in Scala, which lacks enums altogether), in Java it's not. Whatever you're doing, you can do much better in other ways.

cvi

@boomzilla said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

But why interface? What benefit is there to having an interface, as opposed to, I don't know, enum?

Using the language's typing?

Why not? template< SomeEnum > class Foo.

(It has its uses.)

boomzilla

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

@boomzilla said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

But why interface? What benefit is there to having an interface, as opposed to, I don't know, enum?

Using the language's typing? I dunno...I expect @antiquarian will be along soon to make some snide Ada remark or @Captain will post some cryptic Haskell code.

While it is a valid solution in some other languages sometimes (e.g. in Scala, which lacks enums altogether), in Java it's not. Whatever you're doing, you can do much better in other ways.

Bullshit. How are you doing polymorphism with enums much better than using an interface?

Gąska

@boomzilla but they're not doing polymorphism. Those interfaces are empty.

boomzilla

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

@boomzilla but they're not doing polymorphism. Those interfaces are empty.

And? So you have methods that expect a parameter of that interface and not another one. But other methods expect different interfaces. You're only thinking from one side. Or I'm using the word not quite correctly and your brain isn't smart enough to put the pieces together.

dkf

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

Those interfaces are empty.

They inherit from a non-empty type, so they're not useless, but even so it's a bit fussy. The main benefit of doing this is that they can declare, for example, that a method takes objects made by one kind of factory and not another, despite both intermediate objects having the same method signature. That's a form of (weak!) functional interface semantics guarantee, and an example of why just matching stuff by the basic function signatures (as Go interfaces do) is insufficient.

antiquarian

@boomzilla said in A very opinionated code review of Sponge, a Minecraft modding framework:

I expect @antiquarian will be along soon to make some snide Ada remark or @Captain will post some cryptic Haskell code.

I think something like Haskell's parsec would probably be a lot more readable. You could do it in Ada but your fingers would fall off.

Gąska

@dkf said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

Those interfaces are empty.

They inherit from a non-empty type, so they're not useless, but even so it's a bit fussy. The main benefit of doing this is that they can declare, for example, that a method takes objects made by one kind of factory and not another, despite both intermediate objects having the same method signature. That's a form of (weak!) functional interface semantics guarantee, and an example of why just matching stuff by the basic function signatures (as Go interfaces do) is insufficient.

It looks like what they're doing is reimplementing enums as classes because enums are evil or something. I haven't quite got to the bottom of what exactly they're using it for, but my gut instinct says "gloves".

Especially since Java enums can have fields and methods and even implement interfaces just fine.

dkf

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

It looks like what they're doing is reimplementing enums as classes because enums are evil or something.

I've not dug deep enough to see if you're correct or wrong there. I simply don't feel like digging through that much code written by people I don't know at all.

Gąska

@dkf also, the fact that IDs of enums of different types have to distinct sounds like they're doing something very wrong.

dkf

@Gąska They might be mapping things in the Minecraft code itself, so conventional techniques won't apply nicely?

Benjamin Hall

@dkf said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska They might be mapping things in the Minecraft code itself, so conventional techniques won't apply nicely?

From what I've heard, Minecraft's core code is its own pile of steaming s

Carnage

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

@boomzilla but they're not doing polymorphism. Those interfaces are empty.

TLDR: Ramblings. Feel free to ignore.

While it does seem like an enterprisey solution just for the sake of enterprisey, this is sadly not that uncommon. And it does have it's uses, for instance if you have lists of things that gets done something with during a specific cycle, so you make the list take a specific type and some items need to go in different lists because raisins, so you end up with empty interfaces for the stuff that really doesn't need anything apart from the base type. Now, there's probably some other fuckery going in in this case but it does have it's uses.
Also, I've worked in a place that explicitly forbade enums in java, because EVUL! but I never got any explanation of what was so evil about them. This fella might have gotten is first exposure in one of those strange places.
Now, I haven't looked at the code myself, because I get plenty of shitty code in my daily life so getting more than a representative snippet of horribleness is more than I can muster the energy to deal with.

JBert

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

@dkf said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

Those interfaces are empty.

They inherit from a non-empty type, so they're not useless, but even so it's a bit fussy. The main benefit of doing this is that they can declare, for example, that a method takes objects made by one kind of factory and not another, despite both intermediate objects having the same method signature. That's a form of (weak!) functional interface semantics guarantee, and an example of why just matching stuff by the basic function signatures (as Go interfaces do) is insufficient.

It looks like what they're doing is reimplementing enums as classes because enums are evil or something. I haven't quite got to the bottom of what exactly they're using it for, but my gut instinct says "gloves".

Especially since Java enums can have fields and methods and even implement interfaces just fine.

One thing to watch out for with enums is that an enum can implement an interface, but nothing can extend an enum nor can an enum extend an abstract class (because under the hood anything declared as an enum will always have Enum<T> as a base class).

Gąska

@JBert an enum extending a class would be truly horrible.

pie_flavor

@Gąska There's a very good reason for that. On the back end they actually are implemented by enums most of the time. However, Sponge is designed to be used with Forge, so client modding may add more of any enum at any time. Thus, to be better able to handle mods via the API, any enum-like construct is usually done as an interface extending CatalogType, with another class holding static instances of the default members (marked by @CatalogedBy). All instances, whether in the container class or not, get registered to the GameRegistry on startup. They get registered under their corresponding Class object, which is why there's empty interfaces. Then, during the game, instances that the plugin didn't know about beforehand or only has the ID of can be found via the registry, with the static instances only existing for the convenience of hardcoding types. This gives the advantage of always being able to programmatically handle elements of these pseudo-enums, even if they didn't exist when you wrote your program, most notably seen with ItemType, BlockType, EntityType, SoundType, etc. In fact there are a couple of JVM languages that you can't use with Forge because they naively assume that only the enum variants present at compile time will exist at runtime.

Captain

@Gąska Aren't java enums closed? And isn't this a game modding engine, where you'd want to construct custom enums at compile-time or run-time?

Gąska

@pie_flavor said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska There's a very good reason for that. On the back end they actually are implemented by enums most of the time. However, Sponge is designed to be used with Forge, so client modding may add more of any enum at any time. Thus, to be better able to handle mods via the API, any enum-like construct is usually done as an interface extending CatalogType, with another class holding static instances of the default members (marked by @CatalogedBy). All instances, whether in the container class or not, get registered to the GameRegistry on startup. They get registered under their corresponding Class object, which is why there's empty interfaces. Then, during the game, instances that the plugin didn't know about beforehand or only has the ID of can be found via the registry, with the static instances only existing for the convenience of hardcoding types. This gives the advantage of always being able to programmatically handle elements of these pseudo-enums, even if they didn't exist when you wrote your program, most notably seen with ItemType, BlockType, EntityType, SoundType, etc. In fact there are a couple of JVM languages that you can't use with Forge because they naively assume that only the enum variants present at compile time will exist at runtime.

I haven't looked at the code just yet, but based on what you said, I'm 99% convinced that a bunch of classes with a single string field would work just as well.

@Captain said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska Aren't java enums closed? And isn't this a game modding engine, where you'd want to construct custom enums at compile-time or run-time?

There's no way to leverage this compile-time-ness in Java.

boomzilla

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Captain said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska Aren't java enums closed? And isn't this a game modding engine, where you'd want to construct custom enums at compile-time or run-time?

There's no way to leverage this compile-time-ness in Java.

I usually do that by typing new stuff in my source files but YMMV.

Gąska

@boomzilla THIS compile-time-ness. As opposed to OTHER compile-time-nesses, which you can (and I very much hope you do) leverage.

boomzilla

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

@boomzilla THIS compile-time-ness. As opposed to OTHER compile-time-nesses, which you can (and I very much hope you do) leverage.

When I want to leverage OTHER compile-time-nesses I type some more stuff and recompile.

Gąska

@boomzilla exactly. But you can't do that with THIS one.

boomzilla

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

@boomzilla exactly. But you can't do that with THIS one.

True. It's already been compiled. Like tears in the rain...

loopback0

@boomzilla said in A very opinionated code review of Sponge, a Minecraft modding framework:

I usually do that by typing new stuff in my source files

Liar.

Groaner

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

Especially since Java enums can have fields and methods and even implement interfaces just fine.

What the shit?! And in spite of this, people still complain to me about how horrible C++ is?

Groaner

@antiquarian said in A very opinionated code review of Sponge, a Minecraft modding framework:

@boomzilla said in A very opinionated code review of Sponge, a Minecraft modding framework:

I expect @antiquarian will be along soon to make some snide Ada remark or @Captain will post some cryptic Haskell code.

I think something like Haskell's parsec would probably be a lot more readable.

Is that a measure of how long the source code would be?

dkf

@Captain said in A very opinionated code review of Sponge, a Minecraft modding framework:

Aren't java enums closed?

Yes. You can simulate being open by creating an interface that the enum implements and providing a factory that uses a value from the enum when it encounters something it knows about, and uses a slower registry-based mechanism otherwise. The result is a system that has about half the advantages of a pure enum, but which is able to model a space of values where they are not fully known at compile time.

That's occasionally extremely useful, but being able to define the whole space of values (i.e., be a closed set) is better where possible.

dkf

@Groaner said in A very opinionated code review of Sponge, a Minecraft modding framework:

What the shit?!

They're fundamentally objects, not integers. Once you have them, you can still do things like passing them around and switching on them, but with them actually being objects, you can associate multiple constant values with them. This is actually very useful sometimes.

For example, here's a CardinalDirection enum that knows what deltas to apply to transform a coordinate system:

enum CardinalDirection {
    NORTH(0, 1), EAST(1, 0), SOUTH(0, -1), WEST(-1, 0);
    private final int dx, dy;
    CardinalDirection(int dx, int dy) {
        this.dx = dx; this.dy = dy;
    }
    public Coords go(Coords start) { // I suck at method naming tonight
        return new Coords(start.x + dx, start.y + dy);
    }
    // Defining a multi-step version is an exercise for the reader
}

That's a heck of a lot neater than having a bunch of switch statements all over. (We've got some much more complicated versions in our code, that have about seven linked numerical characteristics, yet they really are best expressed as an enum. We've tried it the other way in our Python version of the code and it was U.G.L.Y!)

Captain

@dkf Yeah, that functor is how "data types a la carte" works.

pie_flavor

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

@pie_flavor said in A very opinionated code review of Sponge, a Minecraft modding framework:

@Gąska There's a very good reason for that. On the back end they actually are implemented by enums most of the time. However, Sponge is designed to be used with Forge, so client modding may add more of any enum at any time. Thus, to be better able to handle mods via the API, any enum-like construct is usually done as an interface extending CatalogType, with another class holding static instances of the default members (marked by @CatalogedBy). All instances, whether in the container class or not, get registered to the GameRegistry on startup. They get registered under their corresponding Class object, which is why there's empty interfaces. Then, during the game, instances that the plugin didn't know about beforehand or only has the ID of can be found via the registry, with the static instances only existing for the convenience of hardcoding types. This gives the advantage of always being able to programmatically handle elements of these pseudo-enums, even if they didn't exist when you wrote your program, most notably seen with ItemType, BlockType, EntityType, SoundType, etc. In fact there are a couple of JVM languages that you can't use with Forge because they naively assume that only the enum variants present at compile time will exist at runtime.

I haven't looked at the code just yet, but based on what you said, I'm 99% convinced that a bunch of classes with a single string field would work just as well.

https://jd.spongepowered.org/7.1.0/org/spongepowered/api/item/ItemType.html
Or, heck, see it in action: https://github.com/pie-flavor/Pieconomy/blob/master/src/main/kotlin/flavor/pie/pieconomy/PieconomyCurrency.kt https://github.com/pie-flavor/Pieconomy/blob/master/src/main/kotlin/flavor/pie/pieconomy/PieconomyCurrencyRegistryModule.kt

Gąska

Let's take a look at another class. By @pie_flavor, I'm gonna take on org.spongepowered.common.command now.

\https://github.com/SpongePowered/SpongeCommon/blob/stable-7/src/main/java/org/spongepowered/common/command/SpongeCommandDisambiguator.java

/*
 * This file is part of Sponge, licensed under the MIT License (MIT).
 * (snip)
 */

public class SpongeCommandDisambiguator implements Disambiguator {
    private final Game game;

    /**
     * Disambiguator that takes preferences from the global configuration, falling back to {@link SimpleDispatcher#FIRST_DISAMBIGUATOR}.
     *
     * @param game The game instance to be used
     */
    public SpongeCommandDisambiguator(Game game) {
        this.game = game;
    }

    @Override
    @NonnullByDefault
    public Optional<CommandMapping> disambiguate(@Nullable CommandSource source, String aliasUsed, List<CommandMapping> availableOptions) {
        if (availableOptions.size() > 1) {
            final String chosenPlugin = SpongeImpl.getGlobalConfig().getConfig().getCommands().getAliases().get(aliasUsed.toLowerCase());
            if (chosenPlugin != null) {
                Optional<PluginContainer> container = this.game.getPluginManager().getPlugin(chosenPlugin);
                if (!container.isPresent()) {
                    SpongeImpl
                        .getGame().getServer().getConsole().sendMessage(t("Unable to find plugin '" + chosenPlugin + "' for command '" + aliasUsed
                                                                          + "', falling back to default"));
                } else {
                    final Set<CommandMapping> ownedCommands = this.game.getCommandManager().getOwnedBy(container.get());
                    final List<CommandMapping> ownedMatchingCommands = ImmutableList.copyOf(Iterables.filter(availableOptions,
                            Predicates.in(ownedCommands)));
                    if (ownedMatchingCommands.isEmpty()) {
                        SpongeImpl.getGame().getServer().getConsole().sendMessage(t("Plugin " + container.get().getName() + " was specified as the "
                                                                                    + "preferred owner for " + aliasUsed + ", but does not have any such command!"));
                    } else if (ownedMatchingCommands.size() > 1) {
                        throw new IllegalStateException("Plugin " + container.get().getName() + " seems to have multiple commands registered as "
                                + aliasUsed + "! This is a programming error!");
                    } else {
                        return Optional.of(ownedMatchingCommands.get(0));
                    }

                }
            }
        }
        return SimpleDispatcher.FIRST_DISAMBIGUATOR.disambiguate(source, aliasUsed, availableOptions);
    }
}

Oh hey, they're passing Game as constructor parameter. Cool! Even though it's a singleton, they did the good thing and implemented dependency injection. Too bad they forgot about it three lines later. And two lines earlier, for that matter, where they're using another singleton that's not passed through argument.
toLowerCase on caller's side. This guarantees trouble sooner or later, when someone forgets to use it.
See that copyOf? They're copying the result of list filtering. Either they don't know what they're doing and are copying lists for no reason, which is minor but still a WTF, or they HAVE a reason, in which case it's HUGE WTF.
Negated condition in if/else. A pet peeve of mine.
See that t() function they're using? It's a pretty nice pattern for localization - everything goes through translation module before being printed, but you still can read the original message in source. The translation is in form of key-value pairs, where the key is full text of the string in source code. Too bad they're building these strings dynamically here.
Why is Disambiguator taking care of making sure that the plugin does not contain conflicting command names? Why is it not done at plugin load time!?

gleemonk

@Gąska shouldn't you be posting this as an unsolicited code-review on Github?

I might even use a fake account to sprinkle some emojizz on your comments.

pie_flavor

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

toLowerCase on caller's side. This guarantees trouble sooner or later, when someone forgets to use it.

getAliases returns a Map. If you look earlier on the call chain, it's calling directly to an object-mapped configuration.

See that copyOf? They're copying the result of list filtering. Either they don't know what they're doing and are copying lists for no reason, which is minor but still a WTF, or they HAVE a reason, in which case it's HUGE WTF.

Iterables.filter returns an Iterable. Not a List. So they're 'copying' it which really means collecting it.

See that t() function they're using? It's a pretty nice pattern for localization - everything goes through translation module before being printed, but you still can read the original message in source. The translation is in form of key-value pairs, where the key is full text of the string in source code. Too bad they're building these strings dynamically here.

Some of the time it's done for translation purposes. Here it's just a shortcut for Text.of.

Why is Disambiguator taking care of making sure that the plugin does not contain conflicting command names? Why is it not done at plugin load time!?

Because plugin load time isn't when commands are registered. Commands can be registered and unregistered at any point, and are done programmatically rather than through a manifest.

Carnage

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

Let's take a look at another class. By @pie_flavor, I'm gonna take on org.spongepowered.common.command now.

\https://github.com/SpongePowered/SpongeCommon/blob/stable-7/src/main/java/org/spongepowered/common/command/SpongeCommandDisambiguator.java
/*
 * This file is part of Sponge, licensed under the MIT License (MIT).
 * (snip)
 */

public class SpongeCommandDisambiguator implements Disambiguator {
    private final Game game;

    /**
     * Disambiguator that takes preferences from the global configuration, falling back to {@link SimpleDispatcher#FIRST_DISAMBIGUATOR}.
     *
     * @param game The game instance to be used
     */
    public SpongeCommandDisambiguator(Game game) {
        this.game = game;
    }

    @Override
    @NonnullByDefault
    public Optional<CommandMapping> disambiguate(@Nullable CommandSource source, String aliasUsed, List<CommandMapping> availableOptions) {
        if (availableOptions.size() > 1) {
            final String chosenPlugin = SpongeImpl.getGlobalConfig().getConfig().getCommands().getAliases().get(aliasUsed.toLowerCase());
            if (chosenPlugin != null) {
                Optional<PluginContainer> container = this.game.getPluginManager().getPlugin(chosenPlugin);
                if (!container.isPresent()) {
                    SpongeImpl
                        .getGame().getServer().getConsole().sendMessage(t("Unable to find plugin '" + chosenPlugin + "' for command '" + aliasUsed
                                                                          + "', falling back to default"));
                } else {
                    final Set<CommandMapping> ownedCommands = this.game.getCommandManager().getOwnedBy(container.get());
                    final List<CommandMapping> ownedMatchingCommands = ImmutableList.copyOf(Iterables.filter(availableOptions,
                            Predicates.in(ownedCommands)));
                    if (ownedMatchingCommands.isEmpty()) {
                        SpongeImpl.getGame().getServer().getConsole().sendMessage(t("Plugin " + container.get().getName() + " was specified as the "
                                                                                    + "preferred owner for " + aliasUsed + ", but does not have any such command!"));
                    } else if (ownedMatchingCommands.size() > 1) {
                        throw new IllegalStateException("Plugin " + container.get().getName() + " seems to have multiple commands registered as "
                                + aliasUsed + "! This is a programming error!");
                    } else {
                        return Optional.of(ownedMatchingCommands.get(0));
                    }

                }
            }
        }
        return SimpleDispatcher.FIRST_DISAMBIGUATOR.disambiguate(source, aliasUsed, availableOptions);
    }
}
Oh hey, they're passing Game as constructor parameter. Cool! Even though it's a singleton, they did the good thing and implemented dependency injection. Too bad they forgot about it three lines later. And two lines earlier, for that matter, where they're using another singleton that's not passed through argument.

toLowerCase on caller's side. This guarantees trouble sooner or later, when someone forgets to use it.

See that copyOf? They're copying the result of list filtering. Either they don't know what they're doing and are copying lists for no reason, which is minor but still a WTF, or they HAVE a reason, in which case it's HUGE WTF.

Negated condition in if/else. A pet peeve of mine.

See that t() function they're using? It's a pretty nice pattern for localization - everything goes through translation module before being printed, but you still can read the original message in source. The translation is in form of key-value pairs, where the key is full text of the string in source code. Too bad they're building these strings dynamically here.

Why is Disambiguator taking care of making sure that the plugin does not contain conflicting command names? Why is it not done at plugin load time!?

As someone doing backendy servery crap with uptime requirements, those massive, unchecked callchains makes my skin crawl. I mean, they may be perfectly ok, but man... Crawly feels.
Also, the configuration design makes me go "Eh? U Wot?"

dkf

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

See that t() function they're using? It's a pretty nice pattern for localization - everything goes through translation module before being printed, but you still can read the original message in source. The translation is in form of key-value pairs, where the key is full text of the string in source code. Too bad they're building these strings dynamically here.

Guess that tells us that that line of code has never really been tested except on the original author's system (if ever).

(The right way? Translate a template string, then feed the result into String.format for substitution of the values… which may or may not themselves need localization.)

dkf

@Gąska said in A very opinionated code review of Sponge, a Minecraft modding framework:

Negated condition in if/else. A pet peeve of mine.

Also they don't seem to be believers in early-exit. But they love their final prophylactics that just make the code longer and add nothing of value! (I know Josh Bloch thinks they're great. He's sometimes full of shit.)