Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
    
For what it's worth, add a ByEnum method and then use that everywhere you need to hard-code the desired value. –  Steven Doggart Mar 6 '13 at 17:00
1  
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 '13 at 17:11
12  
Two words: "code reviews." –  Blrfl Mar 6 '13 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 '13 at 18:41
1  
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. –  MichaelT Mar 6 '13 at 23:22
show 2 more comments

7 Answers

up vote 16 down vote accepted

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

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

share|improve this answer
1  
+1 for ninja-ing my answer. –  Telastyn Mar 6 '13 at 17:02
    
I did something similar here: stackoverflow.com/a/2930351/3312 –  Jesse C. Slicer Mar 6 '13 at 17:35
    
I like this because it's doesn't significantly complicate the API. –  p.s.w.g Mar 6 '13 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 '13 at 19:14
add comment

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!

share|improve this answer
    
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 '13 at 17:37
4  
@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. –  Winston Ewert Mar 6 '13 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 '13 at 22:31
    
@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 '13 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 '13 at 23:03
show 4 more comments

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.

share|improve this answer
    
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 '13 at 21:44
    
@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. –  Bart van Ingen Schenau Mar 7 '13 at 14:24
add comment

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.

share|improve this answer
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 '13 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 '13 at 17:20
    
I was thinking about something kind of like a custom FxCop rule. –  p.s.w.g Mar 6 '13 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. –  Sean McSomething Mar 7 '13 at 0:15
add comment

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.

share|improve this answer
    
I like this answer, but it is devious and it seems like it could create more work than it's worth during testing. –  Jonathan Rich Mar 6 '13 at 17:14
add comment

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.

share|improve this answer
    
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 '13 at 23:29
add comment

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.

share|improve this answer
    
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 '13 at 23:13
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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