Using a lot of hardcoded strings in code

Question

When I am looking at my code and I am writing things like..

if (role == "Customer")
{
    bCustomer = true;
}
else if (role == "Branch")
{
    bIsBranch = true;
}

Or

foreach(DataRow as row in myDataSet.Tables[0].Rows)
{
    row["someField"]=somefield.Tostring()
}

Are you guys doing this? When is this ok to do and when shouldn't you be doing this? What if any would be a better approach to write this if any?

Thanks For the Comments: I guess I should add what if (for this example purposes) I am only using this role comparison once? Is it still a better idea to make a whole new class? Also should I have 1 class called "constants" are multiple classes that that hold specific constants, like "roles" class for example?

It's hard to give a definite answer, but in the first example, I would try to refactor it to use polymorphism if applicable. In the second case I would probably create a wrapper class around the DataTable which has the column names as string constants so there is one central place for them. All consumers are only using the wrapper class and not the DataTable. — Daniel Hilgarth, Feb 29 '12 at 15:32
using `nameof` these days to refer to the names code elements like `types`, `classes`, `properties`, etc. See my answer below for a more detailed understanding of the `nameof` expression. — Reap, Aug 10 '21 at 04:06

score 36 · Accepted Answer · answered Feb 29 '12 at 15:33

36

No. Don't use "magic strings". Instead create a static class with constants, or an enum if you can.

For example:

public static class Roles
{
  public const string Customer = "Customer";
  public const string Branch = "Branch";
}

Usage:

if (role == Roles.Customer)
{

}
else if (role == Roles.Branch)
{

}

Here's a good discussion on various solutions.

answered Feb 29 '12 at 15:33

jrummell

42,637
17
112
171

1

I don't see why this is an instance of 'magic strings'? From the wiki: "A magic string is an input that a programmer believes will never come externally and which activates otherwise hidden functionality. A user of this program would likely provide input that gives an expected response in most situations. However, if the user does in fact innocently provide the pre-defined input, invoking the internal functionality, the program response is often quite unexpected to the user (thus appearing 'magical')". – pfries Feb 29 '12 at 16:07
1

I'm not a fan of the wording on the wikipedia article, but it was the first definition I found. In software, magic strings typically refer to string values that could be easily mistyped by the developer. – jrummell Feb 29 '12 at 16:09
1

I usually think of 'magic strings' as flags whose meaning or use is arcane or otherwise not self-evident. If not for the compiler, the entire C# language would qualify as magic strings by your definition, not to mention all interpreted dynamic languages. I mistype that stuff all the time...actually it does sorta feel like magic when I remember the api without reference. – pfries Feb 29 '12 at 16:14
2

This has nothing to do with the language, simply hard coded string values in source code. As a statically typed, compiled language, any misspellings of keywords or types would be caught by the compiler. – jrummell Feb 29 '12 at 16:20
"Please use Enums to solve problems like this!" How, why, and what if a space is needed? – shiggity Jan 17 '17 at 22:30
2

In C# (or .NET and many other languages b.t.w.) it is considered a code smell to use `plublic const` variables. When you are using `const` variables you need to ensure they cannot leave the scope of the assembly (see [here](https://exceptionnotfound.net/const-vs-static-vs-readonly-in-c-sharp-applications/) and the note in the [official documentation from Microsoft](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/constants)). Instead make the `const` internal or use `public static readonly` variables instead. – Maurits van Beusekom Sep 26 '19 at 09:35

score 3 · Answer 2 · answered Feb 29 '12 at 15:33

It is always better to declare the hard coded strings separately as constants rather then declaring a new string every time. It keeps code clean and reduce errors which are caused by typing mistakes.

Regarding should or shouldn't be done totally depends on scenario.

Reap · Answer 3 · 2022-08-12T21:57:45.797

Eliminate the use of magic strings in your C# code by using the `nameof` expression

Using the nameof expression, you can retrieve the literal casing of types, classes, structs, properties, methods, functions, fields, arguments, parameters, locals, and more to the casing they appear in the code at compile time. This will not eliminate or solve all your "magic string" problems, but its a good start and worth discussing.

For example, getting the literal casing of an enum value.

public enum ExportType
{
   CSV,
   Excel
}

`nameof` Usage

nameof(ExportType.CSV); // "CSV"
nameof(ExportType.Excel); // "Excel"
nameof(ExportType); // "ExportType"

