http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660@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?http://ayende.com/156481/unprofessional-code-answer#comment33http://ayende.com/156481/unprofessional-code-answer#comment33Sun, 15 Jul 2012 07:18:20 GMTI 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.http://ayende.com/156481/unprofessional-code-answer#comment32http://ayende.com/156481/unprofessional-code-answer#comment32Fri, 13 Jul 2012 12:57:47 GMTDavid, 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.http://ayende.com/156481/unprofessional-code-answer#comment31http://ayende.com/156481/unprofessional-code-answer#comment31Fri, 13 Jul 2012 10:32:45 GMTTo 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.http://ayende.com/156481/unprofessional-code-answer#comment30http://ayende.com/156481/unprofessional-code-answer#comment30Fri, 13 Jul 2012 10:07:48 GMTI 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). http://ayende.com/156481/unprofessional-code-answer#comment29http://ayende.com/156481/unprofessional-code-answer#comment29Tue, 03 Jul 2012 09:03:20 GMT"claim the coder as unprofessional." A person is not their code.http://ayende.com/156481/unprofessional-code-answer#comment28http://ayende.com/156481/unprofessional-code-answer#comment28Fri, 29 Jun 2012 13:10:02 GMT"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 morehttp://ayende.com/156481/unprofessional-code-answer#comment27http://ayende.com/156481/unprofessional-code-answer#comment27Thu, 28 Jun 2012 08:31:46 GMTRobert, Their fault, their problem. I am not going to write code for "what if an idiot is going to copy/paste it" design principles.http://ayende.com/156481/unprofessional-code-answer#comment26http://ayende.com/156481/unprofessional-code-answer#comment26Thu, 28 Jun 2012 07:32:10 GMTIt'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 occurancehttp://ayende.com/156481/unprofessional-code-answer#comment25http://ayende.com/156481/unprofessional-code-answer#comment25Thu, 28 Jun 2012 05:15:14 GMTRobert, 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.http://ayende.com/156481/unprofessional-code-answer#comment24http://ayende.com/156481/unprofessional-code-answer#comment24Thu, 28 Jun 2012 04:52:31 GMTI 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.http://ayende.com/156481/unprofessional-code-answer#comment23http://ayende.com/156481/unprofessional-code-answer#comment23Thu, 28 Jun 2012 04:00:55 GMTIs 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.http://ayende.com/156481/unprofessional-code-answer#comment22http://ayende.com/156481/unprofessional-code-answer#comment22Wed, 27 Jun 2012 21:30:00 GMT@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. http://ayende.com/156481/unprofessional-code-answer#comment21http://ayende.com/156481/unprofessional-code-answer#comment21Wed, 27 Jun 2012 03:26:02 GMTThe 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. http://ayende.com/156481/unprofessional-code-answer#comment20http://ayende.com/156481/unprofessional-code-answer#comment20Wed, 27 Jun 2012 02:43:35 GMTAwesome! 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.http://ayende.com/156481/unprofessional-code-answer#comment19http://ayende.com/156481/unprofessional-code-answer#comment19Tue, 26 Jun 2012 18:28:11 GMTSla, 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.http://ayende.com/156481/unprofessional-code-answer#comment18http://ayende.com/156481/unprofessional-code-answer#comment18Tue, 26 Jun 2012 15:40:31 GMTMaxime, 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.http://ayende.com/156481/unprofessional-code-answer#comment17http://ayende.com/156481/unprofessional-code-answer#comment17Tue, 26 Jun 2012 15:39:54 GMTExposing debugging concerns to production code in my opinion isn't professional either. Such problems should be encapsulated in tests.http://ayende.com/156481/unprofessional-code-answer#comment16http://ayende.com/156481/unprofessional-code-answer#comment16Tue, 26 Jun 2012 15:15:58 GMTWhy 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.http://ayende.com/156481/unprofessional-code-answer#comment15http://ayende.com/156481/unprofessional-code-answer#comment15Tue, 26 Jun 2012 14:10:51 GMTI 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.http://ayende.com/156481/unprofessional-code-answer#comment14http://ayende.com/156481/unprofessional-code-answer#comment14Tue, 26 Jun 2012 13:54:19 GMTI 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.http://ayende.com/156481/unprofessional-code-answer#comment13http://ayende.com/156481/unprofessional-code-answer#comment13Tue, 26 Jun 2012 13:32:55 GMTUm, 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.http://ayende.com/156481/unprofessional-code-answer#comment12http://ayende.com/156481/unprofessional-code-answer#comment12Tue, 26 Jun 2012 13:24:56 GMTA useful feature, but why one call the absence of it 'unprofessional'? That seems harsh.http://ayende.com/156481/unprofessional-code-answer#comment11http://ayende.com/156481/unprofessional-code-answer#comment11Tue, 26 Jun 2012 13:12:29 GMTI 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 ?? "")); http://ayende.com/156481/unprofessional-code-answer#comment10http://ayende.com/156481/unprofessional-code-answer#comment10Tue, 26 Jun 2012 11:30:41 GMTMark, I make some assumptions, sure. I expect things to _work_.http://ayende.com/156481/unprofessional-code-answer#comment9http://ayende.com/156481/unprofessional-code-answer#comment9Tue, 26 Jun 2012 11:20:19 GMTBevan, 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.http://ayende.com/156481/unprofessional-code-answer#comment8http://ayende.com/156481/unprofessional-code-answer#comment8Tue, 26 Jun 2012 11:20:03 GMTYou 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. http://ayende.com/156481/unprofessional-code-answer#comment7http://ayende.com/156481/unprofessional-code-answer#comment7Tue, 26 Jun 2012 10:55:52 GMTYeah, 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 :)http://ayende.com/156481/unprofessional-code-answer#comment6http://ayende.com/156481/unprofessional-code-answer#comment6Tue, 26 Jun 2012 10:45:57 GMTBevan, 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.http://ayende.com/156481/unprofessional-code-answer#comment5http://ayende.com/156481/unprofessional-code-answer#comment5Tue, 26 Jun 2012 10:24:29 GMTRei, Yes, I do call YAGNI on that. And knowing the user's IP actually IS a trivial part of the application.http://ayende.com/156481/unprofessional-code-answer#comment4http://ayende.com/156481/unprofessional-code-answer#comment4Tue, 26 Jun 2012 10:23:29 GMT