10
\$\begingroup\$

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;
     }
}
\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the answer! 1. I've tried to create a variable but I can not call Variable._SubMenuBtn1Text; Where I use the value of Variable in stead of the name Variable. Is there a way to do so? 2. I preffer _ for constant data, but as I see it is not recommended; I'll change that :). 3. I think a class is nicer because you can place that somewhere else? Placing text in a resource file would be nice, I've thought about that. But as I am still learning I'll pass that to a new project. 4. I'll check that out! Idk whether it works on C#. \$\endgroup\$ – 2pietjuh2 Oct 23 '12 at 7:45
  • 1
    \$\begingroup\$ 4. I've found an artilcle on this topic. I'll rewrite my code using that! \$\endgroup\$ – 2pietjuh2 Oct 23 '12 at 7:53
  • 1
    \$\begingroup\$ I don't see any public methods in the question. And the public constant fields are not so public, because they are in a private struct inside another type. \$\endgroup\$ – svick Oct 23 '12 at 19:16
  • \$\begingroup\$ @svick ah good point. more of a typo although I would still stand by renaming thefields to not having the _ \$\endgroup\$ – dreza Oct 23 '12 at 20:41
  • \$\begingroup\$ @svick Yea, would be better to make them private I guess. no need for public. \$\endgroup\$ – 2pietjuh2 Oct 24 '12 at 9:04
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Why do you have the string constants as const fields? Wouldn't it be better to inline them? (E.g. public string Text { get { return "text submenu 1 button 1"; } }) \$\endgroup\$ – svick Oct 23 '12 at 19:19
  • 1
    \$\begingroup\$ I guess my thinking was to create only one version of the string that will be referenced where ever it is used. I guess it comes from working on an API that get hundreds of hits a second. Also, see stackoverflow.com/questions/1707959/…. Short answer from what I understand: by putting it in a const, you are telling the compiler "This is never going to change" so it can be optimized properly. \$\endgroup\$ – Jeff Vanzella Oct 23 '12 at 20:26
  • \$\begingroup\$ I use the const always for static data I.E: Texts, names of resources (images) or in switch/case structures I always define the cases in const to gain better readability and more flexibility. Especially when you use them at more places \$\endgroup\$ – 2pietjuh2 Oct 24 '12 at 8:41
  • \$\begingroup\$ Thank you for the answer! I've posted what I created, and am wondering why it is better/more efficient/better readable to use an interface for each button, 2 classes for each button, a dictionary, and two functions to update the buttons. Seems quite a lot to me. Could you please explain why you suggest/advise this, or when it usefull to use this? \$\endgroup\$ – 2pietjuh2 Oct 24 '12 at 8:55
  • 1
    \$\begingroup\$ My thought process was to limit the number of parameters in any given function. On average, I try and keep it to two, three in extreme cases. Anything more than that and it gets confusing. I also think doing it this way more portrays the intent/design of the application. I also used interface / class combination so you can do your button assignments in a single method. I used the dictionary to eliminate a possibly huge if/else or switch statement. And if you look at it, it's only one class for each button. There is one class that is shared through all of them. \$\endgroup\$ – Jeff Vanzella Oct 24 '12 at 15:28
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.