+972 52-548-6969
Unprofessional code: Answer
In my previous post, I mentioned that I don’t like this code, that it lacked an important *ities, and I asked what was the problem with it.
The answer is that it isn’t easily debuggable. Consider the case where we have a support call from a user “I am in France but I see courses that are running in Australia”.
How do you debug something like that, and how can you work with the user on that.
In this case, adding something like this is enough to make the system much nicer to work with under those scenarios:
This is an internal detail, never expose externally (until this blog post, obviously), but it allows us to override what the system is doing. We can actually pretend to be another user and see what is going on. Without the need of actual debugging any code. That means that you can try things out, in production. It means that you don’t have to work hard to replicate a production environment, get all of the production data and so on.
Thinking like that is important, it drastically reduces your support burden over time.
Comments
Or instead of Reques.UserHostAddress use a "IIPService thing" because you will need more than just UserHostAddress to get the user's IP address in case of him being behind a proxy (for example), abstracting it away leads to easier testing.
I know you'll probably call YAGNI on this one, but knowing the IP of the user doesn't seem like a trivial part of the apps logic, it adds business value, so, why not :)
Just to point out that you better be sure that you don't rely on the IP for some security checks or whether to show or hide some features based on the user location, because you just introduce a way for the user to fake his IP address..
My issue with is that it just reeks of bad UX.
The code is taking an important choice away from the user (what location they are interested in). There is no sign it lets them know what region it has assigned them and doesn't give them the ability to override it.
Even worse... if it can't work out the region it just shows nothing.... ewww....
You probably wouldn't get any support calls.... because the user wouldn't come back.
Given that geo-location is not 100% (to 9 digits...) reliable it should be an progressive enhancement (i.e. auto choice the region in a list).
Rei, Yes, I do call YAGNI on that. And knowing the user's IP actually IS a trivial part of the application.
Bevan, Take a look at http://ravendb.net/events, where you can see this in action. You don't have a way to know from the provided code what kind of a UX you have. As you can see from the actual site, you do have a way to override that in the UI.
Yeah, the end result does look ok... though I can't see any "EventsNearYou" not being anywhere near any of those places.
That said the site should still let people know what region it is detecting them as. Unless I knew that the site had a "EventsNearYou" function I couldn't tell it was there (let alone if it was working or not).
Even telling the user there was nothing in their region would do. Though I suppose you could argue the map does do that as most people should be able to find themselves :/
You could also argue that the IP detection functionality is not critical to the function of that page (though it is too the PartialView) so you already have progressive enhancement.
Hmmm... maybe I'll just leave now :)
You set a high standard. In my day-to-day code, we must be happy to deliver at all. These kind of thoughts are totally too far out. But of course, I get paid by the hour, and that's another factor. Also, I am temp personnel, so I expect not to be here in 1 year time anyway.
Bevan, There is a really big map right in the center of this page. It has dots that shows where we have courses. If you can't find yourself on a world map... maybe you aren't a candidate for one of our courses.
Mark, I make some assumptions, sure. I expect things to _work_.
I believe the location should be a method parameter. The code circled in red could be put in a seperate method.
So you could get something like this: EventsNearYou(getNearbyLocation());
The next step would be to parameterise getNearbyLocation which seems to need an ip string.
Ending up with: EventsNearYou(getNearbyLocation(Request.UserHostAddress ?? ""));
A useful feature, but why one call the absence of it 'unprofessional'? That seems harsh.
Um, that's what you consider "unprofessional?" Wow. That's highly unfair and slightly arrogant to pick that piece of code for that reason and claim the coder as unprofessional.
I agree with Lee. The ability to override the IP in order to debug is very useful, but it doesn't make it unprofessional. Though I suppose it depends on your definition of professional. Some would consider the fact that the method can't be easily tested as unprofessional. Of course, I see your point that not being able to debug to solve a customer's problem is also unprofessional.
I thought it looked unprofessional because it takes some precautions that should be unnecessary. For example Request.UserHostAddress should never be null or blank. I don't see how that's possible. Next, using TryParse when it shouldn't be possible for the address to be malformed. These are things the asp.net runtime should guarantee.
I also agree with Stefan that the location should be bound to a parameter instead of parsed in the controller. The controller shouldn't care where the location is coming from (client IP address, query string parameter, user preference cookie). It should just accept a location as input.
Why not extract that to a CustomModelBinderAttribute?
You extract the "Location" as a parameter of your method and you stamp your newly created CustomModelBinderAttribute to that parameter.
Then, you build a custom IModelBinder for those specifically binded element. Then, you have extracted that Location logic out of the Controller and right into a testable model binder.
No?
Assumption: You are using ASP.NET MVC 3.
Exposing debugging concerns to production code in my opinion isn't professional either. Such problems should be encapsulated in tests.
Maxime, Because I only need it in one place, it is a few lines of code, it is actually important for me to be able to see what is going on if there is something wrong. And not have to hunt for what is happening. Clarify is _important_, and this code is very clear about what it does.
Sla, This isn't a debugging concern, this is a support concern. I need to be able, live on the production system, to be able to replicate a customer scenario. Being able to do so is very helpful.
Awesome! It's exactly those tiny little details that absolutely make the difference between a 'normal' codebase and one that I want to work with.
That's probably the most valuable thing I've learned from your code - things like WaitForUserToContinue or Tasks / Commands with Session as a public property... you know what I mean. Thanks for that.
The only problem I see with it is that the next guy that works with this code will have a really bad time figuring out why the heck you can receive the IP address from a querystring when the environment gives it to you. Maybe even you can get confused after not seeing this code in a few months.
If someone needs to rewrite this piede of functionality they might keep it around "just in case" because they don't know who calls it passing the querystring.
Good for debugging, bad for keeping it.
@Tucaz
I disagree. Why you can receive an IP address from a querystring when the environment gives it too you is clearly documented. It is documented in the name of the querystring variable. The name override-ip very clearly states that it is used to override the user's IP address.
Is it just me or have you introduced a massive security hole. Anyone using GeoLocation as a basis of business rules that allows a simple querystring override is making the situation worse than the original problem.
You should be basing the override from a server side storage mechanism ( ie Session or similar ) that requires an explicit claim or role ( ie Admin or Support ) to be able to set it.
I would be careful about doing stuff like this especially when dealing with sensitive issues. For example a getSocialSecurityNumber method should never accept a user as an argument but impersonate and use the identity from the current caller.
Robert, Security hole? That depend on your actual usage. In this case, we are using Geo Location as a way to show you where you are located so we can find a course nearby. There is no security concern related to changing the location.
It's a principle thing. You might only be using this to locate nearby courses but someone looking at this code and coping verbatim into their code base without understanding the implications ( - "but it's testable therefore it must be ok" ) is an all too common occurance
Robert, Their fault, their problem. I am not going to write code for "what if an idiot is going to copy/paste it" design principles.
"their fault, their problem"
yea, unless you live near nuclear power plant that used code like that for something critical :D
anyway, there are "WIAIIGTC/PI" design principles? tell us more
"claim the coder as unprofessional."
A person is not their code.
I have seen a similar problem with DateTime. Always using DateTime.Now means that a particular LeapYear/Bank Holiday criterion cannot be tested, demonstrated/UATed/Supported without hacking the system clock (when other bad things start to happen).
To me this is a missing usecase, not unprofessional code. Had the usecase been there the dev would never have missed it. You are clearly stating that your support people need to quickly verify what the user is telling you, but considering that you call it unprofessional code and that particular feature is missing in its entirety, i make the assumption that this requirement was never clearly communicated to the dev responsible.
David, This is an general issue, devs should think about the support requirement of their code. In the same sense that we don't state "shouldn't use too much memory", and assume that features wouldn't use gb's of RAM for no good reason.
There is a distinct difference between a "coder" - knows to write code, and a "developer" - who actually write software.
I disagree, IMO expectations like that lead to estimates that are off, and an app with a slew of undocumented features and hooks known only by word of mouth. I also believe one should be carefull when calling people out as unprofessional when they're not conforming to ones personal preferences.
@David L +1
Ayende is right that, that's the better way of doing it. But, that doesn't mean he should expect it from other coders. Your comment is more reflective of the real world, than Ayende's post. The problem with Ayende is that he's way too geeky, he lives inside the .NET framework and he breathes C# instead of air, and eats his own code for food. Thus, in his world, anyone who doesn't code the way he does, is unprofessional.
I always wonder what happens to people like Ayende, when they grow old, do they hate themselves because they can't spit out C# code at the speed they used to? Or do they continue to have so much pride, that they don't realize they are losing it?
Comment preview