1
\$\begingroup\$

I need to refactor some pice od method for drag-and-drop angular material.

Here is a working method in production, I need suggestion for CLEAN way to write this method. I can't change var name and anything...

WHAT I TRY - I try to made small functions for every if/else, but in this case, I missing this drop event. Like this example

checkIfHasRole() {
    transferArrayItem(
      event.previousContainer.data,
      event.container.data,
      event.previousIndex,
      event.currentIndex
    );
    let lastAddedRole = event.container.data[event.previousIndex];
    if (lastAddedRole !== undefined) {
      this.userRoleCopy.push(lastAddedRole.valueOf());
    }

But, in this case, all event is missing

Here is the working method in production

drop(event: CdkDragDrop < string[] > ) {
  if (event.container.id === "delete") {
    transferArrayItem(
      event.previousContainer.data,
      event.container.data,
      event.previousIndex,
      event.currentIndex
    );
    this.userRoleCopy = this.userRole.slice();
  } else {
    if (event.previousContainer === event.container) {
      moveItemInArray(
        event.container.data,
        event.previousIndex,
        event.currentIndex
      );
    } else {
      let lastRole = Object.values(
        event.previousContainer.data[event.previousIndex]
      );
      let parentRole = JSON.parse(JSON.stringify(event.container.data));

      let hasRole = parentRole.some(rola => {
        return rola.role === String(lastRole);
      });

      if (!hasRole) {
        transferArrayItem(
          event.previousContainer.data,
          event.container.data,
          event.previousIndex,
          event.currentIndex
        );
        let lastAddedRole = event.container.data[event.previousIndex];
        if (lastAddedRole !== undefined) {
          this.userRoleCopy.push(lastAddedRole.valueOf());
        }
        this.resetList();
      }
    }
  }
}
\$\endgroup\$
3
\$\begingroup\$

Yes, this could use a refactoring

I think your instincts are correct. Those nested if-elses are hard to comprehend. And the simplest way to break it apart is by creating helper functions.

Here I go through a step-by-step refactoring. As with any refactoring, it involves a number of judgment calls.

Assumptions

First, I need to make clear one assumption, as not everything was spelled out in the question. I'm assuming that the data properties of the event's container and previousContainer nodes, as well as your this object's userRole and userRoleCopy nodes consist of arrays of objects of the form {role: 'some string'}.

InitialCode

We start with code that looks like this:

  drop: function(event)  {
    if (event.container.id === "delete") {
      transferArrayItem(
        event.previousContainer.data,
        event.container.data,
        event.previousIndex,
        event.currentIndex
      );
      this.userRoleCopy = this.userRole.slice();
    } else {
      if (event.previousContainer === event.container) {
        moveItemInArray(
          event.container.data,
          event.previousIndex,
          event.currentIndex
        );
      } else {
        let lastRole = Object.values(
          event.previousContainer.data[event.previousIndex]
        );

        let parentRole = JSON.parse(JSON.stringify(event.container.data));

        let hasRole = parentRole.some(rola => {
          return rola.role === String(lastRole);
        });

        if (!hasRole) {
          transferArrayItem(
            event.previousContainer.data,
            event.container.data,
            event.previousIndex,
            event.currentIndex
          );
          let lastAddedRole = event.container.data[event.previousIndex];
          if (lastAddedRole !== undefined) {
            this.userRoleCopy.push(lastAddedRole.valueOf());
          }
          this.resetList();
        }
      }
    }
  },

This code implies that there two helper functions available: transferArrayItem and moveItemInArray, and also that the this object also has the method resetList and the properties userRole and userRoleCopy. I created dummies for these, with a wrapper I creatively (:-)) called "something". You can see it in Step 0. I of course do not expect that these helper functions do anything like your original. The goal here is to simply fix things up without changing the behavior.

Removing unnecessary clone

My first step was to remove an unnecessary clone made in this:

        let parentRole = JSON.parse(JSON.stringify(event.container.data));

        let hasRole = parentRole.some(rola => {
          return rola.role === String(lastRole);
        });

There is no advantage to cloning that parent role. The call to some does not alter the data, so we can just use the original.

Thus we can just write:

        let hasRole = event.container.data.some(rola => {
          return rola.role === String(lastRole);
        });

