New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework AEItemStack #3091

Merged
merged 14 commits into from Sep 30, 2017

Conversation

Projects
None yet
3 participants
@fscan
Member

fscan commented Sep 15, 2017

First draft, simple tests seem to work.

This gets rid of the custom AEItemDef and uses ItemStack for it. Deduplication/Sharing is done at ItemStack level instead of NBTTagCompund, this may be less optimal but by using ItemStack we get the benefit of correctly handling caps and other future stuff mc/forge introduces.

}
else if( fuzzy == FuzzyMode.PERCENT_99 )
{
if( this.itemStack.getItemDamage() == 0 )

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

CRITICAL Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule
MAJOR Remove this conditional structure or edit its code blocks so that they're not all the same. rule

@orod-org

orod-org Sep 15, 2017

CRITICAL Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule
MAJOR Remove this conditional structure or edit its code blocks so that they're not all the same. rule

}
else if( fuzzy == FuzzyMode.PERCENT_99 )
{
if( this.itemStack.getItemDamage() == 0 )

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

CRITICAL Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule

@orod-org

orod-org Sep 15, 2017

CRITICAL Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule

@@ -165,8 +165,8 @@ public void partition()
{
if( i.hasNext() && this.isSlotEnabled( ( x / 9 ) - 2 ) )
{
final ItemStack g = i.next().getItemStack();
g.setCount( 1 );
// TODO: check if ok

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@@ -51,7 +51,7 @@ public IAEItemStack injectItems( final IAEItemStack input, final Actionable type
{
if( this.hasPermission( src ) )

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

MAJOR Change this condition so that it does not always evaluate to "false" rule

@orod-org

orod-org Sep 15, 2017

MAJOR Change this condition so that it does not always evaluate to "false" rule

@@ -131,7 +86,6 @@ public static IAEItemStack loadItemStackFromNBT( final NBTTagCompound i )
}
final AEItemStack item = AEItemStack.create( itemstack );
// item.priority = i.getInteger( "Priority" );
item.setStackSize( i.getLong( "Cnt" ) );

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

MAJOR A "NullPointerException" could be thrown; "item" is nullable here. rule

@orod-org

orod-org Sep 15, 2017

MAJOR A "NullPointerException" could be thrown; "item" is nullable here. rule

@@ -146,7 +100,7 @@ public static AEItemStack create( final ItemStack stack )
return null;
}
return new AEItemStack( stack );
return new AEItemStack( AEItemStackRegistry.getRegisteredStack( stack ), stack.getCount() );
}
public static IAEItemStack loadItemStackFromPacket( final ByteBuf data ) throws IOException

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

MINOR Remove the declaration of thrown exception 'java.io.IOException', as it cannot be thrown from method's body. rule

@orod-org

orod-org Sep 15, 2017

MINOR Remove the declaration of thrown exception 'java.io.IOException', as it cannot be thrown from method's body. rule

@@ -48,7 +48,8 @@ public void addProbeInfo( AEBaseTile tile, ProbeMode mode, IProbeInfo probeInfo,
if( displayStack != null )
{
final ItemStack itemStack = displayStack.getItemStack();
// TODO: check if OK

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@@ -62,10 +62,11 @@
final IAEStack<?> displayed = monitor.getDisplayed();
final boolean isLocked = monitor.isLocked();
// TODO: generalize

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

{
if( o == null )
{
return new ArrayList();
return new ArrayList<>();
}
ItemStack itemStack = ItemStack.EMPTY;

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

MAJOR Remove this useless assignment to local variable "itemStack". rule

@orod-org

orod-org Sep 15, 2017

MAJOR Remove this useless assignment to local variable "itemStack". rule

@@ -48,10 +48,11 @@ public void addProbeInfo( IPart part, ProbeMode mode, IProbeInfo probeInfo, Enti
final IAEStack<?> displayed = monitor.getDisplayed();
final boolean isLocked = monitor.isLocked();
// TODO: generalize

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@@ -702,7 +702,8 @@ else if( itemStack.getStackSize() < 0 )
long diff = toStore.getStackSize();
// make sure strange things didn't happen...
final ItemStack canExtract = adaptor.simulateRemove( (int) diff, toStore.getItemStack(), null );
// TODO: check if OK

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

this.putPacketValue( i, this.stackSize );
this.putPacketValue( i, this.countRequestable );
}
protected abstract void writeToStream( final ByteBuf data ) throws IOException;

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

MINOR Redundant 'final' modifier. rule

@orod-org

orod-org Sep 15, 2017

MINOR Redundant 'final' modifier. rule

final byte[] name = this.fluid.getName().getBytes( "UTF-8" );
i.writeByte( (byte) name.length );
i.writeBytes( name );
// TODO: fluids, how do they even work?

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@@ -88,12 +88,14 @@ public void setRecipe( IRecipeLayout recipeLayout, GrinderRecipeWrapper recipeWr
itemStacks.set( ingredients );
}
@Override public void addRecipeCategories( IRecipeCategory... recipeCategories )
@Override
public void addRecipeCategories( IRecipeCategory... recipeCategories )

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

CRITICAL Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation. rule

@orod-org

orod-org Sep 15, 2017

CRITICAL Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation. rule

@@ -240,8 +240,8 @@ public void partition()
{
if( i.hasNext() )
{
final ItemStack g = i.next().getItemStack();
g.setCount( 1 );
// TODO: check if ok

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@@ -484,7 +484,8 @@ public StorageChannel getChannel()
{
if( ais.getItem() == whatToCraft.getItem() && ( !ais.getItem().getHasSubtypes() || ais.getItemDamage() == whatToCraft.getItemDamage() ) )
{
if( details.isValidItemForSlot( slotIndex, ais.getItemStack(), world ) )
// TODO: check if OK

This comment has been minimized.

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@orod-org

orod-org Sep 15, 2017

INFO Complete the task associated to this TODO comment. rule

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Sep 15, 2017

Member

See attached nps snapshots regarding the performance impact of AESharedItemStack#compareTo()

Uncached is the current one, cached does a single lookup in the constructor plus a unchecked cast to AESharedItemStack in compare to access the other one currently.

aeshareditemstack-compareto.zip

Member

yueh commented Sep 15, 2017

See attached nps snapshots regarding the performance impact of AESharedItemStack#compareTo()

Uncached is the current one, cached does a single lookup in the constructor plus a unchecked cast to AESharedItemStack in compare to access the other one currently.

aeshareditemstack-compareto.zip

{
final AESharedItemStack other = (AESharedItemStack) obj;
Preconditions.checkState( this.itemStack.getCount() == 1, "ItemStack#getCount() has to be 1" );

This comment has been minimized.

@orod-org

orod-org Sep 16, 2017

CRITICAL Define a constant instead of duplicating this literal "ItemStack#getCount() has to be 1" 6 times. rule

@orod-org

orod-org Sep 16, 2017

CRITICAL Define a constant instead of duplicating this literal "ItemStack#getCount() has to be 1" 6 times. rule

if( itemstack.isEmpty() )
{
return null;
}
final AEItemStack item = AEItemStack.create( itemstack );
// item.priority = (int) priority;
item.setStackSize( stackSize );

This comment has been minimized.

@orod-org

orod-org Sep 16, 2017

MAJOR A "NullPointerException" could be thrown; "item" is nullable here. rule

@orod-org

orod-org Sep 16, 2017

MAJOR A "NullPointerException" could be thrown; "item" is nullable here. rule

fscan added some commits Sep 21, 2017

private final int itemId;
private final int itemDamage;
public AESharedItemStack( final ItemStack itemStack )

This comment has been minimized.

@orod-org

orod-org Sep 21, 2017

MINOR Redundant 'public' modifier. rule

@orod-org

orod-org Sep 21, 2017

MINOR Redundant 'public' modifier. rule

@orod-org

This comment has been minimized.

Show comment
Hide comment
@orod-org

orod-org Sep 21, 2017

SonarQube analysis reported 83 issues

  • BLOCKER 6 blocker
  • CRITICAL 13 critical
  • MAJOR 36 major
  • MINOR 15 minor
  • INFO 13 info

Watch the comments in this conversation to review them.

Top 10 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. BLOCKER CraftingCPUCluster.java#L466: Refactor this method to not always return the same value. rule
  2. BLOCKER CraftingCPUCluster.java#L847: Refactor this method to not always return the same value. rule
  3. BLOCKER CraftingCPUCluster.java#L961: Refactor this method to not always return the same value. rule
  4. BLOCKER SecurityStationInventory.java#L72: Refactor this method to not always return the same value. rule
  5. BLOCKER SecurityStationInventory.java#L89: Refactor this method to not always return the same value. rule
  6. BLOCKER Platform.java#L1402: Refactor this method to not always return the same value. rule
  7. CRITICAL AEBaseContainer.java#L223: The Cyclomatic Complexity of this method "setTargetStack" is 11 which is greater than 10 authorized. rule
  8. CRITICAL P2PTunnelRegistry.java#L75: Define a constant instead of duplicating this literal "thermaldynamics" 15 times. rule
  9. CRITICAL P2PTunnelRegistry.java#L121: Define a constant instead of duplicating this literal "extrautilities" 3 times. rule
  10. CRITICAL P2PTunnelRegistry.java#L122: Define a constant instead of duplicating this literal "mekanism" 4 times. rule

orod-org commented Sep 21, 2017

SonarQube analysis reported 83 issues

  • BLOCKER 6 blocker
  • CRITICAL 13 critical
  • MAJOR 36 major
  • MINOR 15 minor
  • INFO 13 info

Watch the comments in this conversation to review them.

Top 10 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. BLOCKER CraftingCPUCluster.java#L466: Refactor this method to not always return the same value. rule
  2. BLOCKER CraftingCPUCluster.java#L847: Refactor this method to not always return the same value. rule
  3. BLOCKER CraftingCPUCluster.java#L961: Refactor this method to not always return the same value. rule
  4. BLOCKER SecurityStationInventory.java#L72: Refactor this method to not always return the same value. rule
  5. BLOCKER SecurityStationInventory.java#L89: Refactor this method to not always return the same value. rule
  6. BLOCKER Platform.java#L1402: Refactor this method to not always return the same value. rule
  7. CRITICAL AEBaseContainer.java#L223: The Cyclomatic Complexity of this method "setTargetStack" is 11 which is greater than 10 authorized. rule
  8. CRITICAL P2PTunnelRegistry.java#L75: Define a constant instead of duplicating this literal "thermaldynamics" 15 times. rule
  9. CRITICAL P2PTunnelRegistry.java#L121: Define a constant instead of duplicating this literal "extrautilities" 3 times. rule
  10. CRITICAL P2PTunnelRegistry.java#L122: Define a constant instead of duplicating this literal "mekanism" 4 times. rule

@yueh yueh added this to the rv5.alpha - 1.12 milestone Sep 29, 2017

@yueh yueh merged commit 1e15b23 into AppliedEnergistics:rv5-1.12 Sep 30, 2017

2 of 3 checks passed

sonarqube SonarQube reported 83 issues, with 13 critical and 6 blocker
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins Success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment