Creating submenus with buttons

Question

I have a menu, where a user selects one out of 4, and another 2 or 4 options will appear. I used big buttons; a picturebox + label to create the buttons.

The 4 main buttons are fixed, but the 4 other buttons depend on the first choice. Text and image will change. I programmed this, but I kinda feel this must be possible more easily.

I have 4 structs which contain the image path and label text. like this: (I'll only show with 2, because the rest won't add anything, it is just the same.)

private const string _mainButton1 = "ConfigDP";
private const string _mainButton2 = "ConfigADP";

struct ConfigDPSubmenu
{
    public const string _SubMenuBtn1Text = "text submenu 1 button 1";
    public const string _SubMenuBtn2Text = "text submenu 1 button 2";
    public const string _SubMenuBtn1Img = "DevPortal.png";
    public const string _SubMenuBtn2Img = "CustomDevPortal.png";
}
struct ConfigADPSubmenu
{
    public const string _SubMenuBtn1Text = "text submenu 2 button 1";
    public const string _SubMenuBtn2Text = "text submenu 2 button 2";
    public const string _SubMenuBtn1Img = "ADP.png";
    public const string _SubMenuBtn2Img = "CustomADP.png";
}

Then I have the function where I make the buttons: (it is called when press on the main buttons)

private void CreateSubButtons(string sender)
{
     pictureBoxSubBtn3.Image = null;
     pictureBoxSubBtn4.Image = null;
     SubBtn3Text.Text = "";
     SubBtn4Text.Text = "";
     panelSubBtn3.BorderStyle = BorderStyle.None;
     panelSubBtn4.BorderStyle = BorderStyle.None;
     switch (sender)
     {
          case _mainButton1:
              SubBtn1Text.Text = ConfigDPSubmenu._SubMenuBtn1Text;
              SubBtn2Text.Text = ConfigDPSubmenu._SubMenuBtn2Text;
              pictureBoxSubBtn1.Image = Image.FromFile(ConfigDPSubmenu._SubMenuBtn1Img);
              pictureBoxSubBtn2.Image = Image.FromFile(ConfigDPSubmenu._SubMenuBtn2Img);
              panelSubBtn1.BorderStyle = BorderStyle.FixedSingle;
              panelSubBtn2.BorderStyle = BorderStyle.FixedSingle;
              break;
          case _mainButton2:
              SubBtn1Text.Text = ConfigADPSubmenu._SubMenuBtn1Text;
              SubBtn2Text.Text = ConfigADPSubmenu._SubMenuBtn2Text;
              pictureBoxSubBtn1.Image = Image.FromFile(ConfigADPSubmenu._SubMenuBtn1Img);
              pictureBoxSubBtn2.Image = Image.FromFile(ConfigADPSubmenu._SubMenuBtn2Img);
              panelSubBtn1.BorderStyle = BorderStyle.FixedSingle;
              panelSubBtn2.BorderStyle = BorderStyle.FixedSingle;
              break;
     }
}

Answer 1

A couple of points that may help you.

1. When you see yourself copy and pasting or writing duplicated code, consider refactoring. I could only see one difference in your code in the case statements. You could move all that to a method and pass in the necessary struct. Even further, once you have done this you will see further code duplication you could probably further refactor into seperate methods.

2. I'm not sure about having exposed fields on the struct start with _. You might want to read Jon Skeets Answer on stack overflow.

3. I would probably consider removing your struct and using a single class. Then I would create 4 instances of that class with the values required. I might even consider populating the text for those from a resource file.

4. I'm not really sure on this, but in old versions (Vb 6....) you could create controls which were arrays of each other. So instead of panelSubBtn1, panelSubBtn2 etc you would just have panelSubBtn and reference it by panelSubBtn[0] etc If this was possible you could seriously simplify your code. I'll let you figure that out though...

These are a few quick things which may or may not help.

Answer 2

dreza has some great comments, but I would go further and change the structs to classes that inherit from interfaces.

I'll start with the design.

First you could create an interface that represents button information.

public interface IButtonInformation
{
      string Text{ get; }
      string ImageFileName
}

Then an interface that represents the possible statuses.

public interface IFormStatus
{
     IButtonInformation Button1 { get; }
     IButtonInformation Button2 { get; }
}

You can now create concrete classes:

public class DPButton1 : IButtonInformation
{
     private const string TextString = "text submenu 1 button 1";
     private const string ImageFileNameString = "StreamitDevPortal.png";

     public string Text { get { return TextString ; } }
     public string ImageFileName{ get { return ImageFileNameString; } }
}


// Do the same for all 4 possible states.

And create the statuses

public class ConfigDPStatus : IFormStatus
{
     public IButtonInformation Button1 { get; private set;
     public IButtonInformation Button2 { get; private set;

     public ConfigADPStatus()
     {
         Button1 = new DPButton1();
         Button2 = new DPButton2();
     }
}

// Rinse and repeat for ADP Status

Now your CreateSubButtons looks like this:

private const IDictionary<string, IFormStatus> ButtonStatuses = new Dictionary<string, IFormStatus>
    {
         {MainButton1, new ConfigDPStatus()},
         {MainButton2, new ConfigADPStatus()},
    }

private void CreateSubButtons(string sender)
{
     if (!ButtonStatuses.ContainsKey(sender)
     {
          throw new ApplicationException("Unknown sender");
     }

     FormatButtons(ButtonStatuses[sender]);
}

private void FormatButtons(IFormStatus status)
{
     SubBtn1Text.Text = status.Button1.Text;
     SubBtn2Text.Text = status.Button2.Text;
     pictureBoxSubBtn1.Image = Image.FromFile(status.Button1.ImageFileName);
     pictureBoxSubBtn2.Image = Image.FromFile(status.Button1.ImageFileName);
}

You'll notice I didn't clear the controls as you did, I don't think that is necessary as you are just re-assigning them right away anyway.

Answer 3

I didn't see this covered in other peoples' answers, so I thought I'd mention one thing. Your structs with consts are all right by me:

struct ConfigDPSubmenu
{
    public const string _SubMenuBtn1Text = "text submenu 1 button 1";
    public const string _SubMenuBtn2Text = "text submenu 1 button 2";
    public const string _SubMenuBtn1Img = "StreamitDevPortal.png";
    public const string _SubMenuBtn2Img = "CustomDevPortal.png";
}
struct ConfigADPSubmenu
{
    public const string _SubMenuBtn1Text = "text submenu 2 button 1";
    public const string _SubMenuBtn2Text = "text submenu 2 button 2";
    public const string _SubMenuBtn1Img = "StreamitADP.png";
    public const string _SubMenuBtn2Img = "CustomADP.png";
}

However, they are instantiatable as such:

var c1 = new ConfigDPSubmenu();
var c2 = new ConfigADPSubmenu();

which semantically makes no sense. In order to take care of that, convert them to static classes:

static class ConfigDPSubmenu
{
    public const string _SubMenuBtn1Text = "text submenu 1 button 1";
    public const string _SubMenuBtn2Text = "text submenu 1 button 2";
    public const string _SubMenuBtn1Img = "StreamitDevPortal.png";
    public const string _SubMenuBtn2Img = "CustomDevPortal.png";
}
static class ConfigADPSubmenu
{
    public const string _SubMenuBtn1Text = "text submenu 2 button 1";
    public const string _SubMenuBtn2Text = "text submenu 2 button 2";
    public const string _SubMenuBtn1Img = "StreamitADP.png";
    public const string _SubMenuBtn2Img = "CustomADP.png";
}

You shouldn't lose any sort of performance or functionality by that conversion.