Representing a status as single letter strings [duplicate]

Question

I worry that I'm too concerned with code smells. I've spent the last two days procrastinating over implementation details and how I would actively refuse using the approach suggested.

We have a scenario where we "pull" orders and each order can be "deleted", "re-created" or "ignored"

Tech lead suggested having one database field with either values of "A", "D" or "I" - literally.

and the code base we would do something similar too:

if(db.field == "A")
{
  //Add Duplicate
}
else if (db.field == "D")
{
  //Delete and Re-create
}
else
{
  //Must be ignore, so don't insert
}

Now I know this would work but in the grand scheme of things this is in a method that does a lot of model mapping and inserting.

I tried to stress the importance of NOT doing comparisons on hardcoded string values (and also the idea of enums). In my eyes it is against SQL best practices, and is "smelly" code.

This is very embarrassing that we are arguing over such a small feature when our actual code base and systems are very impressive - it would be better to argue over larger issues, rather than nothing.

So If I may ask, how you would you handle this situation? Is this really "smelly" code or am I being too argumentative?

EDIT//

A little clarification, I've not spent the last two days not working, this was in-conjunction with other work.

Answer 1

Use an enum

On the database side this pairs efficient storage with meaningful values in SQL queries and database viewers.

On the C# side this finds typos at compile-time, improves type safety and gives access to IDE features like "Find all references".

Hardcoding strings can sometimes be appropriate, but I don't think this is one of those cases. For example when writing a parser, I might hardcode strings which have specific meaning in the grammar. The important thing is to only specify such a string once, when it's used more often, extract it into a constant.

Use switch instead of an if/else chain

This is exactly the use-case switch has been designed for. It's idiomatic code and possibly faster for longer chains.

Don't implicitly assume that the default case is the last possible value

This hides errors. Instead define a case for all your values and throw an exception in the default case to detect unexpected data.

Keep the single responsibility principle in mind

Make sure not to mix separate concerns in a single field. I don't fully understand you use case, but it might be appropriate to split your field into separate fields.

---

I'd write something like this:

switch(db.field)
{
    case OrderStatus.Deleted:
        ...
        break;
    case OrderStatus.Ignored:
        ...
        break;
    ...
    default:
        throw new NotSupportedException(string.Format("Unexpected OrderStatus '{0}'", db.field));
}

Answer 2

On the hardcoding of string constants

I tried to stress the importance of NOT doing comparisons on hardcoded string values (and also enums). In my eyes against SQL best practises, is "smelly" code and so on.

You know what is worse than hardcoding something? Making something "soft-coded" / "configurable" / "flexible" when it is already hardcoded somewhere else. When you do that, you have all the disadvantages of flexibility (flexibility = more opportunity for error) and none of the advantages (because you can't actually change it anyway). You end up with a value that appears to be something you can change when in fact it has to be set to one and only one value in order to work. It serves no purpose, and in fact it gets in the way.

So I'd ask you to think about two things:

(1) The software that inserts these records. Is it hardcoded? Then hardcode the software that reads it.

(2) The database. When you deploy your software, will the database already exist and have data in it? If yes, then any symbolic data (such as A/D/I) in those tables is as good as hardcoded-- unless you plan to run migration scripts with your deployments.

In my opinion, comparing to a hardcoded string constant is not only perfectly fine in this situation, it is the go-to method for handling it.

Getting rid of that case statement

Now, maybe you have a problem with the case statement. That's fine. You can get around it with some delegates, like this:

//Declare our hardcoded values
const string OrderAdd = "A";
const string OrderDelete = "D";
const string OrderIgnore = "I";

//Initialize.  Add to static constructor somewhere.
var lookup = new Dictionary<string, Action<DataRow>);
lookup.Add(OrderAdd, ExecuteAddOrder);
lookup.Add(OrderDelete, ExecuteDeleteOrder);
lookup.Add(OrderIgnore, (DataRow) => {} );

//Handle a record
var code = dataRow["OrderAction"] as string;
if (!lookup.Exists(code)) throw new InvalidOperationException();
var action = lookup[code];
action(dataRow);

//Define handlers
void ExecuteAddOrder(DataRow sourceRecord)
{
    //Add order here
}

void ExecuteDeleteOrder(DataRow sourceRecord)
{
    //Delete order here
}

While the above seems cool it actually doesn't add much value. It adds a little. Depending on your team, the decrease in readability may not be worth it.

On procrastinating

Unless you are very lucky, you are not working on a piece of artwork. You are working a practical solution to a business problem. In many cases, timeliness has much more business value than elegance. Your success as an engineer will be highly dependent on your ability to solve technical problems quickly and practically. Coming up with the perfect solution a month too late will not get you any extra credit whatsoever.

Remember this, and remember that a year from now, no matter how lovely your code, you will probably look back at it and laugh.

Answer 3

My view on these 'coding style' issues is that they are just that 'coding style'

Whom ever actually writes the particular bit of code in question should be given substantial lee way to use thier own style.

These arguments tend to be won by the person who is most belligerent and regardless of how right you are it just leads to a bad working environment where experimentation and flare are repressed.

If you have a real problem with bad code then you are probably missing requirements on your tasks. Push to get things like scalability and performance tests added, so that you have a legitimate argument if something fails them.