Avoiding magic strings and numbers

Question

I am working on an application that has been edited by various programmers over the past few years and I have stumbled across a problem with using String Literals to access MenuItems.

For Example: in many places there is code like

mainMenu.MenuItems[1].MenuItems[0].Visible=true;

or

mainMenu.MenuItems["View"].MenuItems["FullScreen"].Visible=true;

how do I change the Strings used to identify the MenuItem and catch all of the places that it is being used for access? The menus and menuitems are declared as public and are used throughout this large application
What is the right way prevent the use of these magic indexes from being used. I forsee things being broken everytime a new item is added or the name is changed.

P.S. I have started using an enumerated dictionary approach in which every menuItem is paired with a key. but this still does not force other developers to use my implementation nor is it the most elegant solution to question 2

You can't force magic strings from not being used by other developers, but you could highlight the negatives of this behaviour at a developer meeting and make it stand out in developer documentation that it should be avoided. There are a number of effective ways to push the point through to the entire team. You could even try and get it put in the company developer guidelines! — Lloyd Powell, Jun 19 '12 at 11:29

Roman Starkov · Accepted Answer · 2012-06-19T11:48:19.267

9

Give each menu item a name in the WinForms designer (I assume), and then refer to it by that name.

enter image description here

Then just use this in your code:

menuExit.Visible = false;

If the menu items are added programmatically, do this:

class MyForm : Form
{
    private MenuItem menuExit;

    ...

        myMenu.Items.Add(menuExit = new MenuItem(...));

    ...
}

and then still access it by the menuExit name. The key to avoiding magic numbers and strings is to just keep a direct reference to whatever it is you want to refer to. As a bonus, you can now rename this vairable safely using F2.

edited Jun 19 '12 at 11:48

answered Jun 19 '12 at 11:25

Roman Starkov

59,298
38
251
324

Sorry but they are all added programmatically using new MenuItem("string"); – jth41 Jun 19 '12 at 11:42
Thanks, now how about catching all of the places where the string literals were used? – jth41 Jun 19 '12 at 22:27
@John Hmm... try "Find Symbol" on `MenuItems`? (position your cursor on MenuItems and press Shift+F12). – Roman Starkov Jun 20 '12 at 11:22

score 0 · Answer 2 · answered Jun 19 '12 at 12:59

Romkyns answer is the correct one for this scenarion however if you do need to use string literals in your code I would alwasy keep them in public static classes such as:

public static class Constants
{
    public static class Menu 
    {
        public static readonly string FirstMenuName = "Menu 1";
        ...
    }

    public static class OtherCateogry
    {
        ...
    }
}

You can then access them by Constants.Menu.FirstMenuName.

As for definitively preventing other devs from using literals throughout code - you might have to make recourse to the Rod of Correction (sturdy metal ruler) ;).

Avoiding magic strings and numbers

2 Answers2

Linked