CODEWITHSUNDEEP
c#
Nick LaMarca
Nick LaMarca

Reputation: 8188

Using a lot of hardcoded strings in code

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?

Upvotes: 15

Views: 23368

Answers (8)

Reap
Reap

Reputation: 1235

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();

Upvotes: 2

pfries
pfries

Reputation: 1758

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.

Upvotes: 0

Jeb
Jeb

Reputation: 3799

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; 
}

Upvotes: 2

Michal B.
Michal B.

Reputation: 5719

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!)

Upvotes: 0

Joe
Joe

Reputation: 82614

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.

Upvotes: 2

jrummell
jrummell

Reputation: 43087

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.

Upvotes: 36

Haris Hasan
Haris Hasan

Reputation: 30097

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.

Upvotes: 3

Andrew
Andrew

Reputation: 7778

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

Upvotes: -1

Related Questions