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
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
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
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
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
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
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
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
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.