TransWikia.com

typescript : clean code remove or decrease alot if's

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

One Answer

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:

  • A function which, when called, returns the Promise
  • A function that checks if the result passes or fails the test
  • An error message to throw if the test was failed

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 Errors 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

Add your own answers!

Ask a Question

Get help from others!

© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP