Complex statement in typescript get?

Question

I have some get function like this

 get customersAreNotValid(): boolean {
 let customerIsValid = false;
 if (this.customersOverview.mainCustomer) {
 if (!this.customersOverview.mainCustomer.company) {
 const { companyName, vatNumber, box, ...customer } = this.customersOverview.mainCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 });
 } else {
 const { box, ...customer } = this.customersOverview.mainCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 });
 }
 }
 
 if (this.customersOverview.additionalCustomer) {
 if (!this.customersOverview.additionalCustomer.company) {
 const { companyName, vatNumber, box, ...customer } = this.customersOverview.additionalCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 });
 } else {
 const { box, ...customer } = this.customersOverview.additionalCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 });
 }
 }
 return customerIsValid;
 }

But I don't think that this is a good approach :( Any help about refactor?

• Toby Speight: The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles.

Answer 1

Your code could be simplified to:

get customersAreNotValid(): boolean {
 const customers = [
 this.customersOverview.mainCustomer,
 this.customersOverview.additionalCustomer,
 ]
 return customers.some(oneCustomer => {
 const { companyName, vatNumber, box, ...customer } = oneCustomer;
 return Object.values(customer)
 .some(value => value == null || value === '')
 })
}

And here's how it was done:

Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
})
// to
customerIsValid = Object.values(customer).some(value => value == null || value === '')

You aren't really using array.some() properly. In your case, you're just using it to loop, but you're not really using how it checks if some values are what they say they are. array.some() already returns a boolean, you just need to assign that result to customerIsValid instead of manually doing it. Additional bonus: use == null to check for both undefined and null.

if (this.customersOverview.additionalCustomer) {
 if (!this.customersOverview.additionalCustomer.company) {
 // 1
 } else {
 // 2
 }
}
// to
if (this.customersOverview.additionalCustomer) {
 if (this.customersOverview.additionalCustomer.company) {
 // 2
 } else {
 // 1
 }
}

A minor nitpick, I recommend flipping conditionals so that it's all checking for truthiness instead of falsiness (as hinted by the presence of !). This way, it's easier to spot relationships that are really "and" and making it easier to collapse the condition to a condition1 && condition2 whenever possible. In this case, collapsing wasn't applied (the first level if doesn't have an else).

The same goes for methods and variables. Prefer positive-sided so that you don't have to do mind-bending is and is-not flipping in code.

const { companyName, vatNumber, box, ...customer } = this.customersOverview.additionalCustomer;
const { box, ...customer } = this.customersOverview.additionalCustomer;
// to
const { companyName, vatNumber, box, ...customer } = someCustomer

Destructuring can deal with first-level undefined properties. Seeing that mainCustomer is potentially a subset of additionalCustomer and that you only care about the ...customer, you can apply the same full destructure on mainCustomer. Both companyName and vatNumber are potentially undefined for mainCustomer, but that shouldn't matter because we don't care about them.

if (this.customersOverview.mainCustomer) {
 if (!this.customersOverview.mainCustomer.company) {
 
 } else {
 
 }
}
if (this.customersOverview.additionalCustomer) {
 if (!this.customersOverview.additionalCustomer.company) {
 
 } else {
 
 }
}
// to
const customers = [
 this.customersOverview.mainCustomer,
 this.customersOverview.additionalCustomer
]
const customerIsValid = customers.some(v => /* check properties of v */)

When you start repeating an operation, it's almost always a loop, an extra function, or both. So what I'd do first is find the few things that are different between the chunks of code, and extract them somewhere. Then, make the operation generic so that it accepts variable values in place of the values you extracted.

In your case, customerIsValid is true if either mainCustomer or additionalCustomer have some empty properties. This also shows another use for array.some().

get customersAreNotValid(): boolean {

Assuming mainCustomer and additionalCustomer are instances of subclasses of a base Customer class (and not just an interface to some plain object data), you could implement an isValid() method for it so that you can push off that responsibility to the customer object instead. That would simplify things for your cusotmersAreNotValid

get customersAreNotValid(): boolean {
 const customers = [
 this.customersOverview.mainCustomer,
 this.customersOverview.additionalCustomer,
 ]
 return customers.some(customer => !customer.isValid())
}

• Jörg W Mittag: Note that, based on the defensive checks in the OP's code, either or both of mainCustomer and additionalCustomer might be undefined, which in turn means that oneCustomer might be undefined, which might mean that companyName, vatNumber, and box don't exist, which TypeScript correctly complains about with a type error 'property "companyName" does not exist on type "Customer | undefined"'. At runtime, this will lead to a TypeError 'Right side of assignment cannot be destructured'. The same applies to customer and isValid in the final refactoring.
• Jörg W Mittag: In your first refactoring, you could do the destructuring of oneCustomer directly in the parameter list of the fat arrow lambda, which would then leave you with only a single expression in the body and thus allow you to use the brace-less expression body form: return customers.some(({ companyName, vatNumber, box, ...customer }) => Object.values(customer).some(value => value == null || value === ''));.

Answer 2

Note

@Joseph's answer seems to be perfect and it totally addressed the refactoring we're trying to achieve in the optimal way.

That said, imagine if the nested if statements did not share so much of common code, but you'd want to validate the customers through completely different sets of validation rules.

This is where the below solution would come in handy.

Alternative solution - Chain of Responsibility

The nested ifs seem to be a chain of validation rules, making it a perfect example to implement the Chain of Responsibility design pattern.

So, let's start by modelling the solution into 2 large categories of validation rules:

• Validation rules for main customers.
• Validation rules for addittional customers.

Handlers Contract

Abiding by the CoR we are implementing a common contract for all our validation handlers. That is:

export abstract class BaseHandler {
 public nextHandler: BaseHandler;
 setNext(nextHandler: BaseHandler): void {
 this.nextHandler = nextHandler;
 }
 abstract handle(data: any): boolean;
}

As @Joseph noted on the previous answer, conditionals that indicate correctness instead of falsiness are much more readable/ understandable. So, as a first step to implement our concrete handlers we will be refactoring the if statements as:

Main Customers

 if (this.customersOverview.mainCustomer) {
 if (this.customersOverview.mainCustomer.company) {
 const { box, ...customer } = this.customersOverview.mainCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 });
 } else {
 const { companyName, vatNumber, box, ...customer } = this.customersOverview.mainCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 }) ;
 }
 }

