Simplify if statements by eliminating repetition

Question

Simplify if statements by eliminating repetition

I have been tasked with refactoring the code below and I have already done so (check the image for reference). However, my supervisor is still not satisfied with the changes, LOL.

const { appTargetId, appUserTargetId, appUserId } = buildIndexKeys(input);
    const fromDate = moment(input.requestDate)
      .subtract(retention, 'days')
      .toISOString();

    if (input.targetId && !input.userId) {
      // code for target id without user id
      let query = this.model
        .query('appTargetId')
        .eq(appTargetId)
        .where('createDate')
        .ge(fromDate)
        .where('status')
        .not()
        .eq(NotificationStatus.Delete);
      /* istanbul ignore next */
      if (input.subApplicationId) {
        query = query.filter('subApplicationId').eq(input.subApplicationId);
      }
      return query.exec();
    } else if (input.userId && !input.targetId) {
      // code for user id without target id
      return this.model
        .query('appUserId')
        .eq(appUserId)
        .where('createDate')
        .ge(fromDate)
        .where('status')
        .not()
        .eq(NotificationStatus.Delete)
        .exec();
    } else {
      // code for both user id and target id
      return this.model
        .query('appUserTargetId')
        .eq(appUserTargetId)
        .where('createDate')
        .ge(fromDate)
        .where('status')
        .not()
        .eq(NotificationStatus.Delete)
        .exec();
    }

https://i.sstatic.net/JFgJ7.png

Is there another way I could rewrite this code? I've spent countless hours trying to optimize it without much success. Any suggestions for improvement are highly appreciated.

javascript typescript if-statement

Answer 1

Answer №1

In my opinion, this strikes a perfect balance between brevity and clarity.

When it comes to 1) pair of arrows, I find the index/indexstring variables to be quite cumbersome, so I opted to incorporate them into the if statement.

Regarding 2), I personally believe that the .where().not().eq() chains and similar structures are much easier to comprehend when condensed into a single line rather than being spread out.

As for 3), it can be streamlined into a single return statement.

const { appTargetId, appUserTargetId, appUserId } = buildIndexKeys(input);
const fromDate = moment(input.requestDate)
   .subtract(retention, 'days').toISOString();

// Since the purpose of this function is query-building, it needs to be accessible throughout.
let query;

// Using single ifs enhances clarity
if (input.targetId) {
   if (input.userId) {
      query = this.model.query('appUserTargetId').eq(appUserTargetId);
   } else {
      query = this.model.query('appTargetId').eq(appTargetId);
   }
} else {
   query = this.model.query('appUserId').eq(appUserId);
}

// This segment is common
query = query
   .where('createDate').ge(fromDate)
   .where('status').not().eq(NotificationStatus.Delete);

// It's uncertain whether this is dependent on all 3 conditions or just subApplicationId would suffice
/* istanbul ignore next */
if (input.subApplicationId && input.targetId && !input.userId) {
   query = query.filter('subApplicationId').eq(input.subApplicationId);
}

// Perform the query
return query.exec();

Answer 2

In my opinion, this strikes a perfect balance between brevity and clarity.

When it comes to 1) pair of arrows, I find the index/indexstring variables to be quite cumbersome, so I opted to incorporate them into the if statement.

Regarding 2), I personally believe that the .where().not().eq() chains and similar structures are much easier to comprehend when condensed into a single line rather than being spread out.

As for 3), it can be streamlined into a single return statement.

const { appTargetId, appUserTargetId, appUserId } = buildIndexKeys(input);
const fromDate = moment(input.requestDate)
   .subtract(retention, 'days').toISOString();

// Since the purpose of this function is query-building, it needs to be accessible throughout.
let query;

// Using single ifs enhances clarity
if (input.targetId) {
   if (input.userId) {
      query = this.model.query('appUserTargetId').eq(appUserTargetId);
   } else {
      query = this.model.query('appTargetId').eq(appTargetId);
   }
} else {
   query = this.model.query('appUserId').eq(appUserId);
}

// This segment is common
query = query
   .where('createDate').ge(fromDate)
   .where('status').not().eq(NotificationStatus.Delete);

// It's uncertain whether this is dependent on all 3 conditions or just subApplicationId would suffice
/* istanbul ignore next */
if (input.subApplicationId && input.targetId && !input.userId) {
   query = query.filter('subApplicationId').eq(input.subApplicationId);
}

// Perform the query
return query.exec();

Answer 3

Answer №2

Here's a more optimized version with simplified code structure:

const { appTargetId, appUserTargetId, appUserId } = buildIndexKeys(input);

const fromDate = moment(input.requestDate)
      .subtract(retention, 'days')
      .toISOString();
