The refactoring story today is about a UI editor for a table of Students that lets a user add, rename, and remove columns.
Introduction
The result of an operation is a series of changes that have to be applied and saved to the database.
I will skip the React portion of the code and focus on how to model the changes with types and simplify the update process.
The code uses fp-ts types and functions. You can see the functions and namespaces imported at the top.
If you wish to play with the code I have created a public repl.it that can be forked.
Let’s take a look at the code:
import { pipe } from 'fp-ts/function';
import * as A from 'fp-ts/Array';
const changeExistingStudentsTable = (tableInfo: any, columnChanges: any, idx: number) => {
const isChange = (columnChange: any): boolean =>
!(
!columnChange.mergedCol &&
!columnChange.deleted &&
columnChange.original === columnChange.current
);
const mergeCol = (sourceCol: string, targetCol: string, needDeleteSourceCol: boolean) => (d: any) => {
// if create new col and merge at the same time,
// no need to remove the new col as it does not exist; only fill merged col with null
if (!needDeleteSourceCol) {
d[idx].values = d[idx].values.map(v => {
v[targetCol] = null;
return v;
});
} else {
d[idx].columns = A.filter(e => e !== sourceCol)(d[idx].columns);
d[idx].values = d[idx].values.map(v => {
v[targetCol] = v[sourceCol];
v[sourceCol] = undefined;
return v;
});
}
return d;
};
const deleteCol = (deletedCol: string, isExist: boolean) => (d: any) => {
// if {current : 'new column', deleted: true} -> add and immediately delete, no need to delete
// if {current : 'new column', original: 'exist name', deleted: true} -> add 'new column' save, then change name and delete --> delete 'new column'
if (isExist) {
// todo:
d[idx].columns = A.filter(e => e !== deletedCol)(d[idx].columns);
d[idx].values = d[idx].values.map(v => {
v[deletedCol] = undefined;
return v;
});
}
return d;
};
const addCol = (newCol: string) => (d: any) => {
d[idx].columns.push(newCol);
d[idx].values = d[idx].values.map(v => {
// todo: how to refactor
v[newCol] = undefined;
return v;
});
return d;
};
const renameCol = (old_name: string, new_name: string) => (d: any) => {
d[idx].columns = pipe(
d[idx].columns,
A.filter(e => e !== old_name),
cols => [...cols, new_name],
);
d[idx].values = d[idx].values.map(v => {
v[new_name] = v[old_name];
return v;
});
return d;
};
const getChangeFunc = (columnChange: any): any => {
if (columnChange.mergedCol) {
return mergeCol(
columnChange.current,
columnChange.mergedCol,
Boolean(columnChange.original),
);
} else if (columnChange.deleted) {
return deleteCol(columnChange.original, Boolean(columnChange.original));
} else if (!columnChange.original) {
return addCol(columnChange.current);
} else if (columnChange.original !== columnChange.current) {
return renameCol(columnChange.original, columnChange.current);
}
};
const modifyData = (cur: any, op: any) => op(cur);
return pipe(
columnChanges,
A.filter(isChange),
A.map(getChangeFunc),
A.reduce(tableInfo, modifyData),
);
};
Identifying candidates for refactoring
I noticed that the signature of changeExistingStudentsTable
uses the type any
for each of the arguments.
Perhaps I could narrow the types to make them more specific to the actual possible values. Plus one for modeling with types!
Looking at how the arguments are used in the main function, seems like a good idea to create a Student
type
and a StudentTable
type that contains the columns and a collection of values (rows).
type Student = Record<string, any>;
type ColumnName = string;
type StudentTable {
columns: ColumnName[];
values: readonly Student[];
}
// For example a table that has First Name, Last Name, Age, and Eye Color would look like this:
const table: StudentTable = {
columns: ["FirstName", "LastName", "Age", "EyeColor"],
values: [
{FirstName: "Juan", LastName: "Rodriguez", Age: 10, EyeColor: "Green"},
{FirstName: "Rose", LastName: "Bidai", Age: 11, EyeColor: "Brown"}
]
}
Modeling changes to the columns
The changes could be modeled as a type (I call it legacy to mark the difference later with the refactored version):
type LegacyColumnChange = {
deleted?: string;
original?: string;
current?: string;
mergedCol?: string;
};
Having all fields optional hints that some combinations probably are invalid. We can try to represent the changes in a way that captures the original intention behind the fields but makes the states that are invalid, not possible.
type ColumnName = string;
type RemoveColumn = { type: 'remove'; target: ColumnName };
type AddColumn = { type: 'add'; target: ColumnName };
type RenameColumn = { type: 'rename'; newName: ColumnName; oldName: ColumnName };
// Possible values for the type of change
type ValidColumnChange = RemoveColumn | AddColumn | RenameColumn;
// But sometimes a change should be ignored
type NoChange = { type: 'none' };
// The actual type combines both
type ColumnChange = ValidColumnChange | NoChange;
Good first step, however, the original code relies on the logic behind the combination of the fields.
For example, the columns current
and deleted
may both be set to indicate that the column
was added but deleted after.
Instead of encoding the business logic of the change in the combination of fields it would be better to make the logic explicit by using the types defined above and have clear data that represents each change.
Here is the explanation of what the fields represent in LegacyColumnChange
(columns names A and B are just for example purposes):
Fields | Change | Implementation |
---|---|---|
mergedCol and original exists |
Merge B into A | Delete A and rename B to A |
mergedCol exists but original not |
No change is needed | - |
deleted and original exists |
Delete B | Column B should be removed from columns and each object in values
|
original does not exist but current exists |
Add B | Column B added dot columns (no need to modify values) |
original and current do not exist |
No change is needed | - |
original different than current
|
Rename column A with B | Column A should be called B in columns
|
any other case | No change is needed | - |
All legacy changes can be mapped to one ColumnChange
except for merging columns. Merging columns can be represented with two operations (delete and rename). Using a function that given a legacy change generates multiple ColumnChange
captures all scenarios.
// Converts a legacy change to a well-defined change
function legacyToChange(change: LegacyColumnChange): readonly ColumnChange[] {
// skip this for now
// the logic will be the one mentioned in the table above.
}
Converting a change into a function that applies it to the table
Let’s take a look at the signature of the functions that take a column change
and return another function that will apply the change to the StudentTable
.
const mergeCol = (sourceCol: string, ...) => (d: any) => ...;
const deleteCol = (deletedCol: string, ...) => (d: any) => ...;
const addCol = (newCol: string) => (d: any) => ...;
const renameCol = (oldName: string, ...) => (d: any) => ...;
All the functions are higher order functions.
Higher order functions is a cool technique that helps us encapsulate logic that can be called later.
Each function matching a change will return in turn another function that captures how to apply the change.
Similar to before, is better to have types for the arguments and the result:
// Takes the tables and the index, does the change, and returns the tables updated
type UpdateFn = (tables: StudentTable[], idx: number) => StudentTable[];
// T can be any change
// Given an `AddColumn` I need a `UpdateFn` that adds the column
// Given a `RemoveColumn` I need a `UpdateFn` that removes the column
// and so on ...
type ChangeMapFn<T extends ColumnChange> = (change: T) => UpdateFn;
But the function will be called only for valid changes, thus I can use the ValidColumnChange
type instead.
Every opportunity to narrow the domain of a type is a chance to improve the code for reading.
type ChangeMapFn<T extends ValidColumnChange> = (change: T) => UpdateFn;
For example, now the AddColumn
function looks like this:
// T, in this case, is `AddColumn`
const addCol = ({ target: ColumnName }: AddColumn) => {
return (tables: readonly StudentTable[], idx: number) => {
// The logic goes here
}
};
I could simplify a bit by using the ChangeMapFn
type:
// The types are specified on the left
const addCol: ChangeMapFn<AddColumn> = ({ target }) => (tables, idx) => ....
Before looking at each function I want to rework how the decision of what to call is made.
Deciding which function to call
Here is the original code:
// Decides given a change which function will do the actual update
const getChangeFunc = (columnChange: LegacyColumnChange): any => {
if (columnChange.mergedCol) {
return mergeCol(...),
);
} else if (columnChange.deleted) {
return deleteCol(...));
} else if (!columnChange.original) {
return addCol(...);
} else if (columnChange.original !== columnChange.current) {
return renameCol(...);
}
};
Because I have a function that captures the logic of converting legacy into ColumnChange
I’ll change
the signature of the function (also rename it to changeToUpdateFn
):
const changeToUpdateFn = (change: ValidColumChange): UpdateFn => ...
One option, is to take advantage of the fact that the ColumnChange
type has a type
discriminator field, and write
a switch
to obtain the function that will do the update:
switch (change.type) {
case 'add':
return addCol(change); // returns a function of type UpdateFn
case 'remove':
return deleteCol(change); // same here
case 'rename':
return renameCol(change); // same here
}
However, considering that addCol
, deleteCol
, etc, all return the same kind of function perhaps there is
a way to avoid repeating the last return
at the end of each branch.
This looks quite like pattern matching on the type. And though TS does not support it (yet) external libraries like ts-pattern could be used:
// Takes a valid column change ... and uses the `type` field to decide
const changeToUpdateFn = (change: ValidColumnChange): UpdateFn =>
match(change)
.with({ type: 'add' }, (change: AddColumn) => addCol(change))
.with({ type: 'rename' }, ...)
.with({ type: 'remove' }, ...)
.exhaustive();
This is a case when using a lambda to call a function is equivalent to using the function itself. Here is the code with all the functions:
const changeToUpdateFn = (change: ValidColumnChange): UpdateFn =>
match(change)
.with({ type: 'add' }, addCol) // when matching calls a function with the `change`
.with({ type: 'rename' }, renameCol)
.with({ type: 'remove' }, deleteCol)
.exhaustive();
The update functions
The signature for all the functions is very similar (they all return a UpdateFn
):
// A function to convert a change into an updating function
type ChangeMapFn<T extends ValidColumnChange> = (change: T) => UpdateFn;
const deleteCol: ChangeMapFn<RemoveColumn> = (change) => (table, idx) => ...
const renameCol: ChangeMapFn<RenameColumn> = (change) => (table, idx) => ...
const addCol: ChangeMapFn<AddColumn> = (change) => (table, idx) => ...
Also, all functions have in common that they modify the columns
and the values
for the StudentTable
.
To capture that common functionality I could create a function that focuses on the updates:
type ColumnNames = readonly ColumnName[];
type Values = readonly Record<ColumnName, unknown>[];
// The function takes two functions as arguments
// One to update `ColumnNames`
// One to update `Values` (optional, by default is the _identity_ function)
const updateTables =
(colFn: (columns: ColumnNames) => ColumnNames, valsFn: (vals: Values) => Values = (v) => v) =>
(tables: StudentTable[], idx: number) => {
const table = tables[idx];
table.columns = colFn(table.columns);
table.values = valsFn(table.values);
return tables;
};
Deleting columns
Let’s see how it would look to use updateTables
with deleteCol
.
Two functions are needed, one to update the columns and one to update the values:
A function that updates the columns
To update the columns collection the function needs to take the columns and return a new collection without the target column name.
I will use the filter
function from the ReadOnlyArray
module in fp-ts to remove the target column returning a collection without it.
import * as ROA from 'fp-ts/ReadonlyArray';
const removeColumnName = (target: ColumnName) => ROA.filter<ColumnName>((e) => e !== target);
A function that updates the values
To update the values collection the function needs to go over each value object and remove the field that is associated with the target column.
I will use the map
function to go over each value and omit
to remove the field from the object:
import * as STR from 'fp-ts-std/Struct'; // fp-ts-std library
const removeColumnValue = (target: ColumnName) => ROA.map(STR.omit([target]));
With both functions created, I can now change the declaration for deleteCol
to:
const deleteCol: ChangeMapFn<RemoveColumn> = ({ target }) =>
updateTables(removeColumnName(target), removeColumnValue(target));
Adding and renaming columns
Similarly, I can use updateTables
for both addCol
and renameCol
:
const renameColumnValues = (oldName: string, newName: string) => ROA.map(STR.renameKey(oldName)(newName));
// flow composes functions from right to left
// returning a function/lambda that takes a collection and returns
// a collection is the same as composing filter + append
const renameColumName = (oldName: ColumnName, newName: ColumnName) =>
flow(
ROA.filter<ColumnName>((e) => e !== oldName),
ROA.append(newName)
);
const renameCol: ChangeMapFn<RenameColumn> = ({ newName, oldName }) =>
updateTables(renameColumName(oldName, newName), renameColumnValues(oldName, newName));
const addCol: ChangeMapFn<AddColumn> = ({ target }) => updateTables(ROA.append(target));
And voila! Now the functions are more descriptive and much easier to understand.
Putting it all together
The last bit is to combine all the functions created so far.
Here is the original code, I moved the functions outside the scope of the function body for clarity:
const changeExistingStudentsTable = (tableInfo: StudentsTable[], changes: readonly ColumnChange[], idx: number) => {
const modifyData = (cur: any, op: any) => op(cur);
// This `pipe` takes the changes, filters the ones that are a change (not None).
// creates a function and then applies it to the table.
return pipe(
columnChanges,
A.map(legacyToChange), // I added this
A.filter(isChange),
A.map(getChangeFunc),
A.reduce(tableInfo, modifyData),
);
};
I like the general idea of changeExistingStudentsTable
(except the name, I’ll change that later). There is a collection of changes that need to be applied to a StudentTable
to
obtain a new StudentTable
.
That idea pretty much looks like a reduce
because the changes are folded into an existing table to create
a new version.
The steps are:
- Convert legacy change into a
ColumnChange
. - Filter changes that are
none
. - For every change create a function that will apply the change to a table.
- Take the collections of functions to apply and fold them into the table creating an updated table.
I feel the code is succinct and to the point, I’ll leave it as it is for now.
Filtering not valid changes
To filter the valid changes I need a predicate that returns true when the type of the change
is not none
. Something like:
function isValidChange(change: ColumnChange): Boolean ....
However, using a plain predicate won’t suffice:
pipe(
...
ROA.filter(isValidChange), // returns a collection of ColumnChange
ROA.map(changeToUpdateFn), // changeToUpdateFn takes a ValidColumChange not a ColumnChange
...
)
To fix it, I would like to use a TS type guard to help return the matching type:
function isValidChange(change: ColumnChange): change is ValidColumnChange {
return change.type != 'none';
}
The big enchilada
Putting all together, renaming the function to be a bit more descriptive and using the previous refactored functions:
const applyChangesToTable = (tables: StudentTable[], changes: readonly ColumnChange[], idx: number) => {
return pipe(
changes,
ROA.flatMap(legacyToChange), // One legacy to many changes, thus I need to _flatten_
ROA.filter(isValidChange), // Thanks to the type predicate, all the changes ar ValidColumChange
ROA.map(changeToUpdateFn), // Convert each change into a function to do the update
ROA.reduce(tables, (acc: StudentTable[], updateFn: UpdateFn) => updateFn(table, idx)) // apply each update
);
};
Summary
No good refactoring should happen without answering the question “Is this better than before?”.
The code is larger than the original. The amount of lines is more than double.
We started with lots of declarations that used the type any
and worked our way out to:
- Use types to describe the concepts in the domain: This is a huge benefit when reading the code.
- Make invalid combinations impossible: The type predicate will return only valid changes.
- Use small functions with clear goals: Simpler to read and follow.
- Avoid encoding logic into the changes: Each change now is separate and is only data.
- Created helper functions to capture common functionality: Is similar to having a DSL (domain specific language)
For all these reasons I think the code is better than before and the refactoring is worth it.
Here is all the code together:
type Student = Record<string, unknown>;
type ColumnName = string;
type ColumnNames = readonly ColumnName[];
type Values = readonly Student[];
type StudentsTable = {
columns: ColumnNames;
values: Values;
};
// Possible changes
type RemoveColumn = { type: 'remove'; target: ColumnName };
type AddColumn = { type: 'add'; target: ColumnName };
type RenameColumn = { type: 'rename'; newName: ColumnName; oldName: ColumnName };
type ValidColumnChange = RemoveColumn | AddColumn | RenameColumn;
// We should consider a change that does nothing
type NoChange = { type: 'none' };
type ColumnChange = ValidColumnChange | NoChange;
// A function to update one of the tables by index
type UpdateFn = (tables: StudentsTable[], idx: number) => StudentsTable[];
// A function to convert a change into an updating function
type ChangeMapFn<T extends ValidColumnChange> = (change: T) => UpdateFn;
// Helper function to create an updating function for columns and values
const updateTables =
(colFn: (columns: ColumnNames) => ColumnNames, valsFn: (vals: Values) => Values = (v) => v) =>
(tables: StudentsTable[], idx: number) => {
const table = tables[idx];
table.columns = colFn(table.columns);
table.values = valsFn(table.values);
return tables;
};
// Helper function that removes a column name from the list of columns
const removeColumnName = (target: ColumnName) => ROA.filter((e) => e !== target);
// Helper function that removes a column value from the list of values
const removeColumnValue = (target: ColumnName) => ROA.map(STR.omit([target]));
// Helper function that renames the column in the list of values
const renameColumnValues = (oldName: string, newName: string) => ROA.map(STR.renameKey(oldName)(newName));
// Helper function that renames a column name
const renameColumName = (oldName: ColumnName, newName: ColumnName) =>
flow(
ROA.filter<ColumnName>((e) => e !== oldName),
ROA.append(newName)
);
// The actual function that will apply the RemoveColumn change
const deleteCol: ChangeMapFn<RemoveColumn> = ({ target }) =>
updateTables(removeColumnName(target), removeColumnValue(target));
// The actual function that will apply the RenameColumn change
const renameCol: ChangeMapFn<RenameColumn> = ({ newName, oldName }) =>
updateTables(renameColumName(oldName, newName), renameColumnValues(oldName, newName));
// The actual function that will apply the AddColumn change
const addCol: ChangeMapFn<AddColumn> = ({ target }) => updateTables(ROA.append(target));
// Converts a change into an updating function
const changeToUpdateFn = (change: ValidColumnChange): UpdateFn =>
match(change)
.with({ type: 'add' }, addCol)
.with({ type: 'rename' }, renameCol)
.with({ type: 'remove' }, deleteCol)
.exhaustive();
// Type predicate to check if a change is valid
function isValidChange(change: ColumnChange): change is ValidColumnChange {
return change.type != 'none';
}
// constructors for change types
const createDeleteColumn = (target: ColumnName): RemoveColumn => ({ type: 'remove', target });
const createAddColumn = (target: ColumnName): AddColumn => ({ type: 'add', target });
const createRenameColumn = (newName: ColumnName, oldName: ColumnName): RenameColumn => ({ type: 'rename', newName, oldName });
const noChange = { type: 'none' };
// Converts a legacy change to a well-defined change
function legacyToChange(change: LegacyColumnChange): readonly ColumnChange[] {
const noChanges = [noChange];
if (mergedCol && original) {
return [createDeleteColumn(mergedCol), createRenameColumn(mergedCol, current!)];
}
if (deleted && original) {
return [createDeleteColumn(original)];
}
if (!original && current) {
return [createAddColumn(current)];
}
if (original != current) {
return [createRenameColumn(current!, original!)];
}
return noChanges;
}
// The main function that applies the changes to the table
export const applyChangesToTable = (tables: StudentsTable[], changes: readonly ColumnChange[], idx: number) => {
return pipe(
changes,
ROA.flatMap(legacyToChange),
ROA.filter(isValidChange),
ROA.map(changeToUpdateFn),
ROA.reduce(tables, (table: StudentsTable[], fn: UpdateFn) => fn(table, idx))
);
};
Afterthoughts
A good practice after refactoring is to review the code looking for parts that could be written better. I have a couple of thoughts.
Why use a collection of StudentTable
and an index?
It seems that the code works with one table at a time, perhaps instead of working with
a collection only one StudentTable
could be used and the main function could return the table updated.
type UpdateFn = (table: StudentTable) => StudentTable;
export const applyChangesToTable = (table: StudentsTable, changes: readonly ColumnChange[]) => {
return pipe(
changes,
...
ROA.reduce(table, (table: StudentsTable[], fn: UpdateFn) => fn(table))
);
};
Filtering changes may not be necessary
When converting LegacyColumnChange
into a collection of ColumnChange
instead of returning
NoChange
the function could return an empty collection instead. Because all the results will
be concatenated at the end an empty collection will not affect the result.
function legacyToChange(change: LegacyColumnChange): readonly ValidColumnChange[] {
// The logic here dos not change
return [];
}
// That means that there's no need to filter
export const applyChangesToTable = (tables: StudentsTable[], changes: readonly ColumnChange[], idx: number) => {
return pipe(
changes,
ROA.flatMap(legacyToChange), // no need to filter after
ROA.map(changeToUpdateFn),
...
);
};
Resources
If you wish to play with the code I have created a public repl.it that can be forked.
Do you have questions or comments? Feel free to drop me a line.
Enjoy!