search

SpongePowered / SpongeAPI

A Minecraft plugin API
http://www.spongepowered.org/
MIT License
1.14k stars 342 forks source link

Ongoing Minor Issue List (OCD list) #221

Closed phroa closed 8 years ago

phroa commented 10 years ago

Track at https://github.com/SpongePowered/SpongeAPI/pull/860

Minor Issues List

Checkstyle warnings

File Problem
ghost commented 10 years ago

OCD is good for those kind of things, haha.

octylFractal commented 9 years ago

Maybe we should cleanup the checked ones?

maxov commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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

nightpool commented 9 years ago

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

or even just the second one.

ryantheleach commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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)
sibomots commented 9 years ago

I think (?) we're in agreement that Optional for method arguments is a specialized case.

Where we may (?) agree is using the Optional idiom for return values in the general case.

Thanks

simon816 commented 9 years ago

This list can now be cleared, to make way for any new minor issues discovered :)

stephan-gh commented 9 years ago

@simon816 Have you fixed all of them in your pull request?

simon816 commented 9 years ago

@Minecrell Yes, that was the main purpose of the PR. Of course you could check by hand if you wanted...

stephan-gh commented 9 years ago

@simon816 That's why I asked, I've updated the issue now.

octylFractal commented 9 years ago

I personally can't hit format on GameRegistry because all of the javadoc comments are incorrectly formatted according to Eclipse + Google style formatter. Possibly something to look into?

AlphaModder commented 9 years ago

Add @SuppressWarnings("rawtypes") to SimpleServiceManager line 86.

JonathanBrouwer commented 9 years ago

Missing 'is' at javadoc in MinecraftVersion:49

stephan-gh commented 9 years ago

@Bammerbom Fixed, thanks

Deamon5550 commented 9 years ago
Deamon5550 commented 9 years ago

Suggested changes from #462

gabizou commented 9 years ago

Missing Javadocs for

Missing additional methods in:

Deamon5550 commented 9 years ago
DDoS commented 9 years ago
caseif commented 9 years ago

Looks like I made a slight error in my last PR. This doc has the number 35 twice.

ST-DDT commented 9 years ago

Here is a list of all classes/interfaces that have missing class javadocs. Most of the Events have already been detected by gabizou, but i added them for completeness.

EDIT: Fixed by gabizou

Shall i help adding them?

octylFractal commented 9 years ago

Tamer.getName returns null, not Optional<String>. Tamer.java

RobertHerhold commented 9 years ago

509 - Fixing a typo ("vaildate" --> "validate") in ItemStack.java at line 156.

gabizou commented 9 years ago

@ST-DDT By default, someone from staff will take care of the outstanding issues on the OCD list. It'll be taken care of soon.

ST-DDT commented 9 years ago

@gabizou Thanks for fixing all those javadoc issues:

I scanned the API for more missing javadocs, and this classes are missing their class javadocs.

In addition to that ItemExtractEvent does not extend Event at all. Was that file commited accidentally?

EDIT: Everything fixed

Deamon5550 commented 9 years ago

@gabizou

boformer commented 9 years ago

GenericArguments.none() should return a static CommandElement instead of rebuilding it every time.

https://github.com/SpongePowered/SpongeAPI/blob/a6ce75fc2804fecbd224854b6687a93c4174ce47/src/main/java/org/spongepowered/api/util/command/args/GenericArguments.java#L74

octylFractal commented 9 years ago
octylFractal commented 9 years ago
AlphaModder commented 9 years ago
ghost commented 9 years ago
Faithcaio commented 9 years ago
boformer commented 9 years ago

Or use the full package declaration.

ryantheleach commented 9 years ago 
* <li>{@link #NORTH} targeting towards -Z</li>
* <li>{@link #EAST}  targeting towards +X</li>
* <li>{@link #SOUTH} targeting towards +Z</li>
* <li>{@link #WEST}  targeting towards -X</li>
* <li>{@link #UP}    targeting towards +Y</li>
* <li>{@link #DOWN}  targeting towards -Y</li>
boformer commented 9 years ago

https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/util/event/factory/EventFactoryPlugin.java

Throws a few checkstyle warning because it uses @param tags in the description. Should be replaced with @code tags.

ST-DDT commented 9 years ago

Some method declarations in DataHolder are generic although they don't have to. This leads to worse usability / more raw casts.

DataHolder holder = null;
DataManipulator<?> data = null;
holder.offer(data); // Does not compile
holder.offer((DataManipulator) data);

That offer is usually used in for each loops like SpongeItemStackBuilder.

They should be declared this way:

or that way:

ghost commented 9 years ago

Documentation for Direction#getClosest(Vector3d) is incorrect

RobertHerhold commented 9 years ago

Clean up some javadocs for MessageSinks#combined and MessageSinkFactory#combined: #727

ryantheleach commented 9 years ago

https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/service/permission/context/Context.java#L44

Should these be keys or types? The constructor and method calls them types but the public fields call them keys.

simon816 commented 9 years ago

Quote from @AlphaModder Now that doclint is turned off, there is no need for unnecessary imports like the ones here

IMO they are still needed. I think they are used when generating the javadoc HTML in order to provide links to the docs for that class.

Quote from @saladoc InventoryOperationResult.Type.Cancelled has incomplete javadoc

What's incomplete? There have been no changes since you wrote that comment and to me it reads fine.

ST-DDT commented 9 years ago
Deamon5550 commented 9 years ago

Coerce.toLong Number.shortValue() should be Number.longValue().

ST-DDT commented 9 years ago
JBYoshi commented 9 years ago

@ST-DDT

CommandFlags:162 - [fallthrough] possible fall-through into case

I think what's wrong is that there is a space between // and $. Now: // $FALL-THROUGH$ Should be: //$FALL-THROUGH$

ryantheleach commented 9 years ago

https://github.com/SpongePowered/SpongeAPI/blob/master/src/main/java/org/spongepowered/api/text/Texts.java#L467

Javadoc states it takes a color to strip as opposed to a format marker. Implementation instead looks for a formatter marker and removes both the marker and the color.

https://github.com/SpongePowered/SpongeCommon/blob/master/src/main/java/org/spongepowered/common/text/LegacyTextRepresentation.java#L201

Either the JavaDocs should be fixed, or the implementation should be changed to match the javadocs.