Code Review Asked by Gabriel Costa on January 5, 2021
Hello I have the following code, where I need to do many checks of calls from the database,
My TypeEnum:
export enum ProductTypes {
equipament = 'equipament',
component = 'component',
}
this is my use case class:
export class AssignProductUseCase
implements
IUseCase<AssignProductDTO, Promise<Either<AppError, ProductInstance>>> {
constructor(
@inject(EmployeeRepository)
private employeeRepository: IEmployeeRepository,
@inject(ProductRepository)
private productRepository: IProductRepository,
@inject(ProductInstanceRepository)
private instanceRepository: IProductInstanceRepository,
@inject(ProductStocksRepository)
private stockRepository: IProductStocksRepository,
) {}
public execute = async ({
contract_id,
matricula,
parent_id,
patrimony_code,
product_id,
serial_number,
type,
}: AssignProductDTO): Promise<Either<AppError, ProductInstance>> => {
//verify type
if (!(type in ProductTypes)) throw new Error(`${type}, is invalid`);
//instance checks
const hasInstance = await this.instanceRepository.bySN(serial_number);
if (hasInstance) {
throw new Error(
`This ${serial_number} product has already been assigned `,
);
}
const parentInstance = parent_id
? await this.instanceRepository.byId(parent_id)
: null;
if (parentInstance === undefined) {
throw new Error(`Parent Instance dont exist.`);
}
//products checks
const hasProduct = await this.productRepository.byId(product_id);
if (!hasProduct) throw new Error(`This product dont exist.`);
//employee checks
const hasEmployee = await this.employeeRepository.byMatricula(matricula);
if (!hasEmployee) throw new Error(`This Employee dont exist.`);
const employeeHasProduct = await this.instanceRepository.hasInstance(
product_id,
hasEmployee.id,
);
if (employeeHasProduct) {
throw new Error(
`The enrollment employee: ${matricula} already has an instance of the product.`,
);
}
//stock checks
const stock = await this.stockRepository.byContractAndProduct(
contract_id,
product_id,
);
if (!stock) {
throw new Error(
`The product has no stock, or the contract has no supply.`,
);
}
if (stock.quantity <= 1) {
throw new Error(`The stock of this product is less than or equal to 1`);
}
//create instance
const instance = await this.instanceRepository.create(ProductInstance.build(request))
return right(instance)
};
}
and with respect to repository calls are small methods using mikro orm:
Here is a repository as an example:
export class ProductInstanceRepository implements IProductInstanceRepository {
private em: EntityManager;
constructor(@inject('bootstrap') bootstrap: IBootstrap) {
this.em = bootstrap.getDatabaseORM().getConnection().em.fork();
}
public byArray = async (ids: string[]): Promise<ProductInstance[]> => {
return await this.em.find(ProductInstance, { serial_number: ids }, [
'stock',
]);
};
public create = async (
instance: ProductInstance,
): Promise<ProductInstance> => {
if (!(instance instanceof ProductInstance))
throw new Error(`Invalid Data Type`);
await this.em.persist(instance).flush();
return instance;
};
public update = async (
serial_number: string,
data: any,
): Promise<ProductInstance> => {
const instance = await this.em.findOne(ProductInstance, { serial_number });
if (!instance) throw new Error(`${data.matricula} dont exists`);
wrap(instance).assign(data);
await this.em.persist(data).flush();
return instance;
};
public all = async (pagination: Pagination): Promise<ProductInstance[]> => {
return await this.em.find(ProductInstance, {});
};
public byId = async (
serial_number: string,
): Promise<ProductInstance | undefined> => {
const instance = await this.em.findOne(ProductInstance, { serial_number });
if (!instance) return;
return instance;
};
public bySN = async (
serial_number: string,
): Promise<ProductInstance | undefined> => {
const instance = await this.em.findOne(ProductInstance, { serial_number });
if (!instance) return;
return instance;
};
public hasInstance = async (
product_id: string,
employee_id: string,
): Promise<boolean> => {
const parent = await this.em.findOne(ProductInstance, {
product: { id: product_id },
employee: { id: employee_id },
});
if (!parent) return false;
return true;
};
}
But this became a very ugly code, but I’m not getting a solution on how to refactor so much if, because they are essential checks for the logic. I would be very grateful if someone could give me tips on how I can refactor this in a cleaner way.
My logic:
I need to make sure that my type parameter is contained in my enum,
I need to check that there is already an instance of the product in the db,
I need to check if my instance has a parent_id parameter and if that product instance exists, if it is null there is no need for verification,
need to check the existence of the product and stock
While your code is somewhat verbose, it's not that bad. If you were to keep going with your current approach I think it'd be OK.
That said, the main improvement that can be observed here is that for every check, you're carrying out an asynchronous request, then throwing if the result fails a test. You can make an array of objects, with each object containing:
Then iterate over the array of objects:
const validators = [
{
fn: () => this.instanceRepository.bySN(serial_number),
errorIf: (hasInstance: boolean) => hasInstance,
errorMessage: `This ${serial_number} product has already been assigned `
},
{
fn: () => parent_id ? this.instanceRepository.byId(parent_id) : Promise.resolve(null),
errorIf: (parentInstance: unknown) => parentInstance === undefined),
errorMessage: 'Parent Instance dont exist.',
},
// ...
];
for (const { fn, errorIf, errorMessage } of validators) {
const result = await fn();
if (errorIf(result)) {
throw new Error(errorMessage);
}
}
There's some boilerplate, but it looks a lot cleaner, and the validations being performed are easier to understand at a glance.
One complication is that there is one variable which needs two checks: stock
, with separate error messages, and you don't want to make two duplicate requests for the same thing. There are a couple tricky things you could do (like assign to an outside variable in the fn
), or make a getErrorMessage
property on a validator as well:
const validateStock = (stock) => {
if (!stock) {
return `The product has no stock, or the contract has no supply.`;
}
return stock.quantity <= 1
? 'The stock of this product is less than or equal to 1'
: ''
};
const validators = [
{
fn: () => this.stockRepository.byContractAndProduct(contract_id, product_id),
errorIf: () => true,
getErrorMessage: validateStock,
},
// ...
];
for (const { fn, errorIf, getErrorMessage } of validators) {
const result = await fn();
if (errorIf(result)) {
const errorMessage = getErrorMessage(result);
if (errorMessage) {
throw new Error(errorMessage);
}
}
}
But I think that's over-engineered - I think I'd prefer the original validators
array, with everything in it that's possible given its structure, and then have two separate checks outside for validating the stock
variable.
Other potential improvements:
Promise.all
? Your current code waits for each request to finish before making the next one. Unless you have to wait in serial like this, consider making the requests in parallel instead - parallel requests will finish more quickly.
Require certain parameter types instead of checking and throwing One of the main selling points of TypeScript is turning hard runtime errors into easy compile-time errors. Instead of:
if (!(type in ProductTypes)) throw new Error(`${type}, is invalid`);
If the type
isn't completely dynamic, consider instead requiring an argument whose type is ProductTypes. Change:
} :AssignProductDTO
to
} :AssignProductDTO & { type: ProductTypes }
This will mean that calling the function with { type: 'NotARealType' }
will fail TypeScript's type-checking, but will permit { type: 'equipament' }
. Which leads to the next point:
Spelling and grammar Spelling words properly prevents bugs, makes code look more professional, and improves the user's experience. Change:
equipament
to equipment
Parent Instance dont exist
to Parent Instance doesn't exist.
This product dont exist
to This product doesn't exist.
Unused variable You never use the destructured patrimony_code
variable, so feel free to remove it from the parameter list.
Allow TS to infer types automatically when possible - it cuts down on boilerplate, typos, and other mistakes. If right(instance)
is typed properly, the whole execute
method should not need to note a return type; TypeScript will automatically infer it; you should be able to remove Promise<Either<AppError, ProductInstance>>
.
Remember that TypeScript doesn't track rejected Promise types - if AppError
is a type you're using to indicate Error
s that may be thrown by your checks inside, it shouldn't be part of the type definition.
Correct answer by CertainPerformance on January 5, 2021
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP