I took a long look at this, because I'm not super familiar with the Bladeburner code. When I finished, this was my conclusion:

I think this PR is going in the opposite direction of how we should address this.

Specifically, Person.gainStats is only used by this one place in the entire codebase. Also, as the logging demonstrates, it is already awkward to create this large return object in order to defer multiplication of all these values. All of this is unnecessary. If we let completeAction adjust the stats itself, it never needs to add them to any return object (saving lots of boilerplate lines), we can get rid of a whole useless helper function, and the logging becomes simple and clear. The code also more directly expresses its intent.

The same thing applies with stamina_bonus: I feel like we should be adding the multiplier directly to the bonus, as opposed to multiplying it in later. This makes logging and everything much more clear.

I'm happy with the proposed major change to revert completeAction to its original layout, prior to the introduction of Sleeves. However, this raises the issue of how Sleeves should receive their rewards, as this mechanic differs from how the player gains XP.

To address this, I suggest making Person.gainStats a virtual method returning a dictionary {person : effective stats} that has been applied, and rename it to Person.gainBaseStats. We could then move the call to applySleeveGains from SleeveBladeburnerWork.process to the new Sleeve.gainBaseStats. However, Sleeve.gainBaseStats would need to divide by its XP multipliers to maintain the current gameplay balance, which could get messy.

One option to solve this, is to move the multipliers back to player.gainBaseStats (so, leave them where they are now), but this would reintroduce the logging issue that Alpheus mentioned. If gainBaseStats then returns the effective stats that have been applied (for logging purposes), this problem would be resolved.

(Note: I’d argue that having Sleeves log their XP gains to the Bladeburner console is not well thought out, since they effectively apply stats to all Sleeves and the Player.)

Concerning Stamina: Adding the multiplier directly in completeAction would alter the CyberEdge effect. Instead of increasing max stamina, it would increase the max stamina gain rate from training, which would impact gameplay slightly. Introducing Bladeburner.gainBaseMaxStamina, returning the effective gain, could be an improvement over the current situation.

So, yeah, it's complicated. If desired, I’d be happy to create a new pull request / branch to demonstrate this solution to this issue without altering the current balance.

Hmm, I see now, I hadn't realized the sleeves issue. That does make things more complicated, I'll have to think on it.

To some degree this has been addresses for work in src/Work/WorkStats and Sleeves/Work. It also goes the extra mile to handle shock correctly.

Venusaur just commented on discord:

Is there way to know how much experience I would gain for specific bladeburner action?

Which currently there is Not. Formulas.exe doesn't support BB.

This leads me to suggest an even more drastic "change," while keeping the current gameplay balance intact: Why not completely refactor completeAction / Bladeburner.ts into a more consistent approach? We could encapsulate all data from General, Contract, Operation, and Black Operation into "data files" with consistent typing. We could also write neat functions that access specific information, like getEXPFromAction, getRankGain, getHPChange, getStaminaChange, getMaxStaminaChange, getBounty, getEstimationChange, etc., bringing all current functionality together under one roof. This is in contrast to the quick "appendix" of some features, like when Sleeves were added.

I currently have some free time and would love to work on a refactor. However, I must be honest: without extensive testing, there's a possibility that this could introduce subtle gameplay changes due to certain quirks or undiscovered aspects of the existing code.

Go for it, I'll help you refactor/write tests for it.

Sounds good to me, a formulas namespace was just added for blade but it's very small right now.

Oh, also I strongly encourage you to do it one function/action at a time, instead of one giant PR that changes all of bladeburner.

Ok, I'm mostly done with the refactor of bladeburner.completeAction, and I am quite happy about the new semantics. All effects of an Action are now split up, which makes it rather easy to expand Formula.exe to BladeBurner. Also there is no more special treatment in the code for whichever type an action has (General, Contracts, Operation, BlackOps), and all current Bugs are more clearly written out to understand them more easily. I'm not that happy about the syntax of the solution though, since it wants to be an enity-component system (with actions being the entities, the effects like HPChange, StatChange, ..., the components), without being a real one. I'm not so sure though whether creating such a system (with a separate small classes for each and every component), is the way to go here.

I'll just append the new function here for a moment, maybe you can give your opinion about it. @Alpheus @d0sboots
In the meantime, I'll start writing tests, so that we can assure with high probability, that a refactor won't introduce any new bugs.

Offtopic: Speaking of probability, when thinking about an upcoming Formula.exe expansion, I noticed that many Bladeburner actions are luck based. Not just their success chance, which would be easy to pass to the player, but also when things happen, they often happen following a random distrubution. i.e when a Bounty Contract fails, the player takes some random amount of damage.
If we now want to create a Formula.GetActionHPChange(), then it would actually need to return this distribution instead of a real number. I'm not sure if this may be too advanced for many players though. A simpler solution would be, that those Formula Functions just sample the distributions, maybe optionally multiple times, so that the player only get a "measured" value and needs to decide, what to do with it. Just as a thought for yet another problem to solve

Here is the current code for the new Bladeburner.CompleteAction:

  completeAction(person: Person, actionIdent: ActionIdentifier) {
    const loggingConstants = {
      General: this.logging.general,
      "Black Operations": this.logging.blackops,
      Operations: this.logging.ops,
      Contracts: this.logging.contracts,
    };

    const action = this.getActionObject(actionIdent);
    try {
      const success = action.attempt(this, person);
      const effect = success ? action.combinedSuccessEffect : action.combinedFailureEffect;

      // Money gain
      const earnings = effect.earnings(action) * this.getSkillMult(BladeburnerMultName.Money);
      Player.gainMoney(earnings, person instanceof PlayerObject ? "bladeburner" : "sleeves");
      // Stamina change
      const previousStamina = this.stamina;
      this.stamina = clampNumber(
        0,
        this.stamina + effect.staminaChange(this.maxStamina, action, person),
        this.maxStamina,
      );
      // Stats change
      let statChange = effect.statChange(action, this.getSkillMult(BladeburnerMultName.ExpGain));
      // this if shouldn't exist, since it is a bug #1607 (duplicate application of exp mults)
      if (effect.statChangeExtraMultsMultiply) {
        statChange = multWorkStats(statChange, multToWorkStats(person.mults));
      }
      if (effect.statChangeTimeMultiply) {
        var actionTotalSeconds = action.getActionTotalSeconds(this, person);
        // This line shouldn't exist, since it reintroduced bug #295 again
        actionTotalSeconds *= 1000;
        statChange = scaleWorkStats(statChange, actionTotalSeconds);
      }
      const personStatChange = (stat: WorkStats) =>
        person instanceof PlayerObject
          ? multWorkStats(stat, multToWorkStats(Player.mults)) // player apply exp mults
          : scaleWorkStats(stat, (person as Sleeve).shockBonus()); // sleeve apply shock mult
      const gainStats = (stat: WorkStats) =>
        person instanceof PlayerObject ? person.gainStats(stat) : applySleeveGains(person as Sleeve, stat); // note: this actually applies some stat gains to ALL Sleeves + the Player

      const oldStatChange = statChange;
      statChange = personStatChange(statChange);
      gainStats(statChange);
      // this line shouldn't exist, since it is a bug #1607 (Logging part)
      statChange = oldStatChange;

      // Max Stamina change
      var maxStaminaChange = effect.maxStaminaChange;
      //this line shouldn't exist #1607 (Multiple application of stamina mults), look into getEffectiveMaxStaminaTrainingBonus
      maxStaminaChange *= this.getSkillMult(BladeburnerMultName.Stamina);
      this.staminaBonus += maxStaminaChange;

      let maxStaminaChangeLog = this.getEffectiveMaxStaminaTrainingBonus(maxStaminaChange);
      maxStaminaChangeLog = maxStaminaChange; // this line shouldn't exist, since it is a bug #1607 (Logging part)

      // Rank change, also changes Skill Points and Faction Reputation
      let rankChange = effect.rankGain(action);
      if (rankChange > 0) {
        rankChange *= currentNodeMults.BladeburnerRank;
      }
      this.changeRank(person, rankChange);
      // Team size change
      const teamChange = effect.teamMemberGain(action, this.sleeveSize, this.teamSize);
      this.teamSize += teamChange;
      if (teamChange < 0) {
        this.teamLost -= teamChange;
      }
      // Chaos change
      let chaosPercentChange = 0; // used for logging
      const cities = effect.applyChaosToAllCities ? Object.values(this.cities) : [this.getCurrentCity()];
      for (const city of cities) {
        chaosPercentChange = effect.chaosPercentageChange(person);
        city.changeChaosByPercentage(chaosPercentChange);
        city.changeChaosByCount(effect.chaosAbsoluteChange(city.chaos));
      }
      // Health change
      const previousHealth = person.hp.current;
      const healthChange = effect.hpChange(action);
      if (healthChange < 0) {
        const cost = calculateHospitalizationCost(-healthChange);
        if (person.takeDamage(-healthChange)) {
          ++this.numHosp;
          this.moneyLost += cost;
        }
      } else {
        person.regenerateHp(healthChange);
      }
      // Contract and Operation count change
      for (const contract of Object.values(this.contracts)) {
        contract.count += effect.contractCountChange(contract);
      }
      for (const operation of Object.values(this.operations)) {
        operation.count += effect.operationCountChange(operation);
      }
      // Population absolute change
      const absPopChange = effect.popChangeCount();
      const relPopChange = effect.popChangePercentage();
      const city = this.getCurrentCity();
      city.changePopulationByCount(absPopChange, { estChange: absPopChange, estOffset: 0 });
      city.changePopulationByPercentage(relPopChange.percent, {
        changeEstEqually: relPopChange.changeEqually,
        nonZero: relPopChange.nonZero,
      });

      // Population Estimate change
      const abs = effect.popEstChangeCount() * this.getSkillMult(BladeburnerMultName.SuccessChanceEstimate);
      const pct = effect.popEstChangePercentage(person) * this.getSkillMult(BladeburnerMultName.SuccessChanceEstimate);
      if (isNaN(pct) || pct < 0) {
        throw new Error("Population change calculated to be NaN or negative");
      }
      this.getCurrentCity().improvePopulationEstimateByCount(abs);
      this.getCurrentCity().improvePopulationEstimateByPercentage(pct);

      // Any other effects, like leveling up contracts/operations, counting BlackOps, or trigger migration
      effect.furtherEffect(this, action, this.getCurrentCity());
      // Logging
      const logging = loggingConstants[action.type];
      if (logging) {
        const log = effect.log({
          name: person.whoAmI(),
          statChange: statChange,
          staminaChange: this.stamina - previousStamina,
          rankChange: rankChange,
          chaosPercentChange: chaosPercentChange,
          previousHealth: previousHealth,
          stamina: this.stamina,
          hpChange: healthChange,
          maxStaminaChange: maxStaminaChangeLog,
          action: action,
          teamChange: teamChange,
          earnings: earnings,
          person: person,
        });
        if (log != "") {
          this.log(log);
        }
      }
      //this.resetAction(); // this was previously only needed for blackops
    } catch (e: unknown) {
      exceptionAlert(e);
    }
  }

and this is how for example Training would look like:

[BladeburnerGeneralActionName.Training]: new GeneralAction({
    name: BladeburnerGeneralActionName.Training,
    getActionTime: () => 30,
    generalEffect: new ActionEffect({
      staminaChange: () => 0.5 * BladeburnerConstants.BaseStaminaLoss,
      statChange: () => newWorkStats({ agiExp: 30, defExp: 30, dexExp: 30, strExp: 30 }),
      statChangeExtraMultsMultiply: true, // bug: should be false
      maxStaminaChange: 0.04,
      log: (params: ActionEffectLogParams) =>
        `${params.name}: Training completed. Gained: ${formatExp(params.statChange.strExp)} str exp, ${formatExp(
          params.statChange.defExp,
        )} def exp, ${formatExp(params.statChange.dexExp)} dex exp, ${formatExp(
          params.statChange.agiExp,
        )} agi exp, ${formatBigNumber(params.maxStaminaChange)} max stamina.`,
    }),
    desc:
      "Improve your abilities at the Bladeburner unit's specialized training center. Doing this gives experience for " +
      "all combat stats and also increases your max stamina.",
  }),

[EDIT: Alpheus deleted their original response, which may make this harder to follow. The gist of their message was that the comments in the code I posted for bladeburne.completeAction suggest separate functionalities, which should be extracted into individual functions.]

