HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

User can choose to add one of two node types to a treeView, both of which have almost identical settings

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
cansettingsalmostuserchoosenodeidenticalbothonetwo

Problem

There is a context menu that comes up when a user right-clicks on a node in a TreeView. They are then presented with the choice to either add what is called a "RWB" or an "AQ" node to the tree. Both of these types of nodes have several settings in them, all of which are identical, save for one: AQ nodes have their 'Analog' field set to true, while RWBs have it set to false. Because of this, my event handling methods as well as my methods to add the actual node to the tree are essentially identical with the exception of changing the analog value to true within the AddAQ() method.

The problem is I can't figure out a smart way to accomplish what I need to without duplicating both the event and adding methods twice. Does anyone have any tips on how I can make this cleaner?

```
///
/// Updates the rcontext menu based on the type of item
/// selected in the relay word view.
///
private void UpdateRelayWordPagesAndContextMenu()
{
if (NavigationControlRelayWord.SelectedNode != null)
{
NavigationControlRelayWord.ContextMenu = new ContextMenu();

if (NavigationControlRelayWord.SelectedNode.Parent == null)
{
// Root node
NavigationControlRelayWord.ContextMenu.MenuItems.Add(Resources.AddCategory, AddRWBAQCategoryEvent);
NavigationControlRelayWord.ContextMenu.MenuItems.Add("-");
NavigationControlRelayWord.ContextMenu.MenuItems.Add(Resources.PasteRWBAQNode, PasteRWBAQEvent);

RelayWordNodeValue clipboardNodeValue = ClipboardUtilities.GetRWBAQClipboardNode();
if (string.IsNullOrWhiteSpace(clipboardNodeValue.Name) ||
!RelayWordNodeValue.CategoryType.Equals(clipboardNodeValue.NodeType))
{
NavigationControlRelayWord.ContextMenu.MenuItems[NavigationControlRelayWord.ContextMenu.MenuItems.Count - 1].Enabled = false;
}
}
else if (NavigationControlRelayWord.SelectedNode.Tag is IDeviceLogicCategory)
{
// Category node
NavigationControlRelayWord.ContextMenu.MenuItems.Add(Resource

Solution

Early returns

Early returns are your friend:

private void UpdateRelayWordPagesAndContextMenu()
{
  if (NavigationControlRelayWord.SelectedNode != null)
  {
    // Really long block here....
  }
}


VS:

private void UpdateRelayWordPagesAndContextMenu()
{
  if (NavigationControlRelayWord.SelectedNode == null)
  {
    return;
  }

  // Really long block here
}


Now you have one less level of indentation to keep straight.

You can also user early returns here (AddAQEvent(object sender, EventArgs e)):

if (inputForm.ShowDialog() == DialogResult.OK)
{
  var name = inputForm.ShortName;
  var description = inputForm.DescriptiveName;
  if (name != string.Empty)
  {
    if (Device.DeviceWord.FindItemByName(name) == null)
    {
      AddAnalogQuantityToTreeView(name, description);
    }
    else
    {
      MessageBox.Show(Resources.ElementExists, Resources.DeviceBuilder);
    }
  }
  else
  {
    MessageBox.Show(Resources.BlankAQError, Resources.DeviceBuilder);
  }
}


I would write this as:

if (inputForm.ShowDialog() == DialogResult.OK)
{
  var name = inputForm.ShortName;
  var description = inputForm.DescriptiveName;

  if (name == string.Empty)
  {
    MessageBox.Show(Resources.BlankAQError, Resources.DeviceBuilder);
    return;
  }

  if (Device.DeviceWord.FindItemByName(name) == null)
  {
    AddAnalogQuantityToTreeView(name, description);
  }
  else
  {
    MessageBox.Show(Resources.ElementExists, Resources.DeviceBuilder);
  }
}


Methods

Use more methods! For example, UpdateRelayWordPagesAndContextMenu():

if (NavigationControlRelayWord.SelectedNode.Parent == null)
{
  // block here
}
else if (NavigationControlRelayWord.SelectedNode.Tag is IDeviceLogicCategory)
{
  // block here
}
else if (NavigationControlRelayWord.SelectedNode.Tag is IDeviceLogicElement)
{
  // block here
}


These can be extracted into their own methods, along with a lot of other code.

Code Snippets

private void UpdateRelayWordPagesAndContextMenu()
{
  if (NavigationControlRelayWord.SelectedNode != null)
  {
    // Really long block here....
  }
}
private void UpdateRelayWordPagesAndContextMenu()
{
  if (NavigationControlRelayWord.SelectedNode == null)
  {
    return;
  }

  // Really long block here
}
if (inputForm.ShowDialog() == DialogResult.OK)
{
  var name = inputForm.ShortName;
  var description = inputForm.DescriptiveName;
  if (name != string.Empty)
  {
    if (Device.DeviceWord.FindItemByName(name) == null)
    {
      AddAnalogQuantityToTreeView(name, description);
    }
    else
    {
      MessageBox.Show(Resources.ElementExists, Resources.DeviceBuilder);
    }
  }
  else
  {
    MessageBox.Show(Resources.BlankAQError, Resources.DeviceBuilder);
  }
}
if (inputForm.ShowDialog() == DialogResult.OK)
{
  var name = inputForm.ShortName;
  var description = inputForm.DescriptiveName;

  if (name == string.Empty)
  {
    MessageBox.Show(Resources.BlankAQError, Resources.DeviceBuilder);
    return;
  }

  if (Device.DeviceWord.FindItemByName(name) == null)
  {
    AddAnalogQuantityToTreeView(name, description);
  }
  else
  {
    MessageBox.Show(Resources.ElementExists, Resources.DeviceBuilder);
  }
}
if (NavigationControlRelayWord.SelectedNode.Parent == null)
{
  // block here
}
else if (NavigationControlRelayWord.SelectedNode.Tag is IDeviceLogicCategory)
{
  // block here
}
else if (NavigationControlRelayWord.SelectedNode.Tag is IDeviceLogicElement)
{
  // block here
}

Context

StackExchange Code Review Q#94707, answer score: 4

Revisions (0)

No revisions yet.