This minor change is available in Step 1, and you can see just by switching tabs that the output remains the same.

Tweaking the search

This code seems odd:

        let lastRole = Object.values(
          event.previousContainer.data[event.previousIndex]
        );

        let hasRole = event.container.data.some(rola => {
          return rola.role === String(lastRole);
        });

Perhaps my initial assumption was wrong. But if you're trying to see if that data object has an element whose role property matches the specified value, I believe it's much simpler. This is available in Step 2.

        let lastRole = event.previousContainer.data[event.previousIndex].role

        let hasRole = event.container.data.some(rola => {
          return rola.role === lastRole;
        });

Stop repeating event.x.y

Repeating event.something.somethingElse all over the place makes the code both harder to read and harder to change. We can capture values like prevData and id from the event, and the rest of the code will be simpler. There are several ways we could do this, but I find the nicest one is with destructuring parameters. Using that, we can simplify the code to version Step 3:

  drop: function ({
    container, container: {id, data},
    previousContainer, previousContainer: {data: prevData},
    previousIndex,
    currentIndex
  }) {
    if (id === "delete") {
      transferArrayItem(prevData, data, previousIndex, currentIndex);
      this.userRoleCopy = this.userRole.slice();
    } else {
      if (previousContainer === container) {
        moveItemInArray(data, previousIndex, currentIndex);
      } else {
        let hasRole = pluck('role', data).includes(prevData[previousIndex].role)

        if (!hasRole) {
          transferArrayItem(prevData, data, previousIndex, currentIndex);
          let lastAddedRole = data[previousIndex];
          if (lastAddedRole !== undefined) {
            this.userRoleCopy.push({...lastAddedRole});
          }
          this.resetList();
        }
      }
    }
  }

Using helper functions

I'm still bothered by this bit:

        let lastRole = prevData[previousIndex].role

        let hasRole = data.some(rola => {
          return rola.role === lastRole;
        });

If we were working with ['role 1', 'role 2', 'role 3', ...], it would be very easy to search for our target role by just calling includes. I happen to have handy a helper function that transforms [{x: 'a', y: 1}, {x: 'b', y: 2}, {x: 'c', y: 3}...] into ['a', 'b', 'c'], a function called pluck. In fact it's built into the Ramda REPL I've been using, but we could write our own as easily as this:

const pluck = (prop) => (xs) => xs.map(x => x[prop])

and use it as in Step 4, like this:

        let hasRole = pluck('role', data).includes(prevData[previousIndex].role)

And now that we can extract helpers, we can do a substantial clean-up of our main function by adding a few helper methods to our object:

  deleteRole: function (prevData, data, previousIndex, currentIndex) {
    transferArrayItem(prevData, data, previousIndex, currentIndex);
    this.userRoleCopy = this.userRole.slice();
  },
  transferRole: function(prevData, data, previousIndex, currentIndex) {
    if (!pluck('role', data).includes(prevData[previousIndex].role)) {
      transferArrayItem(prevData, data, previousIndex, currentIndex);
      if (previousIndex in data) {
        this.userRoleCopy.push({...data[previousIndex]});
      }
      this.resetList();
    }
  },
  drop: function ({
    container, container: {id, data},
    previousContainer, previousContainer: {data: prevData},
    previousIndex,
    currentIndex
  }) {
    if (id === "delete") {
      this.deleteRole(prevData, data, previousIndex, currentIndex);
    } else {
      if (previousContainer === container) {
        moveItemInArray(data, previousIndex, currentIndex);
      } else {
        this.transferRole(prevData, data, previousIndex, currentIndex);
      }
    }
  }

(This is Step 5.)

Moving back to an event parameter

As we've proceeded, it seems as though the decision to destructure our event parameter is actually complicating things. Certainly the signature of our main function is not as nice as we'd like. If we undo that, now that we've abstracted things a bit, using these property accesses doesn't seem so ugly.

And this would let us move some of the helpers from object methods to plain functions, which are often more tractable. There is a problem with the this property though, which would necessitate moving this line back into the main function:

   this.userRoleCopy = this.userRole.slice();

With this the code now could look like Step 6:

const pluck = (prop) => (xs) => xs.map(x => x[prop])