Thank you for the quick reply! It looks like you encountered the same issue I did - that the function does many (mostly) unrelated things in sequence. This is what I meant when I suggested that it could be refactored into smaller, more focused components.

I like your idea of first creating separate functions within the bladeburner class. These can later be moved into separate classes if needed.

I need a bit more time to consider your idea of exposing the log data. My initial thought for testing was to create before/after snapshots of the CompleteAction for all actions (like Training), "intersect" the calls to bladeburner.log, and include those in the snapshot. We could also "intersect" the calls to the random() functions to make them deterministic (or set the seed). Once we have the data from the current implementation, we can use it to test the refactor.

Perhaps start with one of the blocks where you have code changes? Let's get that reviewed and merged and see where we can go from there.

This is actually trickier than it sounds. The "blocks" that are neatly organized in the refactor are quite entangled in the original code. At the moment, I don't see a way to change, review, and merge them separately.

I'll push my full refactor to my bb fork, but I won't open a PR yet because there are definitely some small discrepancies between the original and the refactor. So far, I've mostly noticed some different number formatting in the logs. I want to write a test suite to catch these issues before requesting a pull. :)

Deleted old comment—I checked your code changes and got a bit of a better grasp. In short, I'm not a huge fan of where you took the design. The extra level of abstraction for the data-driven tasks is giving me more of a headache than the previous code.

I'm only really invested into this change if we follow d0sboots' original direction and maintain functional composition without the data layer. What you designed is a rule engine and the tasks themselves are not rules, they are explicit behavior that differ quite a bit between each other.

A better fit for your design would be 12 tasks modifying only 1 stat but changing how and when they do so. But 12 tasks modifying 12 different states and requiring further stat effects is a recipe for disaster in my experience. I wouldn't want to maintain this.

I'd much rather a see a safe extraction of Training and see where it fits best, as it has consequences to sleeves (which was the original problem).

I see your concerns, and need to think about it at some other time, I'm having an appointment soon, and will be back in a few hours. I hope to believe that they can be eliminated by my suggestion of moving the separate effects into their own components.

The extra level of abstraction for the data-driven tasks is giving me more of a headache than the previous code.

Can you explain into more detail, what you mean with data-driven task?

For now, I'll push all changes to my repo, maybe this helps to eliminate some of your concerns. Here: dev...cmfrydos:bitburner-src:BladeCompleteActionRefactor

Can you explain into more detail, what you mean with data-driven task?

The state-based constructor with the Action classes and Effects.
As it is this looks too far down the risky path without any new tests added.

Can we restart and start with a smaller PR, say cca. 50 lines to keep on track with your original?

Hi @Alpheus,

Thank you for your feedback. I appreciate the time you've taken to review my proposal. However, I feel that there's a bit of a disconnect in our communication, and I want to ensure we're on the same page moving forward.

I proposed writing a test suite next, but it seems you disagreed. Could you clarify why this isn't the right direction in your view? Practical suggestions on how to proceed would be very helpful.

Additionally, I suggested transitioning the system to an entity-component system, given that it's already functioning in a similar manner. I believe this approach would address the concerns you raised. If you think this would not resolve the issues, could you explain why? If there's any misunderstanding on my part, I'm open to hearing your thoughts.

