I am joining a new company and I want to make a positive impact in regards to my code.
I was asked to do compare two fuzzy name and return if the name is valid or not.
For this I wrote the following code
import fuzzy from 'fuzzy'
const {driverName, tlcNumber} = input
if (!tlcNumber) {
throw new Error("tlcNumber not specified");
}
if (!driverName) {
throw new Error("Driver Number not specified");
}
const result = await getDriver(input.tlcNumber);
if (!result) {
return {
valid: false,
message: `TLC number ${tlcNumber} is not found in the TLC database`
}
}
const {name:tlcRegisteredName} = result
driverName.split(' ').forEach(name => {
const fuzzyMatch = fuzzy.test(name, tlcRegisteredName)
if (!fuzzyMatch) {
return {
valid: false,
message: `TLC number ${tlcNumber} is registered to ${tlcRegisteredName} which does not match ${driverName}`
}
}
});
return {
valid:true
}
Can someone help me in making this code better? or how I would Improve? I have been told to
separate first and last names and do fuzzy matching independently on each
I am using the fuzzy
library.
-
1\$\begingroup\$ It may be of interest to know that TLC stands for "Taxi and Limousine Commission", it has to do with licensing taxi cabs. Hence the talk of "drivers" and "registering". \$\endgroup\$KIKO Software– KIKO Software2022年02月24日 13:17:26 +00:00Commented Feb 24, 2022 at 13:17
1 Answer 1
I guess this code was taken a bit out of context, and might reside in a different form elsewhere. I've noticed a few things here, so here is my review:
- Everything resides in global space and the execution not really structured. Each step happens in a sequential manner, where the actual description on what happens has to be understood by looking at the sequence as a whole. Meaning, extract piece of code into (pure) functions - with proper naming.
- For software in terms of maintainability and readability it's important to be consistent even when/especially syntax wise. For example, there is inconsistent usage of semicola and access to variables is also not consistent e.g.
tlcNumber
vs.input.tlcNumber
- Extract constant wherever possible, for example
.split(' ')
, why is it an empty string? Is this some sort of name separator? - Splitting the drivers name and checking whether or not a substring of the name matches in order to return a check result could be refactored. In fact, TypeScript Playground even yelled at me, that not all codepaths return a value. Consider using functions like,
.map()
,.some()
to achieve the same. The approach is totally fine when for example, an early stop of execution is mandatory. - In general, it might be helpful to avoid abbreviations for variables as they might cause confusion. My personal preference is to use longer and self-explanatory variable names rather than some abbreviation which is not common sense.
As well I do have questions looking at the code, some of them are:
- What is
input
where does it come from? Which typings does it have? - Should this whole code be some sort of API which another person relies on? For example
doesDriverNameAndTlcDriverNameMatch({driverName: string; tlcNumber: number });
So, my refactored version of the above code would look like this:
import fuzzy from "fuzzy";
type DriverNameAndTlcNumber = {
driverName: string;
tlcNumber: number;
}
type PositiveMatch = {
valid: true
}
type NegativeMatch = {
valid: false;
message: string;
}
type MatchResult = PositiveMatch | NegativeMatch;
const NAME_SEPARATOR = ' ';
const getDriverByTlcNumber = async (driversTlcNumber: number): Promise<{name: string;}> => await getDriver(driversTlcNumber);
export const doesDriverNameAndTlcDriverNameMatch = async ({driverName, tlcNumber}: DriverNameAndTlcNumber): Promise<MatchResult> => {
if (!tlcNumber) {
throw new Error("tlcNumber not specified");
}
if (!driverName) {
throw new Error("Driver Number not specified");
}
const driver = await getDriverByTlcNumber(tlcNumber);
if (!driver) {
return {valid: false, message: `TLC number ${tlcNumber} is not found in the TLC database`};
}
const driverNameAndRegisteredNameDoNotMatch = driver.name.split(NAME_SEPARATOR)
.map(x => fuzzy.test(x, driver.name))
.some(x => x === undefined);
return driverNameAndRegisteredNameDoNotMatch ? { valid: false, message: `TLC number ${tlcNumber} is registered to ${driver.name} which does not match ${driverName}` } : { valid: true };
};