const transferEvent = (event) =>
  transferArrayItem(event.previousContainer.data, event.container.data, event.previousIndex, event.currentIndex);

const moveEvent = (event) => 
  moveItemInArray(event.container.data, event.previousIndex, event.currentIndex)

const something = (userRole) => ({
  // ...
  transferRole: function(event) {
    if (!pluck('role', event.container.data).includes(
      event.previousContainer.data[event.previousIndex].role)
     ) {
      transferEvent(event);
      if (event.previousIndex in event.container.data) {
        this.userRoleCopy.push({...event.container.data[event.previousIndex]});
      }
      this.resetList();
    }
  },
  drop: function (event) {
    if (event.container.id === "delete") {
      transferEvent(event);
      this.userRoleCopy = this.userRole.slice();
    } else {
      if (event.previousContainer === event.container) {
        moveEvent(event);
      } else {
        this.transferRole(event);
      }
    }
  }
})

Making plain functions out of methods

The reason we haven't been able to move transferRole into a plain function as well is that it makes reference to this. So long as we need this, we're stuck with methods. But it's quite easy to make it a function parameter. When we do so we end up with a final cleaner codebase, Step 7:

const pluck = (prop) => (xs) => xs.map(x => x[prop])

const transferEvent = (event) =>
  transferArrayItem(event.previousContainer.data, event.container.data, event.previousIndex, event.currentIndex);

const moveEvent = (event) => 
  moveItemInArray(event.container.data, event.previousIndex, event.currentIndex)

const transferRole = (something, event) => {
  if (!pluck('role', event.container.data).includes(
    event.previousContainer.data[event.previousIndex].role)
  ) {
    transferEvent(event);
    if (event.previousIndex in event.container.data) {
      something.userRoleCopy.push({...event.container.data[event.previousIndex]});
    }
    something.resetList();
  }
};

const something = (userRole) => ({tion: 'resetList'}),
  // ...
  drop: function (event) {
    if (event.container.id === "delete") {
      transferEvent(event);
      this.userRoleCopy = this.userRole.slice();
    } else {
      if (event.previousContainer === event.container) {
        moveEvent(event);
      } else {
        transferRole(this, event);
      }
    }
  },
})

Lessons Learned

I was going to write a few paragraphs about choices made during this refactoring, and about alternatives that might apply. But I'm tired now, and want to finish up. So I'll just say although there are many good reasons to refactor code -- including performance, compliance with conventions and standards, and others -- the most important reason is to make your code easier to read and to modify.

I believe the final version makes the branching much more clear. I believe it is easier to read. So I believe we have improved this code.


Note: I'm seeing after writing all this up that we folded two separate changes into Step 5. As well as pulling out helper functions, that step changed this:

          let lastAddedRole = data[previousIndex];
          if (lastAddedRole !== undefined) {
            this.userRoleCopy.push({...lastAddedRole});
          }

to this:

      if (previousIndex in data) {
        this.userRoleCopy.push({...data[previousIndex]});
      }

It does the same thing, but without introducing a temporary variable. To me that's usually an improvement.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Than you soo much... It's helped me a lot \$\endgroup\$ – Arter Dec 21 '19 at 6:25
1
\$\begingroup\$

My first question seeing your code is if all conditions are mutually exclusive. In other words, do you want the code inside the else block to be used if the if condition is not met? Or it would be fine to go to the else code regardless if the first condition was met.

I also recommend following these guidelines to avoid this kind of code flattening arrow code

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

There are some parts you could split out that only need event as a parameter.

transferArrayItem(
  event.previousContainer.data,
  event.container.data,
  event.previousIndex,
  event.currentIndex
);

could be wrapped in something that takes the dragdrop event as a parameter.

I don't quite understand what you're doing in checkIfHasRole() - it seems as if you've put everything (except this.resetList()) within if (!hasRole) into a function named as if it performed the hasRole checks in the previous lines?

You could turn hasRole into a function containing

  let lastRole = Object.values(
    event.previousContainer.data[event.previousIndex]
  );
  let parentRole = JSON.parse(JSON.stringify(event.container.data));

  return parentRole.some(rola => rola.role === String(lastRole));

The way lastRole is accessed seems confusing. Can you log event.previousContainer.data[event.previousIndex] to see why on earth that's accessed through String(Object.values())?

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.