Refactoring a method containing conditional with extremely different code blocks that are the same ;)

Question

So I have this stinky method, the two conditional blocks do almost the exact same thing, but with radically different parameters (at least in my eyes). I want to clean it Uncle Bob style, but can't for the life of me figure out a tidy way of doing it. So I come to you, my fabulous nerd friends, to see how you might distill this down to something that doesn't make one want to gouge out their eyes. The code is AS3, but that doesn't really make a difference in my opinion.

/**
 * Splits this group into two groups based on the intersection of the group
 * with another group. The group is split in a direction to fill empty
 * cells left by the splitting group.
 *
 * @param onGroup
 * @param directionToMoveSplitCells
 * @return
 *
 */
public function split(onGroup:CellGroup, directionToMoveSplitCells:String):CellGroup
{
 if (!hasIntersection(onGroup))
  return this;
 var numCellsToSplit:int = 0;
 var splitCells:Array;
 var newGroup:CellGroup;
 var numberOfCellsToSplit:int;
 var splitStartIndex:int;
 var resultingGroupStartIndex:int;

 if (directionToMoveSplitCells == "RIGHT")
 {
  numberOfCellsToSplit = endIndex - onGroup.startIndex + 1;
  splitStartIndex = length - numberOfCellsToSplit;
  splitCells = trimCells(splitStartIndex, numberOfCellsToSplit);
  resultingGroupStartIndex = onGroup.endIndex + 1;

  if (splitCells.length > 0)
  {
   newGroup = row.createGroup(splitCells, resultingGroupStartIndex)
   newGroup.nextGroup = nextGroup;
   if (newGroup.nextGroup)
    newGroup.nextGroup.previousGroup = newGroup;
   newGroup.previousGroup = this;
   nextGroup = newGroup;

  }
 }
 else
 {
  numberOfCellsToSplit = onGroup.endIndex - startIndex + 1;
  splitStartIndex = 0;
  splitCells = trimCells(splitStartIndex, numberOfCellsToSplit);
  resultingGroupStartIndex = onGroup.startIndex - splitCells.length;

  if (splitCells.length > 0)
  {
   newGroup = row.createGroup(splitCells, resultingGroupStartIndex)
   newGroup.previousGroup = previousGroup;
   if (newGroup.previousGroup)
    newGroup.previousGroup.nextGroup = newGroup
   previousGroup = newGroup;
   newGroup.nextGroup = this;
   var newX:int = (onGroup.endIndex + 1) * cellSize.width;
   x = newX;
  }

 }

 removeArrayOfCellsFromGroup(splitCells);
 row.joinGroups();
 row.updateGroupIndices();
 repositionCellsInGroup();

 return newGroup;
}

Answer 1

var groupThatWillReceiveCells = this;
var groupThatWontReceiveCells = onGroup;

if (directionToMoveSplitCells == "RIGHT")
{
    groupThatWillReceiveCells = onGroup;
    groupThatWontReceiveCells = this;
}

Rename stuff as needed.

Answer 2

/**
 * Splits this group into two groups based on the intersection of the group
 * with another group. The group is split in a direction to fill empty
 * cells left by the splitting group.
 *
 * @param onGroup
 * @param directionToMoveSplitCells
 * @return
 *
 */
public function split(onGroup:CellGroup, directionToMoveSplitCells:String):CellGroup
{
        if(!hasIntersection(onGroup)) return this;
        var numCellsToSplit:int = 0;
        var splitCells:Array;
        var newGroup:CellGroup;
        var numberOfCellsToSplit:int;
        var splitStartIndex:int;
        var resultingGroupStartIndex:int;

  numberOfCellsToSplit = (directionToMoveSplitCells == "RIGHT" ? (endIndex - onGroup.startIndex) : (onGroup.endIndex - startIndex)) + 1;
  splitStartIndex = directionToMoveSplitCells == "RIGHT" ? (length - numberOfCellsToSplit) : 0;
  splitCells = trimCells(splitStartIndex, numberOfCellsToSplit);
  resultingGroupStartIndex = directionToMoveSplitCells == "RIGHT" ? (onGroup.startIndex - splitCells.length) : (onGroup.endIndex + 1);

  if (splitCells.length > 0)
        {
                newGroup = row.createGroup(splitCells, resultingGroupStartIndex)
                newGroup.nextGroup = nextGroup; //not sure how to not set this from jump
                newGroup.previousGroup = previousGroup; //not sure how to not set this from jump
                if (newGroup.previousGroup){
     newGroup.previousGroup.nextGroup = newGroup;
     previousGroup = newGroup;
     var newX:int = (onGroup.endIndex + 1) * cellSize.width;
                 x = newX;
    }
                if (newGroup.nextGroup) newGroup.nextGroup.previousGroup = newGroup;
    else{
     newGroup.nextGroup = this;
     newGroup.previousGroup = this;
                 nextGroup = newGroup;
    }
        }

        removeArrayOfCellsFromGroup(splitCells);
        row.joinGroups();
        row.updateGroupIndices();
        repositionCellsInGroup();

        return newGroup;
}