Additional Customers

 if (this.customersOverview.additionalCustomer) {
 if (this.customersOverview.additionalCustomer.company) {
 const { box, ...customer } = this.customersOverview.additionalCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 });
 } else {
 const { companyName, vatNumber, box, ...customer } = this.customersOverview.additionalCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 });
 }
 }

Concrete Handlers

For our concrete handlers, we are going to implement the execute method by just adding into it the logic of every if statement.

Again, please note that the code could be extracted on the BaseHandler abstract class since it can be re-used for all the handlers -- trying to make a point if the validation rules were different.

So:

export class AdditionalCustomerCompanyHandler extends BaseHandler {
 handle(data: any): boolean {
 let customersOverview: any = data;
 let customerIsValid: boolean = false;
 const { companyName, vatNumber, box, ...customer } = customersOverview.additionalCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 });
 return customerIsValid;
 }
}

export class AdditionalCustomerNoCompanyHandler extends BaseHandler {
 handle(data: any): boolean {
 let customersOverview: any = data;
 let customerIsValid: boolean = false;
 const { companyName, vatNumber, box, ...customer } = customersOverview.additionalCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 });
 return customerIsValid;
 }
}

export class MainCustomerCompanyHandler extends BaseHandler {
 handle(data: any): boolean {
 let customersOverview: any = data;
 let customerIsValid: boolean = false;
 if (customersOverview.mainCustomer.company) {
 const { box, ...customer } = customersOverview.mainCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 });
 }
 return customerIsValid;
 }
}

export class MainCustomerNoCompanyHandler extends BaseHandler {
 handle(data: any): boolean {
 let customersOverview: any = data;
 let customerIsValid: boolean = false;
 const { companyName, vatNumber, box, ...customer } = customersOverview.mainCustomer;
 Object.values(customer).some(value => {
 if (value === null || value === '') {
 customerIsValid = true;
 }
 }) ;
 return customerIsValid;
 }
}

Executor

On client code, you could execute the validation rules by setting a chain of handlers (as Chain of Responsibility suggests) or just loop through the handlers and just call the execute method on each of them.

Since we're following CoR we're going to set chains of handlers for the sake of this example:

export class Executor {
 public execute(customersOverview: any): boolean {
 let isCustomerValid: boolean = false;
 if (customersOverview.mainCustomer) {
 let mainCustomerWithCompanyDefinedHandler: BaseHandler = new MainCustomerCompanyHandler();
 mainCustomerWithCompanyDefinedHandler.setNext(new MainCustomerNoCompanyHandler());
 let executionHandler: BaseHandler = mainCustomerWithCompanyDefinedHandler;
 while (executionHandler.nextHandler) {
 isCustomerValid = executionHandler.handle(customersOverview);
 executionHandler = executionHandler.nextHandler;
 }
 }
 if (customersOverview.additionalCustomer) {
 let additionalCustomerWithCompanyDefinedHandler: BaseHandler = new AdditionalCustomerCompanyHandler();
 additionalCustomerWithCompanyDefinedHandler.setNext(new AdditionalCustomerNoCompanyHandler());
 let executionHandler: BaseHandler = additionalCustomerWithCompanyDefinedHandler;
 while (executionHandler.nextHandler) {
 isCustomerValid = executionHandler.handle(customersOverview);
 executionHandler = executionHandler.nextHandler;
 }
 }
 return isCustomerValid;
 }
}

Benefits

By following CoR we can safely say that our code supports the following:

• The validation rules order, logic and availability can be changed on demand / even on runtime. This gives flexibility for adhering to business requirements / changes with agility and by avoiding ripple effects.
• Single Responsibility Principle: Every Handler class has only one reason to change - if the validation rule that implements changes.
• Open/Closed Principle: The implementation is open for extension but closed for modification. We can add new Handlers in our chains or change the order of the chain without having to alter existing code.

• Ioannis Brant-Ioannidis: You can find a rough implementation of it on GitHub: Kata- Refactor complex get where the BaseAbstractHandler will handle the recursive calling of the entire chain.