-
-
Notifications
You must be signed in to change notification settings - Fork 72
Add : Sum upgrade system #1476
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
base: master
Are you sure you want to change the base?
Add : Sum upgrade system #1476
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1476 +/- ##
==========================================
+ Coverage 35.43% 36.01% +0.58%
==========================================
Files 240 244 +4
Lines 7372 7449 +77
Branches 561 568 +7
==========================================
+ Hits 2612 2683 +71
- Misses 4646 4652 +6
Partials 114 114
Continue to review full report at Codecov.
|
|
|
||
| InventoryItemInstance? RemoveItemAmountFromInventory(short amount, Guid id); | ||
|
|
||
| List<InventoryItemInstance?> RemoveItemAmountFromInventory(short amount, short vnum); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don’t need that if you have the load by vnum and amount
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| // GNU General Public License for more details. | ||
| // | ||
| // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All those comment change make it harder to review
| return retItem; | ||
| } | ||
|
|
||
| public List<InventoryItemInstance?> LoadByVNumAndAmount(short vnum, short amount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we need the amount getting all the item for a vnum is perfectly fine
| amount -= item.ItemInstance!.Amount; | ||
| if (amount <= 0) | ||
| { | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return instead of break
|
|
||
| public List<InventoryItemInstance?> RemoveItemAmountFromInventory(short amount, short vnum) | ||
| { | ||
| var result = new List<InventoryItemInstance?>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole thing is useless
| case EquipmentType.SecondaryWeapon: | ||
| wearable.SetRarityPoint(); | ||
| break; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like all those thing would be better in the item creation service not sure why I put it here. Can be moved to replace the other thing using polymorphism over switch will be better
| var targetSlot = session.Character.InventoryService.LoadBySlotAndType((byte)upgradePacket.Slot2, (NoscorePocketType)upgradePacket.InventoryType); | ||
|
|
||
| await session.SendPacketsAsync(await _sumUpgradeService.SumItemInstanceAsync(session, sourceSlot, targetSlot)); | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to log default
|
|
||
| //TODO RemoveItemAmountFromInventory | ||
| [TestMethod] | ||
| public void RemoveItemAmountFromInventoryByGuidWithoutRemove() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming could be bettrr
| Assert.IsTrue((destinationItem?.ItemInstance?.Amount == 999) && (destinationItem.Slot == 1)); | ||
| } | ||
|
|
||
| //TODO RemoveItemAmountFromInventory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You remove todo but it was here for the other one not for yours 😂 still need to be tested. Moreover yours will disapear for the load one
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task Test_UpgradePacketSum1Async() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to test glove with boots, sum > 6, sum of different resistance
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1476 +/- ##
==========================================
+ Coverage 35.43% 36.01% +0.58%
==========================================
Files 240 244 +4
Lines 7372 7449 +77
Branches 561 568 +7
==========================================
+ Hits 2612 2683 +71
- Misses 4646 4652 +6
Partials 114 114 ☔ View full report in Codecov by Sentry. |
No description provided.