To illustrate my point, here’s how I envisioned the implementation:

// Bladeburner.ts 
function completeAction(action, person) {
  for (const component of action.effects) {
  // apply's parameters will be more explicit and can include setters/getters for stats like stamina, etc.
    component.apply(action, person, this); 
  }
}

For example, here’s how you could construct the Training action using component class constructors:

// GeneralActions.ts 
const trainingActionEffect = new ActionEffect([
  new StaminaChangeComponent(() => 0.5 * BladeburnerConstants.BaseStaminaLoss),
  new StatChangeComponent(() => newWorkStats({
    agiExp: 30, 
    defExp: 30, 
    dexExp: 30, 
    strExp: 30
  }), statChangeExtraMultsMultiply = true),
  new MaxStaminaChangeComponent(0.04),
  new LogComponent((params) => 
    `${params.name}: Training completed. Gained: ${formatExp(params.statChange.strExp)} str exp, 
    ${formatExp(params.statChange.defExp)} def exp, 
    ${formatExp(params.statChange.dexExp)} dex exp, 
    ${formatExp(params.statChange.agiExp)} agi exp, 
    ${formatBigNumber(params.maxStaminaChange)} max stamina.`)
]);

Here’s how the StaminaChangeComponent might be implemented, extending a base BladeburnerComponent:

// BladeburnerComponent.ts
class BladeburnerComponent {
  apply(action, person, context) {
    // This method would be overridden by each specific component
    throw new Error("apply method must be implemented in derived classes");
  }

  getEffect(...args) {
    // This method would be overridden by each specific component
    throw new Error("getEffect method must be implemented in derived classes");
  }
}

// StaminaChangeComponent.ts
class StaminaChangeComponent extends BladeburnerComponent {
  constructor(staminaChangeFunction) {
    super();
    this.staminaChangeFunction = staminaChangeFunction;
  }

  apply(action, person, blade) {
    // Get the effective change as a tuple
    const [oldStamina, staminaChange, newStamina] = this.getEffect(action, person, blade);

    // Apply the new stamina value
    blade.stamina = newStamina;

    // Pass data to log component if it exists
    // This one is the least figured out yet. I could also imagine that completeAction collects the changes
    // and then passes them to the LogComponent if it exists.
    const logComponent = action.getEffect('LogComponent');
    if (logComponent) {
      logComponent.changes.stamina = staminaChange;
    }
  }

  getEffect(action, person, blade) {
    // Calculate the stamina change
    const oldStamina = blade.stamina
    const staminaChange = this.staminaChangeFunction(blade.maxStamina);

    // Ensure the new stamina value is within valid bounds
    const newStamina = clampNumber(0, oldStamina + staminaChange, blade.maxStamina);

    // Return the old value, the change, and the new value as a tuple
    return [oldStamina, staminaChange, newStamina];
  }
}

I truly believe that this approach could solve most of the problems we’ve identified with Bladeburners, while also aligning with the criteria we previously agreed upon. However, I’m open to reconsidering this if you can provide more detailed reasons why you think this approach might fail, either in functionality or maintainability.

If you have an alternative vision for the code’s architecture that you believe better addresses the issues at hand, I would greatly appreciate it if you could outline it more precisely.

Thank you again for your time and for any further insights you can provide. I’m committed to finding the best solution, and I hope we can align on the best path forward.

Looking forward to your response.

I'm not opposed to any of your suggestions, in principle, as long as you can keep this PR to sub-50 lines of changes prior to new tests added.

Your passion is on point, it's very appreciated. But I stand with everyone else's sentiment: take it in small steps. This is touching a lot of older parts of the codebase and you'll likely make mistakes along the way. Look at the major refactors going on in open PRs, all blocked for several months due to their scale. Keep it small and you'll get your things merged. If you want to do an all-in-one huge refactor and add tests on top while also fixing bugs, I'm afraid it's not going to pass review any time soon.

