Refactoring: Reverse Boolean

You have a boolean variable that represents a negative condition and you want the variable to represent a positive condition.

Replace all assignments to the variable with their logical inverse and replace all uses of the variable with its logical inverse.

Motivation

When a boolean variable represents a negative condition, it is harder to reason about and read in code. For instance, suppose we have a document that contains embedded figures. We have a program that can print the document with or without figures. If we had a boolean variable called dontPrintFigures, the code might look something like this:

bool dontPrintFigures = true;
// ...
if (!dontPrintFigures)
{
    PrintDocument();
}
else
{
    PrintDocumentWithoutFigures();
}

The code is clearer to read if the boolean conditions are expressed as positive conditions:

bool printFigures = false;
// ...
if (printFigures)
{
    PrintDocument();
}
else
{
    PrintDocumentWithoutFigures();
}

Mechanics

  1. After the declaration of the negative variable, introduce a declaration for the new positive variable and assign it to the logical negation of the existing variable.
  2. After each assignment to the negative variable, insert an assignment to the positive variable with the logical negation of the negative variable.
  3. One by one, replace each use of the negative variable with the logical negation of the positive variable.
  4. After replacing each use of the negative variable, compile and test.
  5. Replace each assignment to the negative variable with an assignment statement to the positive variable using the logical negation of the original expression.
  6. Eliminate the declaration of the existing negative variable by inlining the initialization of the positive variable.

Example

You can find many examples by using google code search for C++ code containing the text “!dont”. Here is one such example from MainEdit.cpp in the passwordsafe project on sourceforge. The original code looks like this:

// Delete key was pressed (in list view or tree view) to delete an entry.
void DboxMain::OnDelete()
{
  if (m_core.GetNumEntries() == 0) // easiest way to avoid asking stupid questions...
    return;
 
  bool dontaskquestion = PWSprefs::GetInstance()->
                         GetPref(PWSprefs::DeleteQuestion);
 
  bool dodelete = true;
  int num_children = 0;
 
  if (m_ctlItemTree.IsWindowVisible()) {
    HTREEITEM hStartItem = m_ctlItemTree.GetSelectedItem();
    if (hStartItem != NULL) {
      if (m_ctlItemTree.GetItemData(hStartItem) == NULL) {  // group node
        dontaskquestion = false; // ALWAYS ask if deleting a group
        // Find number of child items
        num_children = 0;
        if (m_ctlItemTree.ItemHasChildren(hStartItem)) {
          num_children = CountChildren(hStartItem);
        }  // if has children
      }
    }
  }
 
  //Confirm whether to delete the item
  if (!dontaskquestion) {
    CConfirmDeleteDlg deleteDlg(this, num_children);
    INT_PTR rc = deleteDlg.DoModal();
    if (rc == IDCANCEL) {
      dodelete = false;
    }
  }
 
  if (dodelete) {
    Delete();
    if (m_bFilterActive)
      RefreshViews();
  }
}

Notice the variable dontaskquestion is used in a condition that tests a double negative. Further indications that this variable is confusing is that in a simple assignment to this variable, the author felt an additional comment was needed for clarity.

First, we introduce our variable stating the positive condition:

// Delete key was pressed (in list view or tree view) to delete an entry.
void DboxMain::OnDelete()
{
  if (m_core.GetNumEntries() == 0) // easiest way to avoid asking stupid questions...
    return;
 
  bool dontaskquestion = PWSprefs::GetInstance()->
                         GetPref(PWSprefs::DeleteQuestion);
  bool askQuestion = !dontaskquestion;
 
  bool dodelete = true;
  int num_children = 0;
 
  if (m_ctlItemTree.IsWindowVisible()) {
    HTREEITEM hStartItem = m_ctlItemTree.GetSelectedItem();
    if (hStartItem != NULL) {
      if (m_ctlItemTree.GetItemData(hStartItem) == NULL) {  // group node
        dontaskquestion = false; // ALWAYS ask if deleting a group
        // Find number of child items
        num_children = 0;
        if (m_ctlItemTree.ItemHasChildren(hStartItem)) {
          num_children = CountChildren(hStartItem);
        }  // if has children
      }
    }
  }
 
  //Confirm whether to delete the item
  if (!dontaskquestion) {
    CConfirmDeleteDlg deleteDlg(this, num_children);
    INT_PTR rc = deleteDlg.DoModal();
    if (rc == IDCANCEL) {
      dodelete = false;
    }
  }
 
  if (dodelete) {
    Delete();
    if (m_bFilterActive)
      RefreshViews();
  }
}

Next, for each assignment to the negative variable, we add an assignment to the positive variable with the negative variable’s logical negation:

// Delete key was pressed (in list view or tree view) to delete an entry.
void DboxMain::OnDelete()
{
  if (m_core.GetNumEntries() == 0) // easiest way to avoid asking stupid questions...
    return;
 
  bool dontaskquestion = PWSprefs::GetInstance()->
                         GetPref(PWSprefs::DeleteQuestion);
  bool askQuestion = !dontaskquestion;
 
  bool dodelete = true;
  int num_children = 0;
 
  if (m_ctlItemTree.IsWindowVisible()) {
    HTREEITEM hStartItem = m_ctlItemTree.GetSelectedItem();
    if (hStartItem != NULL) {
      if (m_ctlItemTree.GetItemData(hStartItem) == NULL) {  // group node
        dontaskquestion = false; // ALWAYS ask if deleting a group
        askQuestion = !dontaskquestion;
        // Find number of child items
        num_children = 0;
        if (m_ctlItemTree.ItemHasChildren(hStartItem)) {
          num_children = CountChildren(hStartItem);
        }  // if has children
      }
    }
  }
 
  //Confirm whether to delete the item
  if (!dontaskquestion) {
    CConfirmDeleteDlg deleteDlg(this, num_children);
    INT_PTR rc = deleteDlg.DoModal();
    if (rc == IDCANCEL) {
      dodelete = false;
    }
  }
 
  if (dodelete) {
    Delete();
    if (m_bFilterActive)
      RefreshViews();
  }
}

Now we can replace uses of the negative variable with uses of the logical negation positive variable. This is where we will replace double negative conditions with positive conditions.

// Delete key was pressed (in list view or tree view) to delete an entry.
void DboxMain::OnDelete()
{
  if (m_core.GetNumEntries() == 0) // easiest way to avoid asking stupid questions...
    return;
 
  bool dontaskquestion = PWSprefs::GetInstance()->
                         GetPref(PWSprefs::DeleteQuestion);
  bool askQuestion = !dontaskquestion;
 
  bool dodelete = true;
  int num_children = 0;
 
  if (m_ctlItemTree.IsWindowVisible()) {
    HTREEITEM hStartItem = m_ctlItemTree.GetSelectedItem();
    if (hStartItem != NULL) {
      if (m_ctlItemTree.GetItemData(hStartItem) == NULL) {  // group node
        dontaskquestion = false; // ALWAYS ask if deleting a group
        askQuestion = !dontaskquestion;
        // Find number of child items
        num_children = 0;
        if (m_ctlItemTree.ItemHasChildren(hStartItem)) {
          num_children = CountChildren(hStartItem);
        }  // if has children
      }
    }
  }
 
  //Confirm whether to delete the item
  if (askQuestion) {
    CConfirmDeleteDlg deleteDlg(this, num_children);
    INT_PTR rc = deleteDlg.DoModal();
    if (rc == IDCANCEL) {
      dodelete = false;
    }
  }
 
  if (dodelete) {
    Delete();
    if (m_bFilterActive)
      RefreshViews();
  }
}

Finally, we can inline the references to the negative variable, eliminating it completely. Inlining from the bottom up works best so that when we get to the declaration of the positive variable, it should remove the last reference to the negative variable and inline away its declaration. First, inline the references:

// Delete key was pressed (in list view or tree view) to delete an entry.
void DboxMain::OnDelete()
{
  if (m_core.GetNumEntries() == 0) // easiest way to avoid asking stupid questions...
    return;
 
  bool dontaskquestion = PWSprefs::GetInstance()->
                         GetPref(PWSprefs::DeleteQuestion);
  bool askQuestion = !dontaskquestion;
 
  bool dodelete = true;
  int num_children = 0;
 
  if (m_ctlItemTree.IsWindowVisible()) {
    HTREEITEM hStartItem = m_ctlItemTree.GetSelectedItem();
    if (hStartItem != NULL) {
      if (m_ctlItemTree.GetItemData(hStartItem) == NULL) {  // group node
        askQuestion = true// ALWAYS ask if deleting a group
        // Find number of child items
        num_children = 0;
        if (m_ctlItemTree.ItemHasChildren(hStartItem)) {
          num_children = CountChildren(hStartItem);
        }  // if has children
      }
    }
  }
 
  //Confirm whether to delete the item
  if (askQuestion) {
    CConfirmDeleteDlg deleteDlg(this, num_children);
    INT_PTR rc = deleteDlg.DoModal();
    if (rc == IDCANCEL) {
      dodelete = false;
    }
  }
 
  if (dodelete) {
    Delete();
    if (m_bFilterActive)
      RefreshViews();
  }
}

Finally, inline the declaration:

// Delete key was pressed (in list view or tree view) to delete an entry.
void DboxMain::OnDelete()
{
  if (m_core.GetNumEntries() == 0) // easiest way to avoid asking stupid questions...
    return;
 
  bool askQuestion = !PWSprefs::GetInstance()->
                         GetPref(PWSprefs::DeleteQuestion);
 
  bool dodelete = true;
  int num_children = 0;
 
  if (m_ctlItemTree.IsWindowVisible()) {
    HTREEITEM hStartItem = m_ctlItemTree.GetSelectedItem();
    if (hStartItem != NULL) {
      if (m_ctlItemTree.GetItemData(hStartItem) == NULL) {  // group node
        askQuestion = true// ALWAYS ask if deleting a group
        // Find number of child items
        num_children = 0;
        if (m_ctlItemTree.ItemHasChildren(hStartItem)) {
          num_children = CountChildren(hStartItem);
        }  // if has children
      }
    }
  }
 
  //Confirm whether to delete the item
  if (askQuestion) {
    CConfirmDeleteDlg deleteDlg(this, num_children);
    INT_PTR rc = deleteDlg.DoModal();
    if (rc == IDCANCEL) {
      dodelete = false;
    }
  }
 
  if (dodelete) {
    Delete();
    if (m_bFilterActive)
      RefreshViews();
  }
}

This completes the refactoring.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: