.Net - Strategies to avoid magic string

Question

In code at work, we have many uses of magic strings like the following code snippet:

if (user.HasRight("Profile.View")) {...}

So there are many places where we pass a string as a parameter to see if the user has a specific right. I don't like that because that generates a lot of magic strings.

What would be a better way of doing it?

Enum, Constant, class ?

Justin Niessner · Accepted Answer · 2010-07-08T16:41:52.940

19

In that specific case, use an Enum. There will be no magic strings and if the Enum changes (in a way that would break the magic strings solution), the app will no longer compile.

public enum ProfilePermissions
{
    View,
    Create,
    Edit,
    Delete
}

Then you can simply have:

if(user.HasRight(ProfilePermissions.View)) { }

You could also use a class, but then you limit yourself when it comes to more complex scenarios. For instance, a simple change of the Enumeration to something like:

public enum ProfilePermissions
{
    View = 1,
    Create = 2,
    Edit = 4,
    Delete = 8
}

Would allow you to use bitwise operators for more complex permissions (for example, a situation where a user needs either Create or Delete):

if(user.HasRight(ProfilePermissions.Create | ProfilePermissions.Delete));

edited Jul 08 '10 at 16:41

answered Jul 08 '10 at 15:59

Justin Niessner

242,243
40
408
536

1

Depending on the requirements this can also be turned into a flags-enum, i.e. where values can be combined using `|`, for example by using the values 0x1, 0x2, 0x4, etc... – Dirk Vollmar Jul 08 '10 at 16:12
1

Good answer and my answer was identical. I might add that the point of an `enum` is to create an "enumerated list" of constants. Grouping constants where they can be listed like this is a great idea and gives the program a more logical flow as well. – Armstrongest Jul 08 '10 at 16:13
In case if also a string representation is required, e.g. to store the permission set in a human-readable way, you can either simply call `ProfilePermissions.View.ToString()` to get the string "View", or if you need a custom string representation, you could add an adorning *Description* attribute as described here: http://blogs.msdn.com/b/abhinaba/archive/2005/10/20/483000.aspx – Dirk Vollmar Jul 08 '10 at 16:36
What happens when you need to add permissions? Recompile? Constants and enums should be used for static data. View, create, edit and delete are fine, but I would not call the enum ProfilePermissions, just Permissions. In the profile class add a property for permissions and you are done. The entire thing can be managed in an outer data source and your code keeps it's readability and security. – Zohar Peled Sep 29 '15 at 20:34
I'm a big fan of enums replacing magic strings. One other nice feature is where you absolutely have to pass in a string (say because it's a third party library that only handles strings) you can use the ToString() method to keep the strength of using enums, but still have a string. It only works for alphabetical characters but is handy. e.g. ThirdPartyLibrary.GetResponse(string role) could take enum Roles { Admin, Editor, Guest } as ThirdPartyLibrary.GetResponse(Roles.Guest.ToString()) – csharpforevermore Mar 21 '17 at 11:57

score 8 · Answer 2 · answered Jul 08 '10 at 16:21

8

This is common enough in the .NET framework as well. Examples are System.Windows.DataFormats and System.Net.WebRequestMethods.Http. You'd want the readonly variety:

public static class MumbleRights {
  public static readonly string ProfileView = "Profile.View";
  // etc..
}

answered Jul 08 '10 at 16:21

Hans Passant

922,412
146
1,693
2,536

7

@Markus, if public constants are used from an external assembly, they are baked in; if you later change your public constant, the external assembly will still use the constant it saw when it was compiled. – Dan Bryant Jul 08 '10 at 16:41
1

http://stackoverflow.com/questions/277010/what-are-the-benefits-to-marking-a-field-as-readonly-in-c/312840#312840 – Daniel Auger Jul 08 '10 at 16:41
Right, there are few constants in the world that are invariant enough to allow them to be public constants. Math.Pi is okay. – Hans Passant Jul 08 '10 at 16:51

score 3 · Answer 3 · answered Jul 08 '10 at 16:39

Extension methods! Keep them in the same place to keep track of all magic strings.

public static class UserRightsExtensions {
  public static bool CanReadProfile(this User user)
  {
    return user.HasRight("Profile.View");
  }

  // etc..
}

Then you can:

if (user.CanReadProfile()) .....

score 1 · Answer 4 · edited Jul 23 '15 at 21:12

1

Create a class which strongly-types those properties, like

public static class UserInfo
{
  public static bool CanViewProfile { get { return User.HasRight("Profile.View"); } }
}

This will keep your "magic strings" in one place within your code. An enum will also work, but isn't as readable in my opinion.

Note: my example is intended to act as a property proxy for the logged in user, thus the static class. If you wanted something that would work on more immediate data (say, a list of users), this type of class would need to be non-static and instantiated on per-user-account basis.

edited Jul 23 '15 at 21:12

Abdul Ahmad

9,673
16
64
127

answered Jul 08 '10 at 16:25

3Dave

28,657
18
88
151

`CanViewProfile` makes no sense as a static property in my opinion and should rather be an instance method on a user object. The snippet suggests that there would be a static class for each user? – Dirk Vollmar Jul 08 '10 at 16:33
No, I'm not suggesting a static class for each user. The class above just acts as a proxy to properties of the logged-in user. (I used "Alice" because I'm used to using alice, bob, charlie, david, etc instead of foo/bar. – 3Dave Jul 08 '10 at 16:35

score 0 · Answer 5 · answered Jul 08 '10 at 16:02

0

You can do constant strings in C#.

You could define all of the strings in a header like this:

const string PROFILE_VIEW "Profile.View";

Not sure if this is the "best" way, but its certainly better than having magic values in the code.

answered Jul 08 '10 at 16:02

Avalanchis

4,500
3
39
48

score 0 · Answer 6 · answered Jul 08 '10 at 16:34

I second the way shown by "Justin Niessner". But in some cases I would rather prefer writing following construct of code.

public  class User
    {
        public Permission Permission { get; set; }

    }
    public abstract class Permission
    {

    }
    public class ViewPermission:Permission
    {

    }

and you can consume it as

User user=new User();
            if(user.Permission is ViewPermission)
            {

            }

.Net - Strategies to avoid magic string

6 Answers6

Linked