Changing Everything, So We Can Change Nothing: In Pursuit of Immutability
Tags: codesmells, refactor, typescript, immutability
Published: 2020-05-08 15:22:48

---
title: "Changing Everything, So We Can Change Nothing: In Pursuit of Immutability"
published: true
tags: [code smells, refactor, typescript, immutability]
---

This blog post was jointly written by [Lanae BK](https://dev.to/lanae_bk) and [Brittany DePoi](https://dev.to/brittdepoi).


With software engineering experience comes the extremely useful ability to recognize [code smells](https://en.wikipedia.org/wiki/Code_smell). In an attempt to fight the phenomenon of sensory adaptation - when a smell fades after you get used to it - we conduct periodic code reviews with other engineers who are not involved in regular development for our product. These larger code reviews are a great opportunity to share knowledge across the team.

During a recent review, the team determined our code to be slightly malodorous. Looking at the original code snippet below, you may be able to identify the smell.

**Old, smelly code**

```typescript
export class DiffAnalysisAction {
  public async handle(
    state: string,
    api: MergeRequestApi,
    customConfig: IBotActionConfigResponse,
    logger: winston.Logger
  ): Promise<DiffAnalysisResponse> {
    const diffAnalysisResponse: DiffAnalysisResponse = new DiffAnalysisResponse();
    const diffAnalysisNote: DiffAnalysisNote = new DiffAnalysisNote();

    const apiResponse: GitLabGetResponse = await api.getSingleMRChanges();

    diffAnalysisResponse.apiRequest = apiResponse.apiRequest;

    if (
      diffAnalysisResponse.apiRequest.success &&
      apiResponse.result.hasOwnProperty("changes")
    ) {
      diffAnalysisResponse.totalDiffs = calculateDiffs(
        apiResponse.result.changes
      );

      diffAnalysisResponse.goodGitPractice =
        diffAnalysisResponse.totalDiffs < customConfig.threshold;
    }

    diffAnalysisResponse.mrNote = diffAnalysisNote.create(
      customConfig,
      diffAnalysisResponse.apiRequest.success,
      state,
      diffAnalysisResponse.goodGitPractice,
      diffAnalysisResponse.totalDiffs,
      logger
    );

    return diffAnalysisResponse;

  }
}
```

Looking at the code above, you may have noticed the tendency to instantiate objects and then modify their properties. These properties are derived from logic dependent on various API call results. This syntax evolved slowly over time as a response to other issues addressed in previous refactors, and we had become accustomed to the smell.

Now let's look at that same code, this time with `// <#>` to indicate the lines where the code smell is on full display. `Response` and `Note` objects are created, each property of the `Response` object is then modified after the API call succeeds, and the `Note` object is instantiated only to call a method.

```typescript
export class DiffAnalysisAction {
  public async handle(
    state: string,
    api: MergeRequestApi,
    customConfig: IBotActionConfigResponse,
    logger: winston.Logger
  ): Promise<DiffAnalysisResponse> {
    const diffAnalysisResponse: DiffAnalysisResponse = new DiffAnalysisResponse(); // <1> 
    const diffAnalysisNote: DiffAnalysisNote = new DiffAnalysisNote(); // <2> 

    const apiResponse: GitLabGetResponse = await api.getSingleMRChanges();

    diffAnalysisResponse.apiRequest = apiResponse.apiRequest; // <3> 

    if (
      diffAnalysisResponse.apiRequest.success &&
      apiResponse.result.hasOwnProperty("changes")
    ) {
      diffAnalysisResponse.totalDiffs = calculateDiffs(
        apiResponse.result.changes
      ); // <4> 

      diffAnalysisResponse.goodGitPractice =
        diffAnalysisResponse.totalDiffs < customConfig.threshold; // <5> 
    }

    diffAnalysisResponse.mrNote = diffAnalysisNote.create( // <6> 
      customConfig,
      diffAnalysisResponse.apiRequest.success,
      state,
      diffAnalysisResponse.goodGitPractice,
      diffAnalysisResponse.totalDiffs,
      logger
    );

    return diffAnalysisResponse; // <7> 

  }
}
```

### The Smells

1. Mutating properties after instantiating an empty object
2. Lots of interior references (eg, `x.x.status = true`)
3. Private data was exposed by the `BotAction` class, which made classes like `DiffAnalysisResponse` necessary. Essentially, these `Response` classes were used like interfaces in order to enforce a reduced class view for logging and encapsulating unique properties, as seen in the following code snippet.

```typescript
export class DiffAnalysisResponse extends BotActionResponse {
  totalDiffs: number;

  constructor(
    apiRequest: GitLabAPIRequest,
    goodGitPractice: boolean,
    mrNote: string,
    totalDiffs: number
  ) {
    super(apiRequest, goodGitPractice, mrNote);
    this.totalDiffs = totalDiffs;
  }
}
```

While this code was sufficient for its task, it introduced complexity in the form of poor data encapsulation and higher-than-necessary cognitive overhead. In addition to leaky abstraction, the excessive mutability also lent itself to insecure and possibly invalid objects.

## Purging the smell

Once this (now obvious) code smell was pointed out, we realized we should refactor 3 key things to improve our code:

1. Use private constructors with inferred field parameters for building all objects
1. Encapsulate creation logic via static methods with standardized naming conventions (eg, `.from(x, y)`)
1. Make immutable objects with limited `readonly` and `static` properties

**Refactored code**

```typescript
export class BotAction {
  constructor(
    readonly apiRequest: GitLabAPIRequest,
    readonly goodGitPractice: boolean,
    readonly mrNote: string
  ) {}
}

export class DiffSize extends BotAction {
  private constructor(
    apiRequest: GitLabAPIRequest,
    goodGitPractice: boolean,
    mrNote: string,
    readonly totalDiffs: number
  ) {
    super(apiRequest, goodGitPractice, mrNote);
  }

  static async from(
    state: string,
    api: MergeRequestApi,
    customConfig: BotActionConfig,
    logger: winston.Logger
  ): Promise<DiffSize> {
    let totalDiffs: number;
    let goodGitPractice!: boolean;

    const apiResponse: GitLabGetResponse = await api.getSingleMRChanges();

    if (
      apiResponse.apiRequest.success &&
      apiResponse.result.hasOwnProperty("changes")
    ) {
      totalDiffs = calculateDiffs(apiResponse.result.changes);
      goodGitPractice = totalDiffs < customConfig.threshold;
    } else {
      totalDiffs = -1;
    }

    return new DiffSize(
      apiResponse.apiRequest,
      goodGitPractice,
      DiffSizeNote.buildMessage(
        customConfig,
        apiResponse.apiRequest.success,
        state,
        goodGitPractice,
        totalDiffs,
        logger
      ),
      totalDiffs
    );

  }
}
```

We introduced the use of a private constructor to instantiate the object right before passing it back to the caller, _after_ all the logic was performed to derive each unique value. In addition, we renamed the public method `handle` to `from` to make clear that the `Response` object is being created _from_ the parameters passed in by the method caller.

The redesign also enabled us to combine the `Action` and `Response` classes into a single class that extends `BotAction`. Previously, `Response` was essentially being used as an interface to enforce conformity across all `Actions` response types and allow some `Actions` to include unique properties in their response.

All methods were modified to be static wherever possible - there is no need to create an object that is never actually passed to any callers.

Now classes like `Note` act more as namespaces that encapsulate all the data and methods required, hiding anything from other classes that don't need to know about the internal workings.

## A (positive) side-effect

One unintended - but very positive - side-effect to come out of the refactoring was the ability to rewrite tests in a clear, consistent way.

For example, before the addition of static properties and methods to the `Note` classes, a `controlNote` had to be created and the properties of that reference instance would then be matched against a string (an internal field - another example of leaky abstraction) returned by our test case instance.

**Old, smelly tests**

```typescript
const controlNote = new BranchAgeAnalysisNote(
  customConfig,
  undefined,
  undefined,
  undefined,
  winlog,
);

describe("branchAgeAnlysisNote function: checking handling of api response", () => {
  const state = undefined;
  const gitLabRequestSuccess = false;
  const goodGitPractice = true;
  expect(
    new BranchAgeAnalysisNote(
      customConfig,
      gitLabRequestSuccess,
      goodGitPractice,
      state,
      winlog,
    ).message,
  ).toBe(controlNote.checkPermissionsMessage);
});
```

With the introduction of static properties and by extracting `Note` logic into small functions that return booleans, our `Note` tests improved in three ways:

1. Simplified boolean tests replace examining the string values of internal data fields
1. For cases where string matching is still desirable, static properties eliminate the need for a `controlNote`, and the string itself is now the return value of the function, so the caller no longer needs to know about internal object details
1. The small functions provide more complete unit test coverage in a syntax that is easily replicable and comprehensive

**Hot new tests**

```typescript
// quick boolean check - string matching not required
describe("standardCaseForCheckPermissionsMessage(gitLabRequestSuccess) function", () => {
  describe("when gitLabRequestSuccess is false", (gitLabRequestSuccess = false) => {
    test("RETURNS BOOLEAN: true", () => {
      expect(
        BotActionNote.standardCaseForCheckPermissionsMessage(gitLabRequestSuccess)
      ).toBe(true);
    });
  });
});

// no more controlNote
describe("conditionallyAddHashtag(message, hashtag) function", (hashtag = "#MyCoolBotAction") => {
  describe("when message === checkPermissionsMessage", (message = BotActionNote.checkPermissionsMessage) => {
    test("RETURNS STRING: checkPermissionsMessage", () => {
      expect(BotActionNote.conditionallyAddHashtag(message, hashtag)).toBe(
        BotActionNote.checkPermissionsMessage
      );
    });
  });
});
```

Improvements like the ones discussed above are a specific example of changes made throughout the codebase. The biggest takeaway for us is what we gained from a focus on immutability:

1. We gained better encapsulation with simple interfaces for deeper objects.
2. We gained object safety giving exclusive control to classes to ensure valid and unchanged objects.
3. We reduced cognitive overhead of tests, making it way more obvious what is expected and being tested.
4. We improved readability by making objects simpler to create, with increased clarity on what factors influence their creation.

We also learned that code smells are hard to detect by those closest to the code, so it is important to get frequent feedback from fresh eyeballs!