Best way to refactor tricky if/else statement

Best way to refactor tricky if/else statement
0.0 0

#1

Hi all,

I have a tricky nested if else situation and wondering on the best way to refactor this. I’ve tried a few approaches building functions with ternary operators and the like, but it still feels a bit like more could be done… Thoughts, opinions and suggestions all welcome :slight_smile: Thanks!

// Work out if this asset is being touched by the selection.
if ((rectDims.x2 < iconDims.x1) || (rectDims.x1 > nameDims.x2)) {
	if ((ctrlKey === true) && (inOrigSel === true)) {
		self.addToSelection(asset);
	} else {
		self.removeFromSelection(asset);
	}
} else if ((rectDims.y2 < iconDims.y1) || (rectDims.y1 > iconDims.y2)) {
	if ((ctrlKey === true) && (inOrigSel === true)) {
		self.addToSelection(asset);
	} else {
		self.removeFromSelection(asset);
	}
} else {
	if ((ctrlKey === true) && (inOrigSel === true)) {
		self.removeFromSelection(asset);
	} else {
		self.addToSelection(asset);
	}
}//end if
				
					

#2

Here is my attempt thus far… still feels like more could be done…?

var addOrRemoveSelection = function(removeFirst) {
	var conditionToCheck = ((ctrlKey === true) && (inOrigSel === true));

	if (removeFirst) {
		return (conditionToCheck) ? self.removeFromSelection(asset) : self.addToSelection(asset);
	}
	return (conditionToCheck) ? self.addToSelection(asset) : self.removeFromSelection(asset);

}

// Work out if this asset is being touched by the selection.
if ((rectDims.x2 < iconDims.x1) || (rectDims.x1 > nameDims.x2)) {
	addOrRemoveSelection();
} else if ((rectDims.y2 < iconDims.y1) || (rectDims.y1 > iconDims.y2)) {
	addOrRemoveSelection();
} else {
	addOrRemoveSelection(true);
} // end if

#3

You don’t need to grouped conditions on either side of an || in parentheses. That would make it slightly easier to read.


#4

I just want to ask, if it is about touching/clicking an asset, couldn’t it be handled by the appropriate event listener?
Not sure what it is you’re doing in the big picture but I wondered if the right event listener would save checking certain conditions.


#5

I have not tested the following, but I think it could work.

var ctrlKeyAndInOrigSel = ctrlKey && inOrigSel;
var addSelection = rectDims.x2 < iconDims.x1 || rectDims.x1 > nameDims.x2 || rectDims.y2 < iconDims.y1 || rectDims.y1 > iconDims.y2 ? true : false;
if ((addSelection && ctrlKeyAndInOrigSel) || (!addSelection && !ctrlKeyAndInOrigSel))
  self.addToSelection(asset);
else
	self.removeFromSelection(asset);

#6

I’m also not sure what your environment is, but if you have access to the Array methods, a trick I like to use for grouping conditionals is this:

if( [ctrlKey, inOrigSel].includes(true) )

Instead of

if ((ctrlKey === true) && (inOrigSel === true))

Well, I see now that this trick only works if you want to check for either/or of those values in the array. And the alternative would be to use Array.every() which wouldn’t save any more space.

So if you want to use this trick, you can use this prototype method to check if only this value is in the array.

Array.prototype.only = function( value ){
  for( let d of this ){
    if( d !== value )
      return false;
  }
  return true;
}

if( [ctrlKey, inOrigSel].only(true) )

#7

Cool, thanks @Emgo-Dev and @randelldawson for the tips, that is exactly the kind of feedback I was after.

Yeah my code is a bit tricky to follow, but yes basically I am using it to multi select items and drag/drop them around to a new place.

Thanks, I’ll keep working on it and let you know how it goes :slight_smile: