CamOps Contract Market - Contract Generation #4967
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4967 +/- ##
============================================
- Coverage 10.47% 10.45% -0.03%
- Complexity 6021 6025 +4
============================================
Files 951 953 +2
Lines 133248 133600 +352
Branches 19377 19409 +32
============================================
+ Hits 13961 13971 +10
- Misses 117940 118285 +345
+ Partials 1347 1344 -3 ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| private static AtBContractType getCovertMission(int roll, int margin) { |
Check notice
Code scanning / CodeQL
Useless parameter Note
|
I'm flipping this to ready for review. Much of the work for this phase is now done--the final things involve implementing negotiation of terms and extracting the payment formula out of |
Algebro7
changed the title
Algebro7
force-pushed
the
camops-contract-market
branch
from
cb929b2 to
f6d64c3
Compare
| @@ -473,7 +472,7 @@ public double calculatePaymentMultiplier(Campaign campaign, AtBContract contract | |||
| } | |||
| } | |||
|
|
|||
| multiplier *= contract.getContractType().getPaymentMultiplier(); | |||
| multiplier *= contract.getContractType().getOperationsTempoMultiplier(); | |||
There was a problem hiding this comment.
Thanks for including this. Will make implementing Fame & Infamy modifiers much easier.
There was a problem hiding this comment.
I've marked this as 'request changes' as I do have a couple points of feedback.
The switch comments may well come down to personal preference. I find switches a lot easier to read at a glance. So, if you disagree, feel free to disregard those points.
We definitely need to add the legal notices to the top of any files that are missing them -- this is one I need to get better about, too, which is the only reason I knew to check.
If possible, I would really like to see you adopt either a dialog notice or selectively color-coded (and bolded) text for the follow-up contract notice. It also probably should be in a ResourceDialog.
MekHQ/src/mekhq/campaign/market/contractMarket/AtbMonthlyContractMarket.java
Show resolved
Hide resolved
MekHQ/src/mekhq/campaign/market/contractMarket/CamOpsContractMarket.java
Show resolved
Hide resolved
MekHQ/src/mekhq/campaign/market/contractMarket/ContractTerms.java
Outdated
Show resolved
Hide resolved
MekHQ/src/mekhq/campaign/market/contractMarket/ContractTerms.java
Outdated
Show resolved
Hide resolved
MekHQ/src/mekhq/campaign/market/contractMarket/ContractTerms.java
Outdated
Show resolved
Hide resolved
MekHQ/src/mekhq/campaign/market/contractMarket/ContractTerms.java
Outdated
Show resolved
Hide resolved
|
@HammerGS I'm going to be away for a couple of days so won't be able to do a follow-up review until Monday. So long as the legal notices are added and we (at least) get some color coding on the follow-up notice, I'm happy for this to be treated as 'approved' for the purposes of merging. |
|
@IllianiCBT Thanks a lot for the review, those were all great points. I didn't know we had I believe I've addressed all of your points except for improvements on the legacy followup contract notification, which we can implement as a separate PR. |
|
Sorry for leaving you hanging, I completely forgot to approve this |
Opening this as a draft while I work on the next batch of functionality for the CamOps Contract Market. Any feedback welcome.