Ongoing Minor Issue List (OCD list)

[phroa]

Track at #860

Minor Issues List

• Discover Minor Issues
• SpongeAPI should use quaternions for rotations (provided by flow-math) as they are superior to euler angles and the standard in games. Both options should be provided: what the implementation doesn't support becomes a simple overload (conversions representations are provided by flow-math).
• Have checkstyle warn on missing Override annotation
• Secondary ordinal or Secondary cardinal? Javadoc and field name conflict. Direction:30
• GameMode javadoc uses fully qualified link to Player
• Some method declarations in DataHolder are generic although they don't have to. This leads to worse usability / more raw casts. See comment: Ongoing Minor Issue List (OCD list) #221 (comment) appears to have been refactored so I won't touch
• GameMode would be better suited in the org.spongepowered.api.data.type package.
• AbstractInventoryProperty could extend AbstractProperty. See comment: Ongoing Minor Issue List (OCD list) #221 (comment) for more
• Make UseItemStackEvent.Finish not cancellable, don't allow modifying the duration in Stop and Finish (changes won't be reflected anyway)

Checkstyle warnings

File	Problem

[ghost]

OCD is good for those kind of things, haha.

[octylFractal]

Maybe we should cleanup the checked ones?

[maxov]

@kenzierocks I considered deleting, but let's keep them there for history. Any new issues will go in a new comment.

EDIT: Github does not track todo items outside of the original issue topic and they're already in the commit history so I'll delete.

[AlphaModder]

Creeper.java line 36: "Gets whether or not the creeper has been struck by lightning." shouldn't it be "Gets whether or not the creeper is powered.", considering plugins can power them.

[nightpool]

@ST-DDT why would we use a nullable when we could use an optional with an overloaded helper?

[nightpool]

i.e. two methods
void setCarriedBlock(BlockState carriedBlock);
and
void setCarriedBlock(Optional<BlockState> carriedBlock);

or even just the second one.

[ryantheleach]

Because null can still fit into both, Optionals are designed for return types.

http://java.dzone.com/articles/java-8-optional-how-use-it

What is Optional not trying to solve
Optional is not meant to be a mechanism to avoid all types of null pointers. The mandatory input parameters of methods and constructors still have to be tested for example.

Like when using null, Optional does not help with conveying the meaning of an absent value. In a similar way that null can mean many different things (value not found, etc.), so can an absent Optional value.

[nightpool]

@ryantheleach I haven't seen any official Oracle statements that have said or implied that Optionals are only for return types, and the Optional used here is not the Java optional anyway. In my opinion moving the possibly of a missing value into the type system is beneficial in many situations, not just for method return statements.

[sibomots]

I'm offering this up as an opinion and I hope there is some truth to it.

TL;DR

In Java we're left with either consuming a result that is either wrapped in some class (eg., the Optional idiom) or else testing a return value specifically (eg., null).

NTL;R

The way I'd suggest looking at this is to treat the SpongeAPI as a black box.

When we use Optional<T> as a return type, the Sponge through the API is providing the plugin developer more than a simple assurance that whatever the result of the method is, the result is safe. The result is required to be tested by the caller in a safe manner.

The example is:

public Optional<T> getSomethingFromSponge();

// ...
// and in the caller:

Optional<T> result = getSomethingFromSponge();

if ( result.isAbsent() ) {
   // proceed down sad path
} else {
   // proceed down happy path
}

vs.

public T getSomethingFromSponge()


// ...
// and in the caller:

T result = getSomethingFromSponge();

if ( result == null ) {
   // proceed down sad path ..  Gee, I think.does null mean it truly failed?
} else {
   // proceed down happy path .. Hmm, a non null value  may mean success, perhaps?
}

I'd rather see the use of Optional in the general case because that API signature conveys to the developer relative safety in the result value. It's harder (much harder) to accidently NPE with a result.

On the other hand, when we look at arguments to methods:

doAnotherThing( T  arg)

I'd rather there be no Optional in the API for arguments. The main reason (going back to the black-box metaphor) is Optional arguments to methods implies that the caller knows that the method may not need the argument. That insider knowledge of the implementation is worrisome.

Methods in the Sponge are (or will be) well crafted and do the right amount of due diligence on the arguments passed to them. Using Optional arguments puts the onus on the plugin author to wrap all their calls to Sponge methods with extra protection just in case Sponge isn't careful with the arguments. I can see why some may be skeptical of any new API implementation, but this is an extreme amount of caution.

Another case for methods with Optional arguments comes up when the Sponge tries to deal with Magic Numbers. These magic numbers are the values used internally to represent meaning or behavior. The magic numbers are not meant ever to be known to the caller.

Optional<T>  getSomethingFromSponge( int  arg ) {

   // ...
   if (arg <=  0) {
      // resort to some default behavior 
      this.internalParameter = -1;
   } else {
       // the argument has quality we can use
       this.internalParameter = arg;
   }

   // return something ...
}

The problem with this kind of code IMHO is that the caller must know in advance that when arg is less or equal to 0, that the internal behavior of the class has a default case. A case where perhaps the behavior in that area is turned off or disabled.

So thus, the argument for Optional in the method parameters would suggest:

