Earlier this week I set aside a couple of hours to try building a new app prototype using GitHub Co-Pilot from the start. The initial functions that it suggested did the job, but they set the CodeMetrics complexity VS Code extension into overdrive ("Complexity is 16 You must be kidding").
NOTE: The CodeMetrics extension computes complexity in TypeScript / JavaScript. They state that it is a close approximation of the Cyclomatic Complexity of the code. This "Cyclomatic Complexity" metric is great for keeping an eye on the "cleanliness" of your code (especially for writing "Code That Fits In Your Head"), but it's not the be-all and end-all. It's a good guide, but it's not the law; it's a tool, not a master.
So, with the complexity alarm bells ringing, I decided this would make a good refactoring case study. As a brand new and fairly simple app, the functionality required is still quite minimal - but it's surprising how much complexity can be hidden amongst even a seemingly simple function. Hopefully you find it a nice, real-world example of how to clean up code. (I'm still not entirely done with the refactoring; I need to think more about names etc. but I think it's a good start).
Quick context: The app is a simple note-taking app. The user will be able to add, edit, and delete notes. They can also make a note a "super note". A super note is one of the three notes that the user wants to keep at the top of the list. The user can have up to three super notes - so if there are already three super notes, the oldest one must be demoted.
Ok, let's go! Here's some code from the app, a function that will be called when a user clicks a note to make it a "super note":
// cc: 10
const makeSuper = (id: string) => {
setNotes((prevNotes) => {
let superNotes = prevNotes.filter((note) => note.isSuper);
if (superNotes.length === 3) {
let oldestSuperNote = superNotes.reduce((oldest, note) => {
return note.lastUpdated < oldest.lastUpdated ? note : oldest;
});
oldestSuperNote.isSuper = false;
}
let noteToPromote = prevNotes.find((note) => note.id === id);
if (noteToPromote) {
noteToPromote.isSuper = true;
noteToPromote.lastUpdated = Date.now();
}
return [...prevNotes];
});
};
As you can see, the Cyclomatic Complexity is reported as 10. That's not terrible, but it's nowhere near great either. It's not even "good". And, as this is just the start of the app, it's only going to get worse! However, beyond the stated complexity, that really is a horrible function! It's using "prevNotes" in a way that's really not clear, and quite probably not safe.
The first thing I did was tell Co-Pilot about the complexity and ask it to resolve the situation :) The results were not great. It suggested a function that was even more complex than the original!
// cc=16:
const makeSuper = (id: string) => {
setNotes((prevNotes) => {
const updatedNotes = prevNotes.map((note) => {
if (note.id === id) {
return { ...note, isSuper: true, lastUpdated: Date.now() };
}
return note;
});
const superNotes = updatedNotes.filter((note) => note.isSuper);
if (superNotes.length > 3) {
const oldestSuperNote = superNotes.reduce((oldest, note) =>
note.lastUpdated < oldest.lastUpdated ? note : oldest
);
const demotedNotes = updatedNotes.map((note) => {
if (note.id === oldestSuperNote.id) {
return { ...note, isSuper: false };
}
return note;
});
return demotedNotes;
}
return updatedNotes;
});
};
Although that appears to be a more complex function (ignoring empty lines there are 24 lines of code, with a cyclomatic complexity of 16), I actually prefer this as a base camp to start from. I can at least immediately see that setNotes will set to whatever "updatedNotes" is. This is a much nicer pattern that I can work with.
So, I decided to take it from here myself. The first thing I
dislike is that the whole complexity is embedded inside the
"setNotes" state setter. I can't see at a glance what setNotes
will actually "set notes" to. "notes" is our state, so "makeSuper"
must update this when it is "upgrading" "promoting*" a
note to a super note, because one of the "notes" has changed its
"super" status. But it doesn't need to (and shouldn't) do the
whole job itself.
* I've changed the word "upgrading" to "promoting" here. I did this in edit, after I refactored and wrote the rest of this article. The reason will become clearer later, but I'll give away a spoiler - we later create a function to "demoteOldestSuperNote". One of the key things when refactoring is to make sure your names (functions, variables, files, states etc.) are clear and unambiguous. You should try to use similar and related words for similar and related actions. So, "promote" is a far better word than "upgrade" in this context, because it's the opposite of "demote".
Ok, essentially "makeSuper" is currently doing two main things: managing the super notes and updating the overall notes array. Let's split that functionality out. First, I'll extract the bulk of the logic to a separate function, then just do a simple setNotes call. I'll declare my intentions up front with an "updatedNotes" variable, then use this at the end to setNotes:
const makeSuper = (id: string) => {
let updatedNotes = [...notes];
// ... the logic to update the updatedNotes array:
// 1. Set selected note to 'super' status
// 2. If there are now more than 3 super notes, demote oldest super note
setNotes(updatedNotes);
};
Now I can focus on the logic to update the "updatedNotes" array. The first thing I want to do is change the "super" status of the appropriate note ("set selected note to 'super' status"). A fair API for this might look like this (although, I'm still not completely happy with the name of the function!):
updatedNotes = updateNoteToSuper(updatedNotes, id);
This updates makeSuper to look like this:
const makeSuper = (id: string) => {
let updatedNotes = [...notes];
// 1. Set selected note to 'super' status
updatedNotes = updateNoteToSuper(updatedNotes, id);
// 2. If there are now more than 3 super notes, demote oldest super note
// TODO
setNotes(updatedNotes);
};
So, the new function updateNoteToSuper() will deal with the "one thing" of updating one of the notes in "updatedNotes" to be a super note. Let's write this function then. It will take the "updatedNotes" array and the "id" of the note to update. It will return a new array with the appropriate note updated. Here's one way to do it:
const updateNoteToSuper = (notes: Note[], id: string): Note[] => {
return notes.map((note) => {
if (note.id === id) {
return { ...note, isSuper: true, lastUpdated: Date.now() };
}
return note;
});
};
So we map over the supplied "notes" array (passed in as an argument), and if we find the note with the correct "id", we return a new note with the "isSuper" status set to true and the "lastUpdated" set to the current time. Otherwise, we return the note unchanged.
This is ok, but for the sake of this example, I want something even more explicit, something that's easier to read for a very junior developer. Let's have another go:
const updateNoteToSuper = (notes: Note[], id: string): Note[] => {
const noteIndex = notes.findIndex((note) => note.id === id);
if (noteIndex === -1) return;
const updatedNote = updateNote(notes[noteIndex], id, true);
const updatedNotes = [...notes];
updatedNotes[noteIndex] = updatedNote;
return updatedNotes;
};
This is a bit more explicit. We find the index of the note we want to update, then we update the note, then we update the array of notes and return it. This is a bit more verbose, but it's also a bit more explicit. It's a trade-off (and one that's constantly in my mind as I code).
I know many developers prefer the first, more succinct version. But I think this is a good example of how to refactor code to make it more readable and maintainable. And that's the point of refactoring, right?
So, you may have noticed that I've introduced a new function in the above code: "updateNote". I'll write that now.
"updateNote" is a function that takes a note, an id, and a boolean, and returns a new note with the "isSuper" status set to the boolean (if it's always going to "update Note.. to set its super status" then the name can be improved, e.g. setNoteSuperStatus). Here's the function:
const updateNote = (note: Note, id: string, isSuper: boolean): Note => {
if (note.id === id) {
return { ...note, isSuper, lastUpdated: Date.now() };
}
return note;
};
This is a simple function that takes a note, an id, and a boolean, and returns a new note with the "isSuper" status set to the boolean. If the note's id matches the id passed in, we return a new note with the "isSuper" status set to the boolean and the "lastUpdated" set to the current time. Otherwise, we return the note unchanged.
Now we've sorted the "super" status of the selected note, we need to deal with the other requirement of the app: "if there are already three super notes, the oldest one must be demoted". I added this code to the makeSuper function:
// If there are now more than 3 super notes, demote oldest super note:
const superNotes = updatedNotes.filter((note) => note.isSuper);
if (superNotes.length > 3) {
updatedNotes = demoteOldestSuperNote(updatedNotes, superNotes);
}
This leaves our "makeSuper" function looking like this:
const makeSuper = (id: string) => {
let updatedNotes = [...notes];
// 1. Set selected note to 'super' status
updatedNotes = updateNoteToSuper(updatedNotes, id);
// 2. If there are now more than 3 super notes, demote oldest super note
const superNotes = updatedNotes.filter((note) => note.isSuper);
if (superNotes.length > 3) {
updatedNotes = demoteOldestSuperNote(updatedNotes, superNotes);
}
setNotes(updatedNotes);
};
You'll see there's another new function in there - let's write that "demoteOldestSuperNote" function. This function will take an array of notes and an array of the superNotes within it, and return a new array with the oldest super note demoted. Here's the function:
const demoteOldestSuperNote = (notes: Note[], superNotes: Note[]) => {
const oldestSuperNote = findOldestSuperNote(superNotes);
const oldestSuperNoteIndex = notes.findIndex(
(note) => note.id === oldestSuperNote.id
);
const updatedNotes = [...notes];
updatedNotes[oldestSuperNoteIndex] = updateNote(
updatedNotes[oldestSuperNoteIndex],
oldestSuperNote.id,
false
);
return updatedNotes;
};
This function is a bit more complex. It finds the oldest super note (another new function), then finds the index of this note in the provided "notes" array. It then updates that oldest super note to be a non-super note, and returns the updated array of notes.
The "findOldestSuperNote" function is a simple one:
const findOldestSuperNote = (superNotes: Note[]) => {
return superNotes.reduce((oldest, note) =>
note.lastUpdated < oldest.lastUpdated ? note : oldest
);
};
This function is a simple one. It takes an array of notes (supernotes) and returns the oldest super note. It does this by reducing the array of super notes to a single note, starting with the first note in the array. It then compares each note to the current oldest note, and if the note is older, it becomes the new oldest note. The final oldest note is returned.
So, that's the refactored code. You may immediately notice that there are more lines of code in the refactored version. But this is fine. We don't have to read them all at once - in fact, we don't have to read them all at all! We split our one function down into multiple functions. As we need to dive deeper through the abstraction layers, we can read the code in chunks, and each chunk is doing one thing.
The code is now far less complex, and far more readable. It's also far easier to test. We can now write tests for each of the functions, and we can test them in isolation. This is a huge benefit of refactoring - it makes your code easier to test. And easier to test code is more reliable code. The chance of bugs being introduced is reduced - and the chance of us being able to find and fix any bugs that crop up is increased massively. This is a win-win!
Remember: your code is written once, but it's read many times. So, make it easy to read. Make it easy to understand. Make it easy to maintain. Make it easy to test. Make it easy to extend. Make it easy to debug. Make it easy to reason about. Make it easy to love!
I hope this has been useful. I'm always looking for ways to improve my code, and I hope you are too. If you have any suggestions or comments, please let me know!