Returns the literal casing of the expression in the argument.

No more "magic strings"

If you are referring the code names of specific type names, classes, etc., strongly consider replacing those fragile magic strings with nameof. You will not fear changing the name of a internal type, or property without fear of breaking the code. Using renaming features IDEs like Visual Studio has will rename all references anywhere in your code base referring to that expression.

Type Safety

This operation is done at compile time. Ultimately, if you are relying on the names of types, classes, etc. in your code, you can introduce compile time type safety via nameof expression into your code when referring to them.

Performance

You can eliminate a lot of reflection in your code as well that gets the names of these objects or types.

Caveats

Getting the name of generic types

nameof(T); // "T"
nameof(TEntity); // "TEntity"

In these cases, you must continue to use reflection to get the name of the type at runtime. typeof(T).Name.

For example:

var enumValuesNames = typeof(ExportType).GetProperties().Select(p => p.Name).ToArray();

score 2 · Answer 4 · answered Feb 29 '12 at 15:33

I would make a Roles static class:

public sealed class Roles
{
    public const string BRANCH = "Branch";
    public const string CUSTOMER = "Customer";

    public static bool IsCustomer(string role)
    {
        return role == CUSTOMER;
    }
}

Then in your code:

bCustomer = Roles.IsCustomer(role);

Alternatively, this requires a little more setup but the RoleProvder (depending on Web or Not) provides a lot of good methods.

score 2 · Answer 5 · edited May 23 '17 at 10:31

2

I believe a better approach is to use application settings which means you won't ever need to recompile your code if "Customer" or "Branch" values change. Magic values are obviously bad, and this would be a good first step/option getting away from them. Additionally it keeps your values in one place, and I also believe you can reload the settings at runtime without restarting the application (although I haven't tried this myself).

E.g.:

if (role == Properties.Settings.Default.CustomerRole) 
{     
    bCustomer = true; 
} 
else if (role == Properties.Settings.Default.BranchRole) 
{    
    bIsBranch = true; 
}

edited May 23 '17 at 10:31

Community

1
1

answered Feb 29 '12 at 15:36

Jeb

3,689
5
28
45

Can you show me some code in the application settings for this? – Nick LaMarca Feb 29 '12 at 15:57
In Visual Studio, right click on your project, select Properties->Settings tab. Then click to create a default settings file (if one does not exist). If it does, then add your entries here for CustomerRole being a string with value "Customer" etc. Then you can reference them through the static default instance as I've shown in my example. Check the first part of this video: http://www.youtube.com/watch?v=t9WIvYQ1dNU and also 3:50 mins in. – Jeb Feb 29 '12 at 16:04

score 0 · Answer 6 · answered Feb 29 '12 at 15:34

Polimorphism is one thing, but using hardcoded strings along your code is not good at all. It's way better to define a variable holding the string and use this variable along the code. This case if you need to change something (believe me you will), you can just change the value of this variable and it's done (less errors too!)

score 0 · Answer 7 · answered Feb 29 '12 at 16:05

For the sake of maintainability, you should formalize string comparators when possible, either as named constants or as an enumeration. The benefit for the programmer is that you can localize changes. Even when using a refactoring tool, finding all the places a string is used can be tedious and error prone. You may only have one place where you're doing this comparison today, but you or a future maintainer may extend this to other parts of the code. Also, the class itself may grow and need to be broken apart. These things tend to creep up on a program over time.

I would simply declare these strings as constants close to where they're used, but together. Don't bother with a new abstraction like Roles until you know you need it. If the number of roles you need to compare grows, or is needed outside of this class, then you can create a Roles enum or Roles class, depending on the complexity of the comparison.

Also, by using constants, you signal the intended use to the compiler, so you get some minor memory management benefits, which, given your comparison is in a loop, is generally a good practice.

score -1 · Answer 8 · answered Feb 29 '12 at 15:32

-1

Well, in my opinion it is up to you and it depends on your application design. I uaully look at it from the positive side- if application works the way it supposed to work, it is all good. IMHO

answered Feb 29 '12 at 15:32

Andrew

7,619
13
63
117

Working is one thing, but magic strings bring some other problems with it like: hard to maintain: 1) if your DB or data layer changes, then you need to change the magic strings in many potential locations; 2) you can easily make typos. etc. – Victor Lee May 27 '21 at 14:42