BLADEBURNER: Move bladeburner team losses to Casualties #1654

Alpheus · 2024-09-16T10:37:34Z

Motivation

I am moving as much of the non-essential logic outside of the 1500-line long Bladeburner file to ease gradual refactoring.

No external or NS API changes.

Coverage

test: coverage for casaulties when overlapping with sleeves refactor: Kill sleeves

test/jest/Bladeburner/BladeburnerCasualties.test.ts

src/Bladeburner/Actions/Casualties.ts

src/Bladeburner/Bladeburner.ts

src/Bladeburner/Actions/Casualties.ts

catloversg

I have to rewrite this entire feedback after rereading everything due to your force-push, so there may be some copy-paste errors. I put feedback into relevant parts of the changes, but there is a small concern for the overall code style. It's opinion-based, so feel free to ignore it.

I think some parts of the PR tend to become a bit overengineered over time. I can guess why they are like that, but I'm not sure if it's necessary. For example: the change of worstCase.

In the first implementation of this PR, it looked like this:

const worstCaseFraction = outcome === OperationCasualtyOutcome.LOW_CASUALTIES ? 0.5 : 1;
const worstCaseOp = Math[outcome === OperationCasualtyOutcome.LOW_CASUALTIES ? "ceil" : "floor"];
const worstCase = worstCaseOp(teamSizeUsedForAction * worstCaseFraction);

IMO, it's overengineered. The old implementation is just const maxLosses = success ? Math.ceil(teamCount / 2) : Math.floor(teamCount);. The new change is much harder to read than:

const worstCase = outcome === OperationCasualtyOutcome.LOW_CASUALTIES
  ? Math.ceil(teamSizeUsedForAction / 2)
  : Math.floor(teamSizeUsedForAction);

In the current implementation, it was put in TeamCasualties and this class's constructor even receives a random function as a parameter. Now, we can "customize" many things: CasualtyFactor, random function, etc. This flexibility is good for testing (no special mocking), but if we overuse it, the code becomes much harder to read. Please note that I'm not saying that your code is hard to read. My point is that we should maintain the balance between the flexibility (tons of options) and the simplicity.

With all being said, there is no need to change your code, though. It comes with proper tests, and the flexibility may bring more benefits than troubles.

src/Bladeburner/Actions/TeamCasualties.ts

catloversg · 2024-09-17T15:23:03Z

src/Bladeburner/Actions/TeamCasualties.ts

+    team.killSupportingSleeves(damagedSleeves);
+
+    team.teamSize -= humanDeaths;
+    team.teamLost += deaths;


The current implementation is wrong. It was reported, but nobody bothers fixing it. It's best to fix it here.

Currently, it's:

this.teamSize -= losses; if (this.teamSize < this.sleeveSize) { const sup = Player.sleeves.filter((x) => isSleeveSupportWork(x.currentWork)); for (let i = 0; i > this.teamSize - this.sleeveSize; i--) { const r = Math.floor(Math.random() * sup.length); sup[r].takeDamage(sup[r].hp.max); sup.splice(r, 1); } this.teamSize += this.sleeveSize; } this.teamLost += losses;

this.teamSize -= losses; is fine, but this.teamSize += this.sleeveSize is wrong.

Let's check this example: We have 2 team members (from recruitment), 4 supporting sleeves and we set team count to 2 when doing the operation. this.teamSize = 6. this.sleeveSize = 4. teamSizeUsed = 2.
If deaths = 2 (all team members die; 0 supporting sleeves take damage), this.teamSize should be 4 (0 supporting sleeves take damage; all supporting sleeves still support the main body), but:

In the current implementation, it's 8: this.teamSize - losses + this.sleeveSize = 6 - 2 + 4 = 8.

In your implementation, it's also 8: team.teamSize - Math.min(teamSizeUsed - supportingSleeves, deaths) = 6 - min(2 - 4, 2) = 6 - min(-2, 2) = 8.

The proper fix may be:

// Deduct the losses this.teamSize -= losses; // If this happens, all team members died and there are "damaged sleeves". In this case, teamSize = sleeveSize. if (this.teamSize < this.sleeveSize) { this.teamSize = this.sleeveSize; } team.teamLost += losses;

@d0sboots Please double-check this logic.

This PR is a refactor. I'm not doing any bugfixes and I'm testing any behavior as-is, including test-covering currently known bugs.

FWIW I agree with the stance of having this being refactor-only, and doing any bugfixes either before or after.

Side-note, If there's an open issue on this change, I'd love to reference it in the future PR so we can keep the discussions separate.

catloversg · 2024-09-17T15:24:08Z

src/Bladeburner/Bladeburner.ts

