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

Refactored the BaseActionSource #3063

Merged
merged 8 commits into from Sep 12, 2017

Conversation

Projects
None yet
4 participants
@yueh
Member

yueh commented Aug 31, 2017

It now uses an interface instead of a concrete class and further subclasses.

Instead of relying on a specific class for a certain action type, it now uses methods with Optional as return values to determine a player or machine issuing an action.
Refer to the JavaDocs for the exact behaviour.

IActionHost no longer extends IGridHost. It never used the additional functionality and if needed the IGridNode will also provide a reference to the corresponding IGridHost.

Refactored the BaseActionSource
It now uses an interface instead of a concrete class and further
subclasses.

Instead of relying on a specific class for a certain action type, it now
uses methods with Optional as return values to determine a player or
machine issuing an action. Refer to the JavaDocs for the exact behaviour.

IActionHost no longer extends IGridHost. It never used the additional
functionality and if needed the IGridNode will also provide a reference
to the corresponding IGridHost.

@yueh yueh added this to the rv5.alpha - 1.12 milestone Aug 31, 2017

}
else if( this.actionSrc.machine().isPresent() )
{
final IActionHost machineSource = this.actionSrc.machine().get();

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@@ -103,7 +103,7 @@ public IAEItemStack injectItems( final IAEItemStack input, final Actionable type
}
@Override
public IAEItemStack extractItems( final IAEItemStack request, final Actionable type, final BaseActionSource src )
public IAEItemStack extractItems( final IAEItemStack request, final Actionable type, final IActionSource src )
{
ItemStack out = ItemStack.EMPTY;

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

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

@orod-org

orod-org Aug 31, 2017

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

@@ -73,7 +73,7 @@ public void removeListener( final IMEMonitorHandlerReceiver<IAEItemStack> l )
}
@Override
public IAEItemStack injectItems( final IAEItemStack input, final Actionable type, final BaseActionSource src )
public IAEItemStack injectItems( final IAEItemStack input, final Actionable type, final IActionSource src )
{
ItemStack out = ItemStack.EMPTY;

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

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

@orod-org

orod-org Aug 31, 2017

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

@@ -48,7 +47,7 @@ public SecurityStationInventory( final TileSecurityStation ts )
}
@Override
public IAEItemStack injectItems( final IAEItemStack input, final Actionable type, final BaseActionSource src )
public IAEItemStack injectItems( final IAEItemStack input, final Actionable type, final IActionSource src )
{
if( this.hasPermission( src ) )

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

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

@orod-org

orod-org Aug 31, 2017

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

@@ -70,13 +69,13 @@ public IAEItemStack injectItems( final IAEItemStack input, final Actionable type
return input;
}
private boolean hasPermission( final BaseActionSource src )
private boolean hasPermission( final IActionSource src )

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

BLOCKER Refactor this method to not always return the same value. rule

@orod-org

orod-org Aug 31, 2017

BLOCKER Refactor this method to not always return the same value. rule

@@ -87,7 +86,7 @@ private boolean hasPermission( final BaseActionSource src )
}
@Override
public IAEItemStack extractItems( final IAEItemStack request, final Actionable mode, final BaseActionSource src )
public IAEItemStack extractItems( final IAEItemStack request, final Actionable mode, final IActionSource src )

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

BLOCKER Refactor this method to not always return the same value. rule

@orod-org

orod-org Aug 31, 2017

BLOCKER Refactor this method to not always return the same value. rule

@@ -87,7 +86,7 @@ private boolean hasPermission( final BaseActionSource src )
}
@Override
public IAEItemStack extractItems( final IAEItemStack request, final Actionable mode, final BaseActionSource src )
public IAEItemStack extractItems( final IAEItemStack request, final Actionable mode, final IActionSource src )
{
if( this.hasPermission( src ) )

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

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

@orod-org

orod-org Aug 31, 2017

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

{
try
{
return this.securityTile.getProxy().getSecurity().hasPermission( ( (PlayerSource) src ).player, SecurityPermissions.SECURITY );
return this.securityTile.getProxy().getSecurity().hasPermission( src.player().get(), SecurityPermissions.SECURITY );

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@@ -469,7 +469,7 @@ private TileCraftingTile getCore()
{
return null;
}
return (TileCraftingTile) this.machineSrc.via;
return (TileCraftingTile) this.machineSrc.machine().get();

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@@ -844,7 +844,7 @@ private void storeItems()
this.markDirty();
}
public ICraftingLink submitJob( final IGrid g, final ICraftingJob job, final BaseActionSource src, final ICraftingRequester requestingMachine )
public ICraftingLink submitJob( final IGrid g, final ICraftingJob job, final IActionSource src, final ICraftingRequester requestingMachine )

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

BLOCKER Refactor this method to not always return the same value. rule

@orod-org

orod-org Aug 31, 2017

BLOCKER Refactor this method to not always return the same value. rule

{
return gridProxy.getSecurity().hasPermission( ( (PlayerSource) src ).player, SecurityPermissions.BUILD );
return gridProxy.getSecurity().hasPermission( src.player().get(), SecurityPermissions.BUILD );

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

{
final IActionHost te = ( (MachineSource) src ).via;
final IActionHost te = src.machine().get();

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@@ -1431,17 +1431,17 @@ public static void configurePlayer( final EntityPlayer player, final AEPartLocat
player.rotationYaw = player.prevCameraYaw = player.cameraYaw = yaw;
}
public static boolean canAccess( final AENetworkProxy gridProxy, final BaseActionSource src )
public static boolean canAccess( final AENetworkProxy gridProxy, final IActionSource src )

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

BLOCKER Refactor this method to not always return the same value. rule

@orod-org

orod-org Aug 31, 2017

BLOCKER Refactor this method to not always return the same value. rule

{
if( this.security.isAvailable() )
{
final IGridNode n = ( (MachineSource) src ).via.getActionableNode();
final IGridNode n = src.machine().get().getActionableNode();

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@@ -137,20 +135,20 @@ private boolean diveList( final NetworkInventoryHandler<T> networkInventoryHandl
return false;
}
private boolean testPermission( final BaseActionSource src, final SecurityPermissions permission )
private boolean testPermission( final IActionSource src, final SecurityPermissions permission )

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

BLOCKER Refactor this method to not always return the same value. rule

@orod-org

orod-org Aug 31, 2017

BLOCKER Refactor this method to not always return the same value. rule

{
if( !this.security.hasPermission( ( (PlayerSource) src ).player, permission ) )
if( !this.security.hasPermission( src.player().get(), permission ) )

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

{
final IActionHost machineSource = ( (MachineSource) this.actionSrc ).via;
final EntityPlayer player = this.actionSrc.player().get();

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

@orod-org

orod-org Aug 31, 2017

MAJOR Call "Optional#isPresent()" before accessing the value. rule

{
final IGridNode node = ( (IGridHost) monitorable ).getGridNode( AEPartLocation.INTERNAL );
final IGridNode node;
if( monitorable instanceof IGridHost )

This comment has been minimized.

@orod-org

orod-org Aug 31, 2017

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

@orod-org

orod-org Aug 31, 2017

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

{
@Override
public int compareTo( Integer o )

This comment has been minimized.

@orod-org

orod-org Sep 2, 2017

MINOR Override "equals(Object obj)" to comply with the contract of the "compareTo(T o)" method. rule

@orod-org

orod-org Sep 2, 2017

MINOR Override "equals(Object obj)" to comply with the contract of the "compareTo(T o)" method. rule

yueh added some commits Sep 6, 2017

Fixes crafting guis not opening.
Due to being hardcoded to IGridHost, they no longer work when
IActionHost is not extending it. Actually IActionHost is the better
solution for it, as it prevents us from looking  the grid up via the
IGridHost and potentially finding a wrong grid
Added a flag to interfaces to tick once more after inv operations.
This ensures it ticks once more after an interface extracts from an
already worked slot of their own inventory before finally going to
sleep.
@orod-org

This comment has been minimized.

Show comment
Hide comment
@orod-org

orod-org Sep 9, 2017

SonarQube analysis reported 59 issues

  • BLOCKER 7 blocker
  • CRITICAL 3 critical
  • MAJOR 36 major
  • MINOR 11 minor
  • INFO 2 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#L961: Refactor this method to not always return the same value. rule
  3. CRITICAL AEBaseContainer.java#L223: The Cyclomatic Complexity of this method "setTargetStack" is 11 which is greater than 10 authorized. rule
  4. CRITICAL GrinderRecipeCategory.java#L91: Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation. rule
  5. MAJOR CraftingJob.java#L114: Remove this unused method parameter "details". rule
  6. MAJOR CraftingTreeProcess.java#L97: Remove this call to "equals"; comparisons between unrelated types always return false. rule
  7. MAJOR CraftingTreeProcess.java#L154: Remove this call to "equals"; comparisons between unrelated types always return false. rule
  8. MAJOR DualityInterface.java#L541: Remove this unused method parameter "dir". rule
  9. MAJOR DualityInterface.java#L1258: Remove this unused method parameter "facing". rule
  10. MAJOR DualityInterface.java#L1264: Remove this unused method parameter "facing". rule

orod-org commented Sep 9, 2017

SonarQube analysis reported 59 issues

  • BLOCKER 7 blocker
  • CRITICAL 3 critical
  • MAJOR 36 major
  • MINOR 11 minor
  • INFO 2 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#L961: Refactor this method to not always return the same value. rule
  3. CRITICAL AEBaseContainer.java#L223: The Cyclomatic Complexity of this method "setTargetStack" is 11 which is greater than 10 authorized. rule
  4. CRITICAL GrinderRecipeCategory.java#L91: Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation. rule
  5. MAJOR CraftingJob.java#L114: Remove this unused method parameter "details". rule
  6. MAJOR CraftingTreeProcess.java#L97: Remove this call to "equals"; comparisons between unrelated types always return false. rule
  7. MAJOR CraftingTreeProcess.java#L154: Remove this call to "equals"; comparisons between unrelated types always return false. rule
  8. MAJOR DualityInterface.java#L541: Remove this unused method parameter "dir". rule
  9. MAJOR DualityInterface.java#L1258: Remove this unused method parameter "facing". rule
  10. MAJOR DualityInterface.java#L1264: Remove this unused method parameter "facing". rule
@fscan

This comment has been minimized.

Show comment
Hide comment
@fscan

fscan Sep 12, 2017

Member

The last two commits, are they applying to 1.10 too? Asking because of #3085

Member

fscan commented Sep 12, 2017

The last two commits, are they applying to 1.10 too? Asking because of #3085

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Sep 12, 2017

Member

Currently no, but mostly because the changes are still a bit experimental and open up one or two code paths, which could create issues in some very rare cases. If they will more likely happen in an established modpack, thus it is not a good idea should we have to revert a stable version because of it.

If there was more testing done/time passed without issues showing, it might be an idea to backport it. But then it is always the question of 1.10 being still relevant at that point.

Member

yueh commented Sep 12, 2017

Currently no, but mostly because the changes are still a bit experimental and open up one or two code paths, which could create issues in some very rare cases. If they will more likely happen in an established modpack, thus it is not a good idea should we have to revert a stable version because of it.

If there was more testing done/time passed without issues showing, it might be an idea to backport it. But then it is always the question of 1.10 being still relevant at that point.

@yueh yueh merged commit 970630a into rv5-1.12 Sep 12, 2017

3 of 4 checks passed

sonarqube SonarQube reported 59 issues, with 3 critical and 7 blocker
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
jenkins Success
Details
@Quest79

This comment has been minimized.

Show comment
Hide comment
@Quest79

Quest79 Sep 12, 2017

Hi yueh, thank you for considering 1.10 for this fix.
Please consider how long 1.7.10 was relevant, many tens of thousands of people still play it. 1.10 i believe has only recently finally 'matured' mod communitywise so I don't think it will become irrelevant for a while (3-4 years if history is any indication), so a lot of people would appreciate any work you put into the 1.10.2 pool.
Thanks again for your work, one of my favorite mods.

Quest79 commented Sep 12, 2017

Hi yueh, thank you for considering 1.10 for this fix.
Please consider how long 1.7.10 was relevant, many tens of thousands of people still play it. 1.10 i believe has only recently finally 'matured' mod communitywise so I don't think it will become irrelevant for a while (3-4 years if history is any indication), so a lot of people would appreciate any work you put into the 1.10.2 pool.
Thanks again for your work, one of my favorite mods.

@yueh yueh deleted the feature-baseactionsource-refactoring branch Oct 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment