3

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.

marked as duplicate by gnat, Doc Brown, RubberDuck, user22815, Robert Harvey May 16 '17 at 5:29

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 2
    Please don't down vote. allow me to rectify issues. – Tez Wingfield May 15 '17 at 9:26
  • 1
    it better not be called db.field unless it's a farming application – Weyland Yutani May 15 '17 at 10:09
  • 2
    I would use an int mapped to an enum, it's more readable, less prone to error. not by much though. analyse if it's a real problem. You might be spending more time thinking about it than it's going to cost. – Weyland Yutani May 15 '17 at 10:11
  • 1
    Using else for ignored without any sanity checks is bad. I'd add another else if(ignored) and throw an exception is the else to indicate that it should be unreachable. And why are you using an if/else if chain instead of a switch? – CodesInChaos May 15 '17 at 10:13
  • 1
    obvs im only going off the snippet youve given. but if the db row is effectively a Task and you are processing it. sure. – Ewan May 15 '17 at 10:27
9

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));
}
  • 3
    To actually answer the question of how to win over your coworkers: write this code, test this code, show them this code, ask them what they think, ask them what could be improved. Don't argue. Collaborate. If I was at the design table this code would get my vote. If vote deadlocks pull out a quarter. Obey the quarter. – candied_orange May 15 '17 at 14:04
  • 1
    Regarding the switch-case, I prefer to use a table-driven-design approach. I'm nitpicking, but what's op question besides nitpicking ? :) – Machado May 15 '17 at 16:15
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.

0

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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.