+
+  public killSupportingSleeves(sleeveDeaths: number) {
+    const sup = Player.sleeves.filter((x) => isSleeveSupportWork(x.currentWork));
+    const damagedIndices = [...Array(Math.min(sup.length, sleeveDeaths)).keys()].sort(() => 0.5 - Math.random());


This is just a nitpick.

If you want to shuffle an array, you should use Fisher–Yates shuffle, not this 0.5 - Math.random() trick. We have duplicated implementation of Fisher–Yates shuffle in our code, and we even import shuffle from lodash. If you want to use Fisher–Yates shuffle, you can use one of them. I'll clean them up later. If you think that it's unnecessary, you can ignore this comment. It's not a big deal in our use case. There are other problems with our randomization functions that I turn a blind eye to.

Can you show me a sample usage of our internal fisher-yates?

You can search the usages of shuffleArray in src\Infiltration\ui\BribeGame.tsx and shuffle in src\data\codingcontracttypes.ts.

Will do, thanks!

src/Bladeburner/Bladeburner.ts

test/jest/Bladeburner/TeamCasualties.test.ts

Alpheus · 2024-09-17T16:58:58Z

Cheers, simple fix. Anything else?

cmfrydos

I like rollOutcome now being overall more readable, but have some suggestions on how to further reorder things slightly.
This is just meant as an addition to catloversg's points, he made some good remarks on how to reuse shuffle functions and his note, that your implementation does not reproduce a known bug. I'd suggest keeping the bugged behaviour in your new implementation, and move fixing the bug, which should then be a one-liner, to another Pr.

Edit: It seems I missunderstood CatLoversG. I'm happy if your refactor reproduces the bug.
Maybe adding a failing testcase that catches the bug would be nice? (mark with .failing):

it.failing("should not spawn team members out of thin air") {...}

😉

cmfrydos · 2024-09-17T17:03:42Z

src/Bladeburner/Actions/TeamCasualties.ts

+export class TeamCasualties {
+  constructor(private severity: CasualtyFactor, private teamSizeUsed: number, private supportingSleeves: number) {}
+
+  rollOutcome(random: typeof getRandomIntInclusive, team: OperationTeam) {


I don't like this 'throwaway' class. If you'd like to parametrize, and then keep it this way, you can do so using a function returning a parametrized function. But you're using it as follows:

const casualties = new TeamCasualties(severity, action.teamCount, this.sleeveSize); return casualties.rollOutcome(getRandomIntInclusive, this);

casualities get's then thrown away the next time the garbage collector runs. The only thing that I can imagine that would somehow make sense to keep parametrized is random, the random number generator, to be able too keep the events deterministic. But that's currently not a thing in the whole codebase. Also, you are using another random function to shuffle the array, so having one as a parameter seems not consistent. What was your intent?

I'd recommend to drop the random parameter, and just use the stack for all 4 remaining arguments:
Make this a static function.

What I really like is the interface for the BladeBurner object (OperationTeam)!

I agree. This is a case of making the implementation awkward in order to try and do testing.

Note I say "try" because an important precept of unit testing is that you should be testing the public interfaces of units, not internal implementation details. This is because the internal details can change, via refactoring; trying to test the internal details is too granular, and also doesn't cover what is actually important, which is the public API. Sometimes you need to expose internal knobs in order to make mocks and make testing easier or possible, but that is different from testing the internals directly.

Regarding the static—I feel like I'm playing devil's advocate here but there isn't a clean solution for this. We have 2 components merging into data flow: the rolls (low, high, size, used), and the outcomes (randomGenerator, deaths, sleeves killed (specifically)).

If I make it static, then the function call has to be curried for it to have segmented return values. If I don't curry it, then the final return injects test-only data into the production code. The class was a sort of middle ground as d0s already commented on this, but I'm afraid I can't make both of you happy.

I disagree on the garbage collection aspect, this calculation is no different than using objects for actionId or returning [nextState, prevState] object arrays in corporations. The state object is a byproduct of the actionObject, which gets created once per action, with relevant bits cloned. This is one of those "cloned" bits.

The separation of the interfaces is specific to allow the random generator to be injected without mocking at test time as the randomness of things in Bladeburner are notoriously error-prone.

Hmm... I feel a little differently. My initial gut reaction is that we should just be mocking random(), providing a series of values, expecting them all to be used, and expecting a certain result from Bladeburner. This is what I meant by "you might need to poke some internal details to do testing, but you should test the external interface."

I agree with d0sboots about the mocking of Math.random. In its current state, the custom random function is only useful for testing while it reduces the readability. I don't think it's worth the trade-off.

If I make it static, then the function call has to be curried for it to have segmented return values. If I don't curry it, then the final return injects test-only data into the production code. The class was a sort of middle ground as d0s already commented on this, but I'm afraid I can't make both of you happy.

Here I cannot really follow. I would't curry here at all, have a simple function that returns deaths and a list / a count of all damaged sleeves. To test the function, just create an object that implements the Operation team interface , and mock random/ getRandomIntInclusive / shuffle.

What I mean is giving it the following signature:

static TeamCasualtiesRollOutcome(team: OperationTeam, severity: CasualtyFactor, teamSizeUsed: number, supportingSleeves: List<SleeveObject>) { ... return {deadPeople, deadSleeves } }

Or, to simplify things further, put teamSizeUsed and supportingSleeves inside the OperationTeam interface.
Maybe even make it a class, put the roll function in there, to stay OO. It would mean more refactoring work though, so this could be a separate pr.

That path already got rejected. And we did the mock version as well in the first two comits, the mock got rejected as well.

Look I appreciate what you guys are doing but it gets tricky when all of a sudden everyone passionately shows up and wants to chip in with sentimental nits or personal preferences.

I presented a working solution with a very small compromise. Non of your suggestions are compromise-free.

Ie. if I follow your suggestion, then the method goes from 3 parameters to 4, and to 5 if I hook in the random or the mock. If I replace that with an object because the method has too many parameters I end up here again.

I'll zoom out and consider an alternative approach.

That path already got rejected. And we did the mock version as well in the first two comits, the mock got rejected as well.

Sorry if that is the case, but I cannot find the rejection. Can you provide a link to that? I somehow lost the overview here, too many code reviews. I also struggle to read them, since somehow the code lines are messed up. I could imagine the force pushes overwrote the reviewed commit on the server.

Look I appreciate what you guys are doing but it gets tricky when all of a sudden everyone passionately shows up and wants to chip in with sentimental nits or personal preferences.

I've been trying to keep my comments down for this reason. XD

There's some stuff I like, and some stuff I don't, but I don't have a compromise-free solution to suggest either. I'll try to break some more time free later today and see if I can come up with a more positive proposal, as opposed to "I don't like some of this." :D

cmfrydos · 2024-09-17T17:12:29Z

src/Bladeburner/Bladeburner.ts

+  public killSupportingSleeves(sleeveDeaths: number) {
+    const sup = Player.sleeves.filter((x) => isSleeveSupportWork(x.currentWork));
+    const damagedIndices = [...Array(Math.min(sup.length, sleeveDeaths)).keys()].sort(() => 0.5 - Math.random());
+    damagedIndices.forEach((idx) => sup[idx].kill());
+  }


I would love to see this also excluded to TeamCasualities.ts
Maybe provide a wrapper to get the sleeves that are doing support work still from bladeburner, meaning keeping this 'bad' code out:

Player.sleeves.filter((x) => isSleeveSupportWork(x.currentWork));

and just provide SleeveTeamMembers or getSleeveTeamMembers to the TeamCasualities.rollOutcome function.

I hope the god singleton object Player will get refactored later to have some less responsibilities xD.

These two are tricky. I'm not sure if the heuristic of picking X random sleeves from the player should be that deep in casualties.

I agree that there should be a heuristic to highlight that the sleeves killed are random, ie. killRandomSleeves, not just killXSleevesInOrder, but for the implementation I'd rather keep in on BB or Player till that mess gets moved somewhere else (I have a feeling I'll be the one doing that anyway).

I meant that casualties should pick random sleeves from the team thats in the operation.
You already pick random people from that team in casualties. The only difference is, that they are just a number / annonym, while the sleeves are objects.

What's the problem with that? I may as well move this to SleevePerson or Person.

test/jest/Bladeburner/TeamCasualties.test.ts

d0sboots · 2024-09-17T17:43:08Z

I have to rewrite this entire feedback after rereading everything due to your force-push, so there may be some copy-paste errors.

I strongly agree with this part in particular. Please don't force-push (which comes from rebasing) during PRs, it messes up the review flow. There is no point in trying to make the individual commits "pretty"; everything will be squashed together into a single commit at the end anyway. It is better to think of the commit history of a PR branch as recording the history of evolution of thinking on the feature, because that is what it is.

(You can freely rebase as much as you want before publishing the PR, of course)

Alpheus · 2024-09-17T17:54:30Z

I like rollOutcome now being overall more readable, but have some suggestions on how to further reorder things slightly. This is just meant as an addition to catloversg's points, he made some good remarks on how to reuse shuffle functions and his note, that your implementation does not reproduce a known bug. I'd suggest keeping the bugged behaviour in your new implementation, and move fixing the bug, which should then be a one-liner, to another Pr.

Edit: It seems I missunderstood CatLoversG. I'm happy if your refactor reproduces the bug. Maybe adding a failing testcase that catches the bug would be nice? (mark with .failing):
it.failing("should not spawn team members out of thin air") {...}
😉

I will test cover the intended bugged behavior. it.failing involves double-negation and makes it confusing to understanding what the intended behavior is.

Alpheus · 2024-09-17T17:55:11Z

I have to rewrite this entire feedback after rereading everything due to your force-push, so there may be some copy-paste errors.

I strongly agree with this part in particular. Please don't force-push (which comes from rebasing) during PRs, it messes up the review flow. There is no point in trying to make the individual commits "pretty"; everything will be squashed together into a single commit at the end anyway. It is better to think of the commit history of a PR branch as recording the history of evolution of thinking on the feature, because that is what it is.

(You can freely rebase as much as you want before publishing the PR, of course)

Sorry, I'm used to stacked-diffing shortlived commits and it's muscle memory at this point. Will do my best in the future.

catloversg · 2024-09-17T17:55:40Z

I remember that I did that in some big PRs (save data compression, TS support, etc.). I feel guilty now.

d0sboots · 2024-09-17T17:52:03Z

src/Bladeburner/Actions/TeamCasualties.ts

+ * Caveats: TeamSize = Human Team + Supporting Sleeves set to this action
+ */
+export class TeamCasualties {
+  constructor(private severity: CasualtyFactor, private teamSizeUsed: number, private supportingSleeves: number) {}


I had to go look up parameter properties to understand what the heck was going on here.

I'm not a fan of this syntax. Almost everything in TypeScript does not affect the runtime semantics; you can trivially strip it out and the JS program remains unchanged. This is different; if you remove the "private"s here, it means something different. IMO we should explicitly use regular JS syntax here, for readability.

Is this a blocker or a nit?

Imho if we don't want this syntax we should automate the check in the linter. The JS target we are using will convert this to javascript in the form of private, non-enumerable properties. Natural JS syntax would involve using hashed properties to denote them private, which goes against the scoping in typescript (as you can make them public).

I checked the codebase and the only one other place constructor promotion is used is
../Casino/CardDeck/Deck.ts
8: constructor(private numDecks = 1) {

So since this is "rare" in this codebase, I'm happy to drop it in favor of an object parameter. Though we're redesigning this bit anyway by the looks of it.

d0sboots · 2024-09-17T17:58:43Z

src/Bladeburner/Actions/TeamCasualties.ts

+export class TeamCasualties {
+  constructor(private severity: CasualtyFactor, private teamSizeUsed: number, private supportingSleeves: number) {}
+
+  rollOutcome(random: typeof getRandomIntInclusive, team: OperationTeam) {


I agree. This is a case of making the implementation awkward in order to try and do testing.

Note I say "try" because an important precept of unit testing is that you should be testing the public interfaces of units, not internal implementation details. This is because the internal details can change, via refactoring; trying to test the internal details is too granular, and also doesn't cover what is actually important, which is the public API. Sometimes you need to expose internal knobs in order to make mocks and make testing easier or possible, but that is different from testing the internals directly.

d0sboots · 2024-09-17T18:02:06Z

Sorry, I'm used to stacked-diffing shortlived commits and it's muscle memory at this point. Will do my best in the future.

This also work out if you don't reflexively push all the time XD
(I can't stand to push "messy" commits either, and compromise by tidying up my work before pushing the latest commit)

d0sboots · 2024-09-17T18:16:05Z

I realized we should take a step back and probably agree on what we're trying to accomplish here.

IMO the goals and non-goals are: Goals:

Make bladeburner code easier to read.
Improve bladeburner test coverage.

Non-goals:

Split up bladeburner.ts.
- It is very possible (even likely) that this needs to happen in order to make the code easier to read, but it's not a goal in and of itself.
Increase testing coverage at the expense of code readability.
- Our overall test coverage is too poor for this to be a goal. There's so much low-hanging fruit in terms of testability, there's no reason to sacrifice readability here to get a little bit more in this particular area.

Alpheus · 2024-09-17T19:21:29Z

I realized we should take a step back and probably agree on what we're trying to accomplish here.

IMO the goals and non-goals are: Goals:

Make bladeburner code easier to read.

Improve bladeburner test coverage.

Non-goals:

Split up bladeburner.ts.

It is very possible (even likely) that this needs to happen in order to make the code easier to read, but it's not a goal in and of itself.

Increase testing coverage at the expense of code readability.

Our overall test coverage is too poor for this to be a goal. There's so much low-hanging fruit in terms of testability, there's no reason to sacrifice readability here to get a little bit more in this particular area.

This is very helpful.

I have a few PRs lined up that split out things like city changes and BB console (it's very isolated to begin with). Is that a reasonable goal for a different kind of PR or are these non-goals global for the foreseeable future?

cmfrydos · 2024-09-17T20:09:07Z

Goals:

* Make bladeburner code easier to read.
* Improve bladeburner test coverage.

I'd say there should be only the first goal:
Write programs for humans, not machines.
(With the exception of performance crititcal parts.)

I'd see test coverage separatly. Refactors, or new code, should be test-covered of course.
Also sometimes you need to refactor to make a part testable, or to fix bugs.
But like you said, it must not make the code harder to read.

Alpheus · 2024-09-17T20:12:11Z

Goals:
* Make bladeburner code easier to read.
* Improve bladeburner test coverage.
I'd say there should be only the first goal: Write programs for humans, not machines. (With the exception of performance crititcal parts.)

I'd see test coverage separatly. Refactors, or new code, should be test-covered of course. Also sometimes you need to refactor to make a part testable, or to fix bugs. But like you said, it must not make the code harder to read.

I disagree. We should strive to make it as readable as possible as long as it is testable. If we keep comparing PRs to the readability of untested code then we keep going back to statics and public-everything interfaces. Those are cute, but partly the maintenance burden stems from there being too large hubs of dependencies centered around the Bitnode processes. Those have to be split up. There is no "testable and more readable" without splitting up the largest files, of which Bladeburner is a heavy contender.

cmfrydos · 2024-09-17T20:16:10Z

it.failing("should not spawn team members out of thin air") {...}
I will test cover the intended bugged behavior. it.failing involves double-negation and makes it confusing to understanding what the intended behavior is.

I see that. You can use skip to show that a test currently fails, but should not fail.
The argument against it.skip would be, that it doesn't then prove that your refactor shows the same bugged behaviour.
I think that having it it.failing until the very last commit, where you change it to it.skip, could be the optimal approach.

Alpheus · 2024-09-17T20:19:34Z

it.failing("should not spawn team members out of thin air") {...}
I will test cover the intended bugged behavior. it.failing involves double-negation and makes it confusing to understanding what the intended behavior is.
I see that. You can use skip to show that a test currently fails, but should not fail. The argument against it.skip would be, that it doesn't then prove that your refactor shows the same bugged behaviour. I think that having it it.failing until the very last commit, where you change it to it.skip, could be the optimal approach.

this is totally out of scope.

cmfrydos · 2024-09-17T20:34:35Z

I'd say there should be only the first goal: Write programs for humans, not machines. (With the exception of performance crititcal parts.)
I'd see test coverage separatly. Refactors, or new code, should be test-covered of course. Also sometimes you need to refactor to make a part testable, or to fix bugs. But like you said, it must not make the code harder to read.

I disagree. We should strive to make it as readable as possible as long as it is testable. If we keep comparing PRs to the readability of untested code then we keep going back to statics and public-everything interfaces. Those are cute, but partly the maintenance burden stems from there being too large hubs of dependencies centered around the Bitnode processes. Those have to be split up. There is no "testable and more readable" without splitting up the largest files, of which Bladeburner is a heavy contender.

Sorry, but I don't follow. I believe we have the same goals, but you are throwing a few things around here; did you quote the right thing?
My opinion to splitting up bladeburner is that it naturally follows as soon as you identify logical sub-components of what it is doing. The team of an operation could be one of those things. It consists of multiple members in bladeburner that could be packed into a new class. This then automatically improves readability. Same with static functions: If they operate on a certain datatype, they should be part of a class. Also improves readability. Public-Everything makes it easier to write/sketch code, but not to read it. It's really helpful when you know which class members are private and thus internally managed, and not part of the "api"/"interface" it provides.

cmfrydos · 2024-09-17T20:52:05Z

this is totally out of scope.

I can see that this was quite a specific thing that doesn't belong to this conversation. I just noticed that you cannot really comment on comments, so next time I'll make this a separate review point, i.e. at this empty line were we could have another test.

Alpheus · 2024-09-17T21:12:51Z

I'd say there should be only the first goal: Write programs for humans, not machines. (With the exception of performance crititcal parts.)
I'd see test coverage separatly. Refactors, or new code, should be test-covered of course. Also sometimes you need to refactor to make a part testable, or to fix bugs. But like you said, it must not make the code harder to read.

I disagree. We should strive to make it as readable as possible as long as it is testable. If we keep comparing PRs to the readability of untested code then we keep going back to statics and public-everything interfaces. Those are cute, but partly the maintenance burden stems from there being too large hubs of dependencies centered around the Bitnode processes. Those have to be split up. There is no "testable and more readable" without splitting up the largest files, of which Bladeburner is a heavy contender.

Sorry, but I don't follow. I believe we have the same goals, but you are throwing a few things around here; did you quote the right thing? My opinion to splitting up bladeburner is that it naturally follows as soon as you identify logical sub-components of what it is doing. The team of an operation could be one of those things. It consists of multiple members in bladeburner that could be packed into a new class. This then automatically improves readability. Same with static functions: If they operate on a certain datatype, they should be part of a class. Also improves readability. Public-Everything makes it easier to write/sketch code, but not to read it. It's really helpful when you know which class members are private and thus internally managed, and not part of the "api"/"interface" it provides.

We don't need to pack the data in a class. For the sake of reviving these bitnode hubs from a savegame, the best place for data is in the core interfaces. That's 99% of what it does. Where we disagree is the seams. I try to separate out behavior, without impacting data flow too much. I don't care for static functions (on classes) as we have reasonable first-class function support.

I disagree on readability, we don't get better readability just by splitting things up. The frequency of change and cohesion must match the intent of the reader. This is very difficult.

For example, in this PR, most of what "is changing" is limited to 3 files which are very close together. That's intentional. Most of what we're doing here on these refactor PRs are temporary at best. We're refactoring semi-testless to be able to get into a minimal state where testing is sensible (ie. the tests don't require 20 mocks each). Only once we get there with a full control of all moving ports and interfaces, can we make larger architectural decisions.

Alpheus · 2024-09-21T18:34:22Z

I've got some time to work on this again.
Here's what I'm going to do:

I'm widening the testing surface to interface from Bladeburner.completeAction
I'll curry the casualties functionality, ditching the class and move them to BlackOperations / Operations
I'll inject the roll itself as a seed rather than injecting the random. From what I understand Math.random cannot be seeded and I'd rather not mock it. This is the simplest (least bad) alternative.
I'll "visit" the casualties check on the action itself, to inject only into the relevant subtypes (this is new).

refactor: functionise team casualties, move to blackops/ops

Alpheus · 2024-09-22T19:31:20Z

New update is up. It got somewhat larger unfortunately, but it did address most feedback so far.

Alpheus · 2024-09-22T19:49:34Z

src/Bladeburner/Bladeburner.ts

-          }
+        if (this.logging.blackops && deaths > 0) {
+          this.log(
+            `${person.whoAmI()}:  You lost ${formatNumberNoSuffix(deaths, 0)} team members during ${action.name}.`,


I always wondered... is this double-space intentional?

I'd assume it is, although if it bugs you I wouldn't mind if you changed it.

Honestly, I'd be OK with completely unifying the logging between the two versions and ironing out the minor differences in wording, to help factor out more stuff. But the 0 vs 1 discrepancy that Marvin brought up throws a big wrench into things. (OK, I guess it wasn't that big.)

By the looks of it we may have a second pass to go over things after this is merged. I'd rather get this one into dev and then do another smaller pass. The PR is getting too stale.

cmfrydos · 2024-09-23T00:29:12Z

Not a full review yet, but one of your comments made me remember a small detail:

/** Best case is always no deaths */

That's actually not the case. This is (the?) one constant where the two 'duplicates' differ:

bitburner-src/src/Bladeburner/Bladeburner.ts

Line 763 in ade92ed

const losses = getRandomIntInclusive(0, maxLosses);

bitburner-src/src/Bladeburner/Bladeburner.ts

Line 1046 in ade92ed

const losses = getRandomIntInclusive(1, teamLossMax);

I hope to find some time tomorrow to do a full review.

Alpheus · 2024-09-23T04:56:23Z

Not a full review yet, but one of your comments made me remember a small detail:

/** Best case is always no deaths */

That's actually not the case. This is (the?) one constant where the two 'duplicates' differ:

bitburner-src/src/Bladeburner/Bladeburner.ts

Line 763 in ade92ed

const losses = getRandomIntInclusive(0, maxLosses);

bitburner-src/src/Bladeburner/Bladeburner.ts

Line 1046 in ade92ed

const losses = getRandomIntInclusive(1, teamLossMax);

I hope to find some time tomorrow to do a full review.

Oh wow. I never spotted this. That's an easy fix, thanks!

Alpheus · 2024-09-23T05:08:44Z

Not a full review yet, but one of your comments made me remember a small detail:

/** Best case is always no deaths */

That's actually not the case. This is (the?) one constant where the two 'duplicates' differ:

bitburner-src/src/Bladeburner/Bladeburner.ts

Line 763 in ade92ed

const losses = getRandomIntInclusive(0, maxLosses);

bitburner-src/src/Bladeburner/Bladeburner.ts

Line 1046 in ade92ed

const losses = getRandomIntInclusive(1, teamLossMax);

I hope to find some time tomorrow to do a full review.

fixed.

d0sboots

I've got some nits, but overall I'm pretty happy with the flow of this. Sorry it took me so long to give it the full review it deserved.

d0sboots · 2024-09-23T04:47:15Z

src/Bladeburner/Bladeburner.ts

@@ -50,10 +49,12 @@ import { GeneralActions } from "./data/GeneralActions";
 import { PlayerObject } from "../PersonObjects/Player/PlayerObject";
 import { Sleeve } from "../PersonObjects/Sleeve/Sleeve";
 import { autoCompleteTypeShorthand } from "./utils/terminalShorthands";
+import type { OperationTeam } from "./Actions/TeamCasualties";
+import { shuffleArray } from "../Infiltration/ui/BribeGame";


This probably should go into its own helper, but that can also wait for later if you want.

Agreed. I'd prefer to not touch non-BB refactors at the moment. Next pass?

d0sboots · 2024-09-23T04:49:22Z

src/Bladeburner/Bladeburner.ts

-          }
+        if (this.logging.blackops && deaths > 0) {
+          this.log(
+            `${person.whoAmI()}:  You lost ${formatNumberNoSuffix(deaths, 0)} team members during ${action.name}.`,


I'd assume it is, although if it bugs you I wouldn't mind if you changed it.

Honestly, I'd be OK with completely unifying the logging between the two versions and ironing out the minor differences in wording, to help factor out more stuff. But the 0 vs 1 discrepancy that Marvin brought up throws a big wrench into things. (OK, I guess it wasn't that big.)

src/Bladeburner/Actions/TeamCasualties.ts

d0sboots · 2024-09-23T05:07:11Z

src/Bladeburner/Actions/BlackOperation.ts

+  getTeamCasualtiesRoll = getRandomIntInclusive;
+
+  resolveTeamCasualties = resolveTeamCasualties;


This is more interface complexity that exists only for testing.

getTeamCasualtiesRoll obfuscates the intent a fair amount (it gets plumbed through a class and an interface to ultimately be a simple call to getRandomIntInclusive. However, this is balanced by the fact that it would be a bit trickier to mock around without it.

AFAICT, resolveTeamCasualties isn't even used as a test injection point, and is thus completely unneeded. If you got rid of both of these variables, you could also completely remove the TeamActionWithCasualties interface and resolveTeamCasualties could be a simple function that takes teamCount as a number instead of semi-awkwardly through this. It would also be just as straightforward to mock, if you ever needed/wanted to in the future.

EDIT: Just saw your change to fix the 0/1 issue. I guess the interface has a stay on execution. XD You could pass that in as another free parameter to the function, but that's getting more ugly... it tips the balance back towards it being worth it to have an interface. I still don't think you need the instance variable for the function here, though. It can just be a free function that takes a TeamActionWithCasualties as first parameter. (Normally you would just put it on the base class, but that doesn't work here when you don't have a base class for just this pair of classes.)

AFAICT, resolveTeamCasualties isn't even used as a test injection point, and is thus completely unneeded. If you got rid of both of these variables, you could also completely remove the TeamActionWithCasualties interface and resolveTeamCasualties could be a simple function that takes teamCount as a number instead of semi-awkwardly through this. It would also be just as straightforward to mock, if you ever needed/wanted to in the future.

It won't work. It needs teamCount, sleeveSize and teamSize (not to be confused with teamCount). I tried it and this is the minimal complexity I've been able to scale it down to. It's so easy to confuse teamCount and teamSize and I wanted it to be handled explicitly. A narrower type (the interface) but generously wide instance (Bladeburner, Action) to make it user-friendly on the developer side who's working on CompleteAction.

I tried mocking it. But ended up with a lot more complexity (and automocking on the module level which made the tests much slower).

However, if you insist I could make it a public module export on TeamCasualties and then mock that, but then the tests get exposed to much more "implementation detail" that isn't captured by the interface. It's a hard balance. Anything touching entropy needs to be controlled.

Okay, a bit more context
I need to be able to control the random range to test it properly. If we don't want this interface shenanigans, then I'd rather throw away the tests that are flaky when random. But that means we lose 2/3rds of the tests. The issue isn't Math.random or something similar. The problem is that our numbers cause the rolls to be exhaustive at random. It's exhaustive if casualties are 0% or 100%, and the ranges can randomly range to 50%-100% or 0%-100% depending on what the attempt roll was. This makes it incredibly difficult to test, even with monte-carlo simulations. I either need control over an non-exhaustive range or a forced scope to the roll (MIN,MAX). I opted for the latter, but it requires one injection point in production scope.

Now, I'm aware you may consider injection scope on an interface more invasive than on a package module, but from the perspective of production code, there will be a public function somewhere to roll the range (either exported, or on a class/interface). There is no way avoiding having it be "mentioned" in the final output. If this is a huge blocker, I'd much rather remove the tests, than try to pretend we're not affecting public scope. As I mentioned in the earlier round of reviews, this is one of those problems that cannot be without compromise. That's the nature of random elements.

Edit: As a compromise if you want I can put the roll on Bladeburner instead (on the OperationTeam interface). That way it requires less care on the Action data side.

OK, as you said it's a compromise but I like this compromise better.

However, I feel like you didn't see the other part of my comment? Specifically, regarding the resolveTeamCasualties function itself, that it doesn't need to be part of the interface but can be a free function that takes an interface object as a param. And that should be just as easy to mock (you can mock out the free function in the tests, if needed.)

I misunderstood. I thought you meant the resolve as being part of the interface, not the function declaration being referenced and obsolete after the 0/1 fixed gave legit purpose for the namespace. Let me re-read.

I did comment on it, quoted below your original. You mean to have it be a global function? Doesn't that make the invocations more verbose?

Oh, you mean it can stay on the class, but not be part of the interface? Yeah it seems we lost that structural need somewhere during refactoring. I'll try it, see how it looks.

Edit: Yeah, that's completely benign. Done!

I still see you referring to an object with an interface but I'm not quite sure what that's highlighting. Could you give me a quick example or code pin?

I think I got what you meant, let me know!

Yup, that was what I was talking about!

src/Bladeburner/Bladeburner.ts

refactor: tyidings, removed dead variables in casualties return value

…ated

…ficial#1654)

refactor: Move bladeburner team losses to Mission Casualties

8336961

test: coverage for casaulties when overlapping with sleeves refactor: Kill sleeves

Alpheus changed the title ~~BLADEBURNER: Move bladeburner team losses to Mission Casualties~~ BLADEBURNER: Move bladeburner team losses to Casualties Sep 16, 2024

d0sboots reviewed Sep 17, 2024

View reviewed changes

Alpheus added 2 commits September 17, 2024 13:25

fix: simplified flow, removed dead types

549ee37

fix: use numbered enum for casualty factor instead of boolean choices

22c2bf0

Alpheus force-pushed the BB-refactor-team-losses branch from 8474d80 to df3f151 Compare September 17, 2024 11:31

chore: zero check is unnecessary, already checked for

0ade0db

Alpheus force-pushed the BB-refactor-team-losses branch from df3f151 to 0ade0db Compare September 17, 2024 11:32

catloversg reviewed Sep 17, 2024

View reviewed changes

cmfrydos suggested changes Sep 17, 2024

View reviewed changes

d0sboots reviewed Sep 17, 2024

View reviewed changes

This was referenced Sep 19, 2024

BUGFIX: Wrong team size when all team members die in Bladeburner's action #1658

Merged

BUGFIX: Follow-up of #1658 (Wrong team size in Bladeburner) #1662

Merged

refactor: widen surface area on tests to Bladeburner.completeAction

4ebfdc3

refactor: functionise team casualties, move to blackops/ops

Merge branch 'dev' into BB-refactor-team-losses

dffaf95

Alpheus commented Sep 22, 2024

View reviewed changes

Merge branch 'dev' into BB-refactor-team-losses

5a3ba3a

fix: black ops and ops differ in mininum casualties

435b6f9

d0sboots reviewed Sep 23, 2024

View reviewed changes

Alpheus added 6 commits September 23, 2024 11:49

fix: Team rolls casualties instead of Action

f1e4f5b

refactor: tyidings, removed dead variables in casualties return value

fix: linter

d7e988a

refactor: needless scoping qualifier

926ca15

refactor: superflous interface member. Subtypes are already discrimin…

4e70585

…ated

refactor: split resolve from interface, invoke without 'this' binding

0a3a453

lint: dead import

6f5c191

d0sboots merged commit b86044b into bitburner-official:dev Sep 24, 2024
5 checks passed

Alpheus deleted the BB-refactor-team-losses branch September 24, 2024 06:02

antoinedube pushed a commit to antoinedube/bitburner-source that referenced this pull request Sep 27, 2024

BLADEBURNER: Move bladeburner team losses to Casualties (bitburner-of…

875b7e3

…ficial#1654)

catloversg mentioned this pull request Sep 30, 2024

BUGFIX: Wrong calculation in team casualties of Bladeburner action #1672

Merged

		getTeamCasualtiesRoll = getRandomIntInclusive;

		resolveTeamCasualties = resolveTeamCasualties;

BLADEBURNER: Move bladeburner team losses to Casualties #1654

BLADEBURNER: Move bladeburner team losses to Casualties #1654

Conversation

Alpheus commented Sep 16, 2024

Motivation

Coverage

catloversg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alpheus commented Sep 17, 2024

cmfrydos left a comment • edited Loading

Choose a reason for hiding this comment

cmfrydos Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmfrydos Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmfrydos Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d0sboots commented Sep 17, 2024 • edited Loading

Alpheus commented Sep 17, 2024

Alpheus commented Sep 17, 2024

catloversg commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d0sboots commented Sep 17, 2024

d0sboots commented Sep 17, 2024

Alpheus commented Sep 17, 2024

cmfrydos commented Sep 17, 2024

Alpheus commented Sep 17, 2024

cmfrydos commented Sep 17, 2024

Alpheus commented Sep 17, 2024

cmfrydos commented Sep 17, 2024

cmfrydos commented Sep 17, 2024

Alpheus commented Sep 17, 2024

Alpheus commented Sep 21, 2024

Alpheus commented Sep 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmfrydos commented Sep 23, 2024 • edited Loading

Alpheus commented Sep 23, 2024

Alpheus commented Sep 23, 2024

d0sboots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alpheus Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmfrydos left a comment •

edited

Loading

cmfrydos Sep 17, 2024 •

edited

Loading

cmfrydos Sep 17, 2024 •

edited

Loading

cmfrydos Sep 17, 2024 •

edited

Loading

d0sboots commented Sep 17, 2024 •

edited

Loading

cmfrydos commented Sep 23, 2024 •

edited

Loading

Alpheus Sep 23, 2024 •

edited

Loading