const index = (input.targetId ? (input.userId ? {"appUserTargetId" : appUserTargetId} : {"appTargetId": appTargetId}) : {"appUserId" : appUserId};

let query = this.model
        .query(Object.keys(index)[0])
        .eq(Object.values(index)[0])
        .where('createDate')
        .ge(fromDate)
        .where('status')
        .not()
        .eq(NotificationStatus.Delete);
if (input.subApplicationId) {
    query = query.filter('subApplicationId').eq(input.subApplicationId);
}

return query.exec();

If the reviewer prefers avoiding single line ternary operators, consider one of the following options:

(targetId && userId && {"appUserTargetId" : appUserTargetId}) || (targetId && {'appTargetId' : appTargetId}) || (userId && {'appUserId': appUserId})

[OR]

Implement Object Oriented Javascript (OOJS) by creating a base class and 3 extended classes, each corresponding to a specific if case. Here is a resource for learning the basics of OOJS:

https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Objects/Object-oriented_JS

Answer 4

Here's a more optimized version with simplified code structure:

const { appTargetId, appUserTargetId, appUserId } = buildIndexKeys(input);

const fromDate = moment(input.requestDate)
      .subtract(retention, 'days')
      .toISOString();
const index = (input.targetId ? (input.userId ? {"appUserTargetId" : appUserTargetId} : {"appTargetId": appTargetId}) : {"appUserId" : appUserId};

let query = this.model
        .query(Object.keys(index)[0])
        .eq(Object.values(index)[0])
        .where('createDate')
        .ge(fromDate)
        .where('status')
        .not()
        .eq(NotificationStatus.Delete);
if (input.subApplicationId) {
    query = query.filter('subApplicationId').eq(input.subApplicationId);
}

return query.exec();

If the reviewer prefers avoiding single line ternary operators, consider one of the following options:

(targetId && userId && {"appUserTargetId" : appUserTargetId}) || (targetId && {'appTargetId' : appTargetId}) || (userId && {'appUserId': appUserId})

[OR]

Implement Object Oriented Javascript (OOJS) by creating a base class and 3 extended classes, each corresponding to a specific if case. Here is a resource for learning the basics of OOJS:

https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Objects/Object-oriented_JS

Answer 5

Answer №3

Give this a shot:

    const { targetAppId, userAppTargetId, userIdApp } = createIndexKeys(input);
    const fromDate = moment(input.requestDate)
      .subtract(retention, 'days')
      .toISOString();

    let indexString;
    let index;

    if (input.targetId && !input.userId) {
      // with target id but no user id
      indexString = 'targetAppId';
      index = targetAppId;
      /* istanbul ignore next */
    } else if (input.userId && !input.targetId) {
      // with user id but no target id
      indexString = 'userIdApp';
      index = userIdApp;
    } else {
      // user id and target id present
      indexString = 'userAppTargetId';
      index = userAppTargetId;
    }

    let query;
    if (input.subAppId) {
      query = query.filter('subAppId').eq(input.subAppId);
    } else {
      query = this.database
          .query(indexString)
          .eq(index)
          .where('creationDate')
          .ge(fromDate)
          .where('status')
          .not()
          .eq(NotificationStatus.Delete)
    }

    return query.exec();

Answer 6

Give this a shot:

    const { targetAppId, userAppTargetId, userIdApp } = createIndexKeys(input);
    const fromDate = moment(input.requestDate)
      .subtract(retention, 'days')
      .toISOString();

    let indexString;
    let index;

    if (input.targetId && !input.userId) {
      // with target id but no user id
      indexString = 'targetAppId';
      index = targetAppId;
      /* istanbul ignore next */
    } else if (input.userId && !input.targetId) {
      // with user id but no target id
      indexString = 'userIdApp';
      index = userIdApp;
    } else {
      // user id and target id present
      indexString = 'userAppTargetId';
      index = userAppTargetId;
    }

    let query;
    if (input.subAppId) {
      query = query.filter('subAppId').eq(input.subAppId);
    } else {
      query = this.database
          .query(indexString)
          .eq(index)
          .where('creationDate')
          .ge(fromDate)
          .where('status')
          .not()
          .eq(NotificationStatus.Delete)
    }

    return query.exec();

Simplify if statements by eliminating repetition

Answer №1

Answer №2

Answer №3

Similar questions

Determine the specific button that was clicked within a React component

What is the best way to execute a .js file with a Mocha suite repeatedly within a loop?

The function window.open when using the target '_blank' will launch a fresh browser window

Exploring Blob functionality in TypeScript?

Techniques for animating background image using jQuery

Live JSON sign-up feature on site

The closeOnClickOutside feature seems to be malfunctioning in the angular-2-dropdown-multiselect plugin

Changing the structure of a webpage in real-time and inserting new elements into the document

Confusion about the unwinding of the call stack in the Graph Depth-

Select a random index and modify it until all unique options have been exhausted, then restart the process

Obtain the output of a single controller in a different controller within the Express framework

Learn how to properly implement cookies in a fetch request within Nextjs

Tips on Avoiding Initial Click Activation

Creating a switch statement that evaluates the id of $(this) element as a case

Potential issue of excessive memory usage in node.js when running an Express server with PM2

The email provided is invalid according to the response received from the Campaign Monitor API within a Meteor JS

Can you explain the concept of a framework operating "on top of" node.js in a way that would be easy for a beginner to understand?

What causes my absolutely positioned element to disappear when using overflow-x: hidden?

Encountering a non-constructor error while trying to import packages in React Typescript

Customizing Material UI Components: Implementing the onChange Method