public Optional<T> getSomething( Optional<Integer> arg ) {
     // ...
     if ( arg.isAbsent() ) {
         // setup behavior to cope with sad-path
     } else {
         // setup behavior to cope with happy path
     }

     // ...
     // return something ...
}

I'd counter that this excuse for Optional is overlooking that the API is simply missing the method:

public Optional<T> getSomething()

Why pass in an Optional that is known by the caller to be literally absent. The caller would have to explicitly make it so.

I cannot make the blanket statement for Sponge that methods shall not have Optional arguments, but I find their use to be specialized.

I could make a blanket statement for Sponge API design that recommends Optional for the return of objects especially when the attempt to produce the result has a non-zero probability of failing while not killing the server with an unnecessary NPE.

All of this is syntax sugar to overcome the fact that Java is merely pass-by-value. There is simply no way to do what we do with pointers in C/C++:

RESULT  doSomething(  int x,   T* pArg ) {
      // whatever the case may be choose:
      //  A)  modify the whatever pArg points to and then return S_OK    or
      //   B) leave pArg alone and return E_FAIL;
}

// and in the caller:

if  ( SUCCEEDED ( doSomething ( n, pFoo ) )  ) {
    // proceed down happy path
} else {
    // proceed down sad path
}

[ryantheleach]

There is, since objects can be passed in and their reference is passed by value, you can just modify the object passed in and return a result. (of course this changes if you are passing in a primitive)

Result method(int x, Object pArg){
    //A) Modify the fields of pArg or call methods as appropriate, then return Result.OK or
    //B) leave pArg alone and return Result.FAIL
}

if(Results.success(doSomething(n, pFoo)){
    //proceed down happy path
} else {
    // proceed down sad path
}

But the reason why I am against using Optionals in parameters is

1. what does the value being absent mean? if you want the default, why not pass in PickupTime.default, then you can use PickupTime.Infinite or others if you need further context.
2. if you don't need extra's why not just pass in null? sponge implementations should be checking for it anyway, so using a nullable parameter and labelling it such isn't that bad. Or just have multiple methods, setPickupTimeInfinite() setPickupTimeDefault() setPickupTime(int i)

[gabizou]

Inventory.slots() should have a signature of Iterable slots(); because it always returns Slots. @Mumfrey

As mentioned here this is a big fat no:

Leaf nodes are currently guaranteed to be Slots but I definitely don't want this encoded in the API, because the entire point of this is that it can support future changes in minecraft's inventory system. This is why pretty much all generic signatures are , because the contract is designed around the core premise that you deal with Inventorys not any particular sub interface.

• Mumfrey 2015

[ryantheleach]

• Did blockbuffers get removed / renamed from the API? If so https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/world/gen/WorldGenerator.java needs it's JavaDocs updated.

[ryantheleach]

• I wouldn't call this a minor issue, but maybe someone else would: Changed getFromAxis to return correct Direction. #895 Helper method returning wrong Axis

[Rain336]

• In SleepingEvent change the #wasSpawnSet() into #isSpawnSet() in the JavaDocs

[randombyte-developer]

SPAWNER_REQURED_PLAYER_RANGE - typo

[phroa]

I already fixed that in #860.

[JonathanBrouwer]

https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/entity/living/monster/Creeper.java
line 30 creeepr -> creeper

[phroa]

That was also already fixed in my PR.

[octylFractal]

@bloodmc Might need discussion, but things related to BlockTrait values (like getTraitMap) should return ? extends Comparable since all values are Comparable.

For example:

// Current
Map<BlockTrait<?>, ?> getTraitMap();

// Proposed
Map<BlockTrait<?>, ? extends Comparable<?>> getTraitMap();
// or
Map<BlockTrait<?>, Comparable<?>> getTraitMap();

[octylFractal]

Inventory.poll() returns ItemStack, but the javadoc claims it can return Optional.empty()! link to relevant line (Fixed)

[gabizou]

@phroa mind updating the issue description of the various OCD things listed here?

[ryantheleach]

• Create Keys.BABY (boolean) and Keys.Adult (boolean)

[JBYoshi]

@ryantheleach I think only one would be needed - the value of one is simply the opposite of the other.

[ryantheleach]

• Add Enderman Carried Data to api. Different from equipment / loot.

[TheRaspPie]

• Scoreboard.java: line 58, blank line missing.

[ghost]

• Keys.EXPLOSIVE_RADIUS, ExplosiveRadiusData and Explosive are inconsistent in whether the explosive radius is a Value<Integer> or a MutableBoundedValue<Integer>. Should be the latter because negative radius makes no sense.

[JBYoshi]

• ImmutableFlamableData is misspelled

[ZephireNZ]

• Sink package is left over after rename to channels: https://github.com/SpongePowered/SpongeAPI/tree/master/src/main/java/org/spongepowered/api/text/sink

[Deamon5550]

@ryantheleach feature requests aren't minor changes, those should be an issue.

[Deamon5550]

Creating a new OCD list for beta See #1000

Any still unresolved issues from here that I may have missed should be reported there.

[Deamon5550]

@Saladoc remake your concern as an issue as well

[randombyte-developer]

https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/entity/EntityTypes.java#L64 shouldn't it be ZOMBIE_PIGMAN instead of PIG_ZOMBIE?