6

I have one one software system which allows developers to specify an ID or name to create NodeReferences. Both work fine, but ID's are not guaranteed to be the same across different environments. I've tried in documentation and in conversations to stress that ID's should not be hard coded (since they will break when deployed to a different environments), but some developers are still using them.

This works only in dev environment:

var level = LevelReference.ById(20);
var node = NodeReference.ByName(level, _patientGuid.ToString());

This works in all environments:

var app = ApplicationReference.ByName("Reporting");
var area = AreaReference.ByName(app, "Default");
var level = LevelReference.ByName(area, "Patient");
var node = NodeReference.ByName(level, _patientGuid.ToString());

I think the appeal is just that it produces fewer lines of code. The use of ID's is not by itself bad (since there are valid use cases like caching the ID returned from the server and using it later for faster look-up), but hard-coded ID's are bad. Most of the time the first code will throw an exception, but it's possible that it could be a valid ID for a different object than the developer intended, and this could result in very bad problems.

What's the best way to discourage the use of such constants in code? Ideally I'd like throw some kind of compiler error when I see code like the first example, or at least throw an exception before the call gets down to the database.

11
  • 3
    Some people hold the opinion that database row id's should never leave the database since they are essentially an implementation detail. In that school of thought, every [aggregate root] object would be referenced by some unique natural or surrogate key instead. I can see a lot of logic in this mentality although I've never had the scope to try it myself.
    – MattDavey
    Mar 6, 2013 at 17:11
  • 21
    Two words: "code reviews."
    – Blrfl
    Mar 6, 2013 at 17:34
  • 1
    The kind of expressions you'd be trying to hunt down look like they'd be easy to find with grep, so you might be able to get a leg up on your code base that way.
    – Blrfl
    Mar 6, 2013 at 18:41
  • 3
    Any constant passed into ById(int) would be a magic number. Not getting rid of magic numbers is a bigger issue than just passing them into this method.
    – user40980
    Mar 6, 2013 at 23:22
  • 2
    It seems you are asking about hardcoded literals rather than constants. They are related concepts but not the same.
    – Flater
    Nov 28, 2019 at 11:28

8 Answers 8

21

Hide the int ids in opaque objects/structs;

var id = level.id;

Here id is such a struct.

This way you can remove the ById(int) method and replace it with ById(Id) and it still lets you keep the cashed Ids for future use.

You can also create unique ID types for each reference type to ensure type safety (so you can't do NodeReference.ById(level.id)).

7
  • I did something similar here: stackoverflow.com/a/2930351/3312 Mar 6, 2013 at 17:35
  • I like this because it's doesn't significantly complicate the API.
    – p.s.w.g
    Mar 6, 2013 at 18:24
  • Figured this would be fairly obvious. Good answer by me. If the developer doesn't want to do it then they can leave.
    – Rig
    Mar 6, 2013 at 19:14
  • I'm aware OP mistakenly asked about constants instead of literals so you're answering the question to the letter; but this answer doesn't really address the intended question on how to avoid developers using literals. Anything can be hardcoded, including an object/struct with its property values. A developer who thinks hardcoding an int is the way to go, is probably just going to hardcode your object/struct as well.
    – Flater
    Nov 28, 2019 at 11:25
  • @Flater But in C# (and many OOP languages) you can prevent outside code from constructing an id struct and restrict construction to only the class the ID pertains to. Nov 28, 2019 at 12:14
23

Provide a better API:

var node = NodeReference.byName("Reporting", "Default", "Patient", _patientGuid.ToString());

Or provide utility functions to fetch important nodes

var node = NodeReference.byName( theDefaultPatientNode(), _patientGuid.ToString() );

Or have the various nodes in an enum, and do something like:

var node = NodeReference.byCategory( REPORT_PATIENT, _patientGuid.ToString() );

You aren't going to be able to sell writing 4 lines of code to do something this simple. They are trying to use ids because its too hard to fetch the relevant object otherwise. So make it easier!

9
  • I've thought about this actually. The problem with this is that there's 2-5 different ways of creating each reference type. ById is the simplest, but every other method requires a separate reference. To provide all methods for generating references in one function call would require on the order of a hundred different overloads. The enum method might work, but that would be up to the client application developer to write.
    – p.s.w.g
    Mar 6, 2013 at 17:37
  • 9
    @p.s.w.g, that just reinforces my impression: it's far too difficult to get these nodes and that's why they are falling back to ids. I think you need to find a way to make it easier to access the nodes rather then slapping their hands for it with ids. Mar 6, 2013 at 22:09
  • You're right. I've been tossing around the idea of a URI style syntax for creating references like NodeReference.ByUri("//Reporting/Default/Patient/#{_patientGuid}"). I guess I'll have to formalize the syntax and write a parser in the next release.
    – p.s.w.g
    Mar 6, 2013 at 22:31
  • 1
    @p.s.w.g Why would you want to use a single string? The solution given by Winston (a method with several parameters) is much better and simpler (no need for a parser).
    – svick
    Mar 6, 2013 at 22:56
  • @svick like I said, ByName is just one way of getting a NodeReference, there are several more. There are also a many ways of creating a LevelReference, AreaReference, and ApplicationReference. I'd have to provide overloads for each method of creating a complete reference chain. With the URI, the developer can do "//Reporting/Default/Patient/#{...}" or "//Reporting/Default/rank:5/parent:jsmith/*" or whatever their application demands.
    – p.s.w.g
    Mar 6, 2013 at 23:03
9

Code like

var level = LevelReference.ById(20);

should never pass a code review, because it contains magic numbers. That alone should be reason enough to ban such practices.

And if they try to work around it by creating named constants, the reviewer should ask the author for where he got the guarantee that the number is correct for all deployments.

2
  • 1
    var app = ApplicationReference.ByName("Reporting"); var area = AreaReference.ByName(app, "Default"); var level = LevelReference.ByName(area, "Patient"); also contain magic numbers
    – Pieter B
    Mar 6, 2013 at 21:44
  • 1
    @PieterB: No, those lines contain string constants whose significance should be immediately apparent to any developer working on the project. The numeric constant 20 does not have such significance. With the strings, if any of the values must change, you can do that with a fairly trivial search/replace, but that does not work for the numeric constant. Mar 7, 2013 at 14:24
5

If I understand correctly, those IDs are the primary keys (identity column) in the database. In this case, a workaround would be not to forbid or track the constants in code, but simply randomize those keys in the deployment script.

Since IDs are not guaranteed to stay the same in the test data, the only solution for the developers would be to move to something more reliable - in your case, names.

1
  • I like this answer, but it is devious and it seems like it could create more work than it's worth during testing. Mar 6, 2013 at 17:14
5

Smack their hand in code review.

That said, is there a real difference between LevelReference.ById() and LevelReference.ByName()? In your example you are using hard coded values in both.

In one code base I've worked on we handled a similar situation by predefining the "hard coded" values that are set up with our database setup script and have an enumeration that maps an enumerated value to the database value.

4
  • 1
    ID's are environment specific. Names are application specific (different applications use this service). Yes both use constants, but the name is a much better choice since it's environment-independent.
    – p.s.w.g
    Mar 6, 2013 at 17:18
  • Right. But there's no way for the compiler to know the difference between which hard coded constant is good and which hard coded constant is bad.
    – Dave Rager
    Mar 6, 2013 at 17:20
  • I was thinking about something kind of like a custom FxCop rule.
    – p.s.w.g
    Mar 6, 2013 at 17:29
  • A custom rule that matched Thing.ById(::digit::+) would just irritate a lazy developer and have them write thing_id = 4 on the line above it. Mar 7, 2013 at 0:15
4

It appears you are trying to find a technical solution to a social problem. You should call it out in code reviews. Do something in your testing/staging environment to ensure that node numbers will never match a clean dev environment - perhaps insert a random number of garbage rows on each install - and throw it back to the dev as broken.

If people insist on trying to work around this, you let them go.

1
  • You're correct that at the core this is a social problem, best fixed with code reviews. Sadly, we don't have the resources to review every check-in. I could periodically rebuild the tables in the stage / dev environment so that the ID's get jumbled up. Kind of like shaking a tree to see what falls out.
    – p.s.w.g
    Mar 6, 2013 at 23:29
3

Doctor! Doctor! It hurts when I do this!

Developers are misusing your API because you've provided it to them. If you think LevelReference.ById() isn't good to use, you shouldn't provide it. If you take it away, they'll stop using it :-)

Since we're talking about C#, you can also mark ById() as [Obsolete] and they'll at least get a deprecation warning. For those who compile with all-warnings-are-errors, that's fatal.

2
  • There are valid use cases for ById -- e.g. caching it from the result of one query and then using it in subsequent queries to avoid the cost of solving the more indirect references. I'd like to not [Obsolete] the method entirely, just raise the flag when they use constants.
    – p.s.w.g
    Mar 6, 2013 at 23:13
  • 1
    But code reviews! Automated builds, with lots of tests! (Seriously, this is the answer I voted for.) Nov 27, 2019 at 14:18
0

Supply an accessor object with getters returning ById(1), ById(2) etc. Then replace all calls to ById(constant) with calls to these getters. Obviously use sensible names for the getters.

Now if your code is really messed up and you used ById(20) to access different data from different sources then you need a second accessor and need to be very careful what to replace each ById call with.

But now you can put the accessors together with your documentation of your ids (I hope it’s all documented) so if ids change, you have one place to change.

If you for example access different versions of the same database at runtime, with different ids, you can create two accessor objects, one for each database version with different ids. This is the point where the hard coded constants would kill you.

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