Answer 3

This is all that occurs to me. The linked list is really a seperate detail... so maybe it can be refactored out....

    public function split(onGroup:CellGroup, directionToMoveSplitCells:String):CellGroup
{
        if (!hasIntersection(onGroup))
                return this;
        valr splitCells:Array;
        var newGroup:CellGroup ;
        var numberOfCellsToSplit:int;
        var splitStartIndex:int;
        var resultingGroupStartIndex:int;

        if (directionToMoveSplitCells == "RIGHT")
        {
                numberOfCellsToSplit = this.endIndex - onGroup.startIndex + 1;
                splitStartIndex = this.length - numberOfCellsToSplit;
        splitCells = trimCells(splitStartIndex, numberOfCellsToSplit);
                resultingGroupStartIndex = onGroup.endIndex + 1;

                if (splitCells.length > 0)
                {
                        newGroup = row.createGroup(splitCells, resultingGroupStartIndex);
                  nextGroup=insertGroup(newGroup,this,nextGroup);
                }
        }
        else
        {
                numberOfCellsToSplit = onGroup.endIndex - startIndex + 1;
                splitStartIndex = 0;
        splitCells = trimCells(splitStartIndex, numberOfCellsToSplit);
                resultingGroupStartIndex = onGroup.startIndex - splitCells.length;

                if (splitCells.length > 0)
                {
                        newGroup = row.createGroup(splitCells, resultingGroupStartIndex);
                        previousGroup=insertGroup(newGroup,previousGroup,this);
                        var newX:int = (onGroup.endIndex + 1) * cellSize.width;
                        x = newX;
                }

        }

        removeArrayOfCellsFromGroup(splitCells);
        row.joinGroups();
        row.updateGroupIndices();
        repositionCellsInGroup();

        return newGroup;
}

private function insertGroup(toInsert:CellGroup,prior:CellGroup,next:CellGroup):CellGroup
{
    toInsert.nextGroup = next;
    toInsert.previousGroup = prior;
    if (toInsert.nextGroup )
            toInsert.nextGroup.previousGroup = toInsert;
    if (toInsert.previousGroup )
        toInsert.previousGroup.nextGroup = toInsert;
    return toInsert;
}

My unindenting of splitCells assigment is to indicate that it is the inoly non-conditional line in the block. I looked at doing what Anon proposed but I can't see any way to make the code actually better that way.

Answer 4

public function split(onGroup:CellGroup, directionToMoveSplitCells:String):CellGroup
{
 if (!hasIntersection(onGroup))
  return this;

 var newGroup:CellGroup;

    if (directionToMoveSplitCells == "RIGHT")
    {
  newGroup = splitGroupAndMoveSplitCellsRight(onGroup);
  if (!newGroup)
   return;

  insertAsNextGroupInLinkedList(newGroup);
    }
    else
    {
  newGroup = splitGroupAndMoveSplitCellsLeft(onGroup);
  if (!newGroup)
   return;

  insertAsPreviousGroupInLinkedList(newGroup);

  x = (onGroup.endIndex + 1) * cellSize.width;
 }

    removeArrayOfCellsFromGroup(splitCells);
    row.joinGroups();
    row.updateGroupIndices();
    repositionCellsInGroup();

    return newGroup;
}


private function splitGroupAndMoveSplitCellsRight(onGroup:CellGroup):CellGroup
{
    var numCellsToSplit:int = endIndex - onGroup.startIndex + 1;
    var splitStartIndex:int = length - numberOfCellsToSplit;

    var splitCells:Array = trimCells(splitStartIndex, numberOfCellsToSplit);
    if (!splitCells.length)
     return null;

    var resultingGroupStartIndex:int = onGroup.endIndex + 1;

    return row.createGroup(splitCells, resultingGroupStartIndex);
}

private function splitGroupAndMoveSplitCellsLeft(onGroup:CellGroup):CellGroup
{
    var numCellsToSplit:int = onGroup.endIndex - startIndex + 1;
    var splitStartIndex:int = 0;

    var splitCells:Array = trimCells(splitStartIndex, numberOfCellsToSplit);
    if (!splitCells.length)
     return null;

    var resultingGroupStartIndex:int = onGroup.startIndex - splitCells.length;

    return row.createGroup(splitCells, resultingGroupStartIndex);
}

private function insertAsNextGroupInLinkedList(group:CellGroup):void
{
    var currentNextGroup:CellGroup = nextGroup;
    if (currentNextGroup)
    {
     group.nextGroup = currentNextGroup;
     currentNextGroup.previousGroup = group;
    }

    group.previousGroup = this;
    nextGroup = group;
}

private function insertAsPreviousGroupInLinkedList(group:CellGroup):void
{
    var currentPreviousGroup:CellGroup = previousGroup;
    if (currentPreviousGroup)
    {
     group.previousGroup = currentPreviousGroup;
     currentPreviousGroup.nextGroup = group;
    }

    group.nextGroup = this;
    previousGroup = group;
}