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.
