This blog post was jointly written by Lanae BK and Brittany DePoi.
With software engineering experience comes the extremely useful ability to recognize code smells. 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
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.
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
- Mutating properties after instantiating an empty object
- Lots of interior references (eg,
x.x.status = true) - Private data was exposed by the
BotActionclass, which made classes likeDiffAnalysisResponsenecessary. Essentially, theseResponseclasses 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.
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:
- Use private constructors with inferred field parameters for building all objects
- Encapsulate creation logic via static methods with standardized naming conventions (eg,
.from(x, y)) - Make immutable objects with limited
readonlyandstaticproperties
Refactored code
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
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:
- Simplified boolean tests replace examining the string values of internal data fields
- 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 - The small functions provide more complete unit test coverage in a syntax that is easily replicable and comprehensive
Hot new tests
// 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:
- We gained better encapsulation with simple interfaces for deeper objects.
- We gained object safety giving exclusive control to classes to ensure valid and unchanged objects.
- We reduced cognitive overhead of tests, making it way more obvious what is expected and being tested.
- 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!
Top comments (0)