A better fit for your design would be 12 tasks modifying only 1 stat but changing how and when they do so. But 12 tasks modifying 12 different states and requiring further stat effects is a recipe for disaster in my experience. I wouldn't want to maintain this.

I think there might be some misunderstanding about the new design we're implementing. Let me clarify further:
We have n different actions, each modifying m stats. The modification of each stat by an action is independent, except for whether an action succeeds or fails.

Let me give you some pseudocode. Here is a breakdown of the current behaviour:

switch(action):
    case action_1:
        modify(stat_1), log(stat_1)
        modify(stat_2), log(stat_2)
        // ...
        // sometimes modifying stats again AFTER logging
        modify(stat_1)
        // ...
        modify(stat_m)
        break;
     case action_2:
         // ... similarly modifying some of the stats
      case action_n

Problems:

• Impossible to get the overall effect on a single stat, since it may be modified multiple times
• Difficult to prove how a single stat is modified throughout the process.
• No guarantee that logging happens after all modifications.

Refactor Design

action_attempt = action.attempt()  // Yields all effects based on success or failure
for each affected_stat in action_attempt:
    stat_change = getEffectOn(affected_stat)
    applyEffect(stat_change)
    log.append(stat_change)
log()

Benefits:

• Allows tracking the effect on a single stat by allowing maximal one Component for each stat.
• Makes it possible to prove how a stat is modified overall (behaviour encapsulated in it's own Component class).
• Guarantees that logging occurs after all stats are modified.

Thank you for your prompt response, especially this late in the evening. Regarding your last reply:

I'm not opposed to any of your suggestions, in principle, as long as you can keep this PR to sub-50 lines of changes prior to new tests added. Your passion is on point, it's very appreciated. But I stand with everyone else's sentiment: take it in small steps. This is touching a lot of older parts of the codebase and you'll likely make mistakes along the way. Look at the major refactors going on in open PRs, all blocked for several months due to their scale. Keep it small and you'll get your things merged. If you want to do an all-in-one huge refactor and add tests on top while also fixing bugs, I'm afraid it's not going to pass review any time soon.

I hope you can see that boiling a 353-line function into 6 lines would yield a super-50-line PR. You have previously expressed your support for a refactor of this function. Do you have any suggestions on how we might break down the 700-line PR into 14 50-line PRs? What about having one PR, but doing one Action (like GeneralAction.Training) at a time / commit? It would look horrible in the code until all Actions are refactored, but it might be easier to review one at a time. I still doubt that it will get merged in steps, but it might help the process of reviewing.

Yes, I'm 100% for the one action at a time refactor. Put in just as much abstraction is needed for everything that has been migrated to-date and keep everything test covered as you move.

Don't get me wrong—you can do as you wish. It's just that I won't support you if your PR will be huge by choice.
You set on a good path early on with a few multi-line change commits. I prefer we get something of that scale gradually merged.

Ok, that sounds good, thank you again for your support.
I'll prepare a small PR tomorrow, and hopefully we can get the maintainer convinced to approach this monster of a function gradually by getting subsequent PR's merged. It might make sense to ask for a branch though, as completeAction might look a little messy while we are in the midst of it. I'm not an expert on PR's and Code Reviews yet, and still have some technical questions on how to best build PR's on top of other PR's.

You won't need to build PRs on top of PRs.

1. make a minor refactor that will give you confidence in how training is structured
2. test cover it in its bugged state
3. merge it into dev

That's it. That in itself is valuable if the refactoring makes the current architecture easier to understand. We can keep doing this in small steps until the code is very clear in regards to modifiers, logs, double-dipping and their side effects onto sleeves (and any double dipping that may occur there). Once we have transparency and visibility, THEN we can decide if the bug is worth fixing.

Here, let me help. Look at your PR for example at https://github.com/bitburner-official/bitburner-src/pull/1606/files#diff-4d95b76d4df7dedcb50e350d2afb8901d87780532239de973e1e0db6deded91bR1077

Your code changes are well-contained to a continuous segment of code and they are all Training-specific.
If you were to extract lines 1077-1086 into its own function then the only inbound parameters would be retValue and staminaBonus (prior to change). It can interact with the outside world either by returning both changed values or by using retValue as a return parameter and carrying staminaGain in some other form. You could also return { retValue, staminaGain } to maintain full, yet clumsy, compatibility.

That is a simple enough function to highlight that these values should be tested pre-multiplier. You can then write a handful of tests using only what you extracted, for example:

• training provides X {stat} exp; stat=agi,dex,str,def
• training gives the player stamina and always succeeds
• training exp is affected by bladeburner multipliers (yes/no/when?)
• training exp is affected by aug multipliers (yes/no/when?)

Deeper Intution
Forget for a moment what you have designed for refactoring. Focus on what's in front of you. The file you are modifying is messy to begin with at no fault of your own. This makes any change, no matter how well-thought-out extremely risky. Prone to typos, variable shadowing, mistaking side effects through mutations on this, messing with other people's PRs.

Here is what FTA says about this file (https://ftaproject.dev/playground):

Notably Cyclomatic complexity and difficulty. These two are proxy metrics for nesting and control structures. You'll get the best bang-for-bug by taking things out of that double-nested looped switch statement and extract it in a manner that allows you to work at a low level of nesting with 1-2 parameters passed in. As long as you don't deal with this issue, any changes you make will make things worse

I'm not a huge fan of metrics, but I know it's in shambles just by looking at the codebase, especially bladeburner.completeAction. I mean, it took me two full days to entangle it. No judgement, it just grew to that by many different contributors writing in their own style extending existing behaviour.

I hope you are aware that our new plan, of extracting Action after Action, will worsen those metrics though, until everything is in the new structure. Again, I'm not a huge fan of metrics, so that's no concern to me.
I've some time now and will prepare a small pr extracting just Training using the ideas I've mentioned before, refactoring it in its components. Before, I'd like to write some tests though.

I hope you are aware that our new plan, of extracting Action after Action, will worsen those metrics though, until everything is in the new structure. Again, I'm not a huge fan of metrics, so that's no concern to me.

As you said, you're not huge on metrics. Your intuition is off and it's understandable given the circumstance. But you're incorrect. Slowly extracting the pieces will improve on complexity and control structures at a small cost to line count. Once everything's in place. You'll spot the underlying designs, such as:

• There are a few actions that only serve to give the player stats
• There are many actions that serve to change the BB world state
• There are a few special actions that serve only to keep the BB player stable

Most actions give BB XP. Some change population.

I already went through this, remember :)? I've noticed that only after extracting a few actions, like all of the Operations, you start to loose branches, thus cyclomatic complexity. I also extracted them in one go before (play-)testing. If you want a running version you need to have a guard that protects you from executing (parts) of the actions twice.

Pseudocode:

extractedActions = ["General.Training"]
if(extractedActions.includes(action){
 newCompleteAction()
} else {
  oldCompleteAction()
}

I'm not 100% sure if that's really needed for the first PR, writing this from my mobile, but at the latest when refactoring contracts or operations, since there is some code that gets executed regardless of the type of contract or operation. Also creating the entity-component system creates some initial overhead, that might introduce complexity.

IMO, whenever you have multiple systems/algorithms in place, and treat your data either or, that requires some cognitive capacity to first understand both systems and then decide on which applies for which object.

What I meant by being not a fan of code metrics is that they just provide numbers that have no intrinsic value. Only when you start to interpret them, like saying programs with less cyclomatic complexity are less "complex" and easier to understand, they start to become useful. But that's often not the truth. Instead we tend to start optimizing for a metric, and this often leads to worse code. Also, what does something like LOC actually mean? There are many different definitions of what a line of code is, like whether it includes empty lines, or ones with a comment. Also, the average "impact" of a line differs alot between programming languages and styles. So a 1000 line Haskell program might be more complex or powerful than a 2000 line Java or
4000 line assembler solution.

I'm currently writing some more tests and hadn't had the time I hoped for today, but will finish tomorrow. Maybe this will then show why I doubt there will be a significant loss of complexity by extracting only a single action.

This is all hyperbole. Yes, optimising for metrics can have bad impacts. But having no metrics at all is just as careless. In this case what we want to look for is trends.

You have the baseline from my screenshot. Do a couple refactors.
Did most stats improve? Yes, we keep it / No, start over.

There are a lot of benign examples of getting rid of branches. Some of which are the logging guards which are always cohesive to their action type along with their actual content—again, fully cohesive with the specific action.

As for components, we already have action objects to capture each type. If you want to play with it I suggest subclassing it further to capture each specific action.

That's what inheritance is good for: subtype polymorphism. Subtype polymorphism is the solution to the code smell of having identity checks with if/else meshes or switches. That's exactly our use case.

Here's another analysis of this file, in case you want to use some more metrics (this is from the dev version):
Duplicate code: lines 586-595
Duplicate code: lines 600-609
Duplicate code: lines 776-786
Duplicate code: lines 964-972
Duplicate code: lines 1,034-1,042
Duplicate code: lines 1,060-1,070

Creating opportunities to capture duplications in population change, hp loss and team size.

I am really sorry, but I'd like to end this here. I am not getting anything out of this conversation. It seems like we don't find a common ground, and this communication is spoiled. I've tried to get a discussion going, but I don't believe you're either taking my arguments nor my follow up questions seriously. Furthermore, I don't see a clear line of thought in your messages, since it seems to me that you are abreviating from previous statement's you've made. This is not worth both our time.

Were I'd like to leave this is as follows:
The original purpose of this PR, fixing the game's bugs by a series of small changes, was not approved. The original game code seemed to be too complicated for both the code reviewer and maintainer to grasp, so they asked for a more complete refactor. After providing a full refactor there were valid concerns that a +/- 600 line refactor is too hard to review, but we didn't find a solution to fix this issue of splitting it down due to communication difficulties.

For anybody who'd like to go from here:

I encapsulated all Actions behaviour into a system where their effects are composed together. What's missing, imo, is a refactoring step to break the application of all components into a separate getEffect and applyEffect functionality.
Also, their exact-to-original behaviour should be shown and supported by a series of unit tests.

Sad to see it, but probably for the best.

Sorry I wasn't around when this conversation was happening. Here's a collection of semi-related thoughts:

• Snarling (the lead maintainer) did a big refactor of Bladeburner, and this was the state it ended up in. He acknowledged at the time that it still had a long way to go. Bladeburner has been one of the ugliest parts of our codebase for a while, and it's kind of obvious why.
• I did ask for a different refactor, but I wasn't entirely sure what that would entail. I'm still not entirely sure.
• I also prefer changes that are small bits at a time, but that might be less possible in Bladeburner's case, where all the logic is crammed into a single giant function currently.
• I don't put much weight in metrics either. Metrics will never be able to capture code cleanliness. Most of the time when metrics like LOC show things have improved, it was obvious that they had improved from casual inspection, and they don't help decide contentious cases.
• This is not the first time someone has failed to improve Bladeburner, and it will probably not be the last. :(

I still believe that my approach would suit the needs of bladeburner best, until convinced otherwise ;), but it will not be about creating 14 or so new singleton classes. I think composition works in this case way better than polymorphism. So I would not bury this approach completely yet. I first need to get an idea about how to introduce this refactor in small pieces that it can be reviewed easily. Until I figured this out, this will be on pause. The refactor is not hard, it's just a lot. And I'm kind of new to PR's and code reviews, as I previously worked directly on repositories. There is always much to learn.

This is a success in that it has brought the three-four of us together.

I apologise to setting a standard without really checking for buyin or providing example. Your passion has inspired me to make things easier as well.

I'll be opening up a few smaller PRs. Hopefully you will use them as inspiration on how to keep PRs small and maybe learn something new on refactoring.