http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660@Ayende "depending on your context"http://ayende.com/156450/unprofessional-code#comment41http://ayende.com/156450/unprofessional-code#comment41Tue, 26 Jun 2012 08:50:08 GMTI was thinking of guessing someone's timezone based on their IP by maybe looking it up via a service or whatever. I ended up defaulting to the server timezone and letting the user pick it. It was much easier that way than trying to a) find a service to use, b) making a possibly totally wrong guess, and c) spend extra time. If it requires more knowledge of what exactly is happening, I can't say what the issue is off-hand. I do have questions around the units of the radius (200km, 200m, 200in?) and why they are only taking 2 events. Overall I would probably offer much more specific ways of looking up location (like Valeriob suggests) and fallback on this as a last resort, if I had to do it at all.http://ayende.com/156450/unprofessional-code#comment40http://ayende.com/156450/unprofessional-code#comment40Tue, 26 Jun 2012 02:06:10 GMThttp://pastebin.com/1Ta7hBDyhttp://ayende.com/156450/unprofessional-code#comment39http://ayende.com/156450/unprofessional-code#comment39Mon, 25 Jun 2012 23:06:32 GMTI would use this service on the client http://www.myipaddress.com/show-my-ip-address/ and pass the ip address as parameter. And maybe implement an overload with lat/long as parameters for situations where i have user consent to acquire location (and mobile apps)http://ayende.com/156450/unprofessional-code#comment38http://ayende.com/156450/unprofessional-code#comment38Mon, 25 Jun 2012 22:13:04 GMTMy immediate thought was about IPv4 and IPv6. The IPAddress.TryParse is expecting an IPv4 formathttp://ayende.com/156450/unprofessional-code#comment37http://ayende.com/156450/unprofessional-code#comment37Mon, 25 Jun 2012 21:13:14 GMTWhat about an overloaded version of GetLocationByIp() that takes a string?http://ayende.com/156450/unprofessional-code#comment36http://ayende.com/156450/unprofessional-code#comment36Mon, 25 Jun 2012 21:04:16 GMTCan I have a try? 1. You do geolocation db search on each request which requires a GeoSession to be opened every time 2. Location information is not stored anywhere so you'll have to do it again in every page that needs the location 3. There's no way for the user to choose a different location if the IP-based location fails or gives incorrect result for some reason http://ayende.com/156450/unprofessional-code#comment35http://ayende.com/156450/unprofessional-code#comment35Mon, 25 Jun 2012 20:58:56 GMTThe action is using the Request, but this is not clear from outside the function.http://ayende.com/156450/unprofessional-code#comment34http://ayende.com/156450/unprofessional-code#comment34Mon, 25 Jun 2012 20:53:33 GMTI would invert the if to have more linear-ITY in the function: if(!TryParse(...)) return PartialView(...); var location = ... I like the normal function flow to be as linear as possible without any branches and use the if conditions only as early exit points/exceptions in my code. It's much easier to read.http://ayende.com/156450/unprofessional-code#comment33http://ayende.com/156450/unprofessional-code#comment33Mon, 25 Jun 2012 19:59:31 GMTThere's absolutely NO reason to define location outside the scope. If it's not null, you return. you should have the definition of location inside the if statement of the try parse. if (TryParse) { var location = blah... return partial view; } I hate when i see this kind of stuff in code too.http://ayende.com/156450/unprofessional-code#comment32http://ayende.com/156450/unprofessional-code#comment32Mon, 25 Jun 2012 19:43:32 GMTmight be nice to know that your results were effected by a lack of or a malformed ip address.http://ayende.com/156450/unprofessional-code#comment31http://ayende.com/156450/unprofessional-code#comment31Mon, 25 Jun 2012 19:36:03 GMTsorry for re-post (edit to above) overload method preferred?: GeoSpace.GetLocationByIP(string ipAddress) which returns null location if ipAddress is incorrect ?http://ayende.com/156450/unprofessional-code#comment30http://ayende.com/156450/unprofessional-code#comment30Mon, 25 Jun 2012 19:16:01 GMTadd override = > GeoSpace.GetLocationByIP(string ip) which returns null location if ip is incorrect ? http://ayende.com/156450/unprofessional-code#comment29http://ayende.com/156450/unprofessional-code#comment29Mon, 25 Jun 2012 18:59:45 GMTWhy have an extra if check when you could use the one you already have?http://ayende.com/156450/unprofessional-code#comment28http://ayende.com/156450/unprofessional-code#comment28Mon, 25 Jun 2012 18:57:02 GMTMike, and others. I am quite amazed by how many people get all too hung up about minor code details. The actual issue that I had in mind was something else all together. And the micro seconds you'll spend on the emtpy string never crossed my mind. http://ayende.com/156450/unprofessional-code#comment27http://ayende.com/156450/unprofessional-code#comment27Mon, 25 Jun 2012 17:57:27 GMTNick, In the entire application, there is just a single location where this is used. Why on earth would I want to encapsulate that before I have a need to do so?http://ayende.com/156450/unprofessional-code#comment26http://ayende.com/156450/unprofessional-code#comment26Mon, 25 Jun 2012 17:57:09 GMTMike, and others. I am quite amazed by how many people get all too hung up about minor code details. The actual issue that I had in mind was something else all together. And the micro seconds you'll spend on the emtpy string never crossed my mind.http://ayende.com/156450/unprofessional-code#comment25http://ayende.com/156450/unprofessional-code#comment25Mon, 25 Jun 2012 17:54:32 GMTApart from that the method has about 1000 things/rules going on with only one descriptor (method name)? Even the Events have so much logic going on - they are within 200 somethings of your lat/long. They are future events, ordered. And there are only two of them. Method has far too much responsibility.http://ayende.com/156450/unprofessional-code#comment24http://ayende.com/156450/unprofessional-code#comment24Mon, 25 Jun 2012 16:40:26 GMTTo me the biggest annoyance with that block is that is is a function with one input (Request.UserHostAddress) and one output (Location) that failed to get abstracted out to its own function.http://ayende.com/156450/unprofessional-code#comment23http://ayende.com/156450/unprofessional-code#comment23Mon, 25 Jun 2012 15:45:24 GMTAyende, It should be noted that a proper X-Forwarded-For *can* be a collection of proxys along the request chain usually in the format of client, proxy1, proxy2, etc. so parse accordingly. However, don't be quick to discard Keith's suggestion. Getting to an end user's IP address implicity will be a uphill climb unless you leverage some client side solution (javascript, flash, active-x) so why not explicitly ask the client for this information? Personally, I like Paul's suggestion of gathering location via something other than IP and prompting the user accordingly. http://ayende.com/156450/unprofessional-code#comment22http://ayende.com/156450/unprofessional-code#comment22Mon, 25 Jun 2012 15:05:48 GMTThe location should be passed in or determined by an alternate method, through one of the many methods supported by browsers, by IP, Geolocation header, and even lat or long, or address through query string. But regardless of the method, the code should be extracted so that it can be used consistently through out the entire application. Maybe something like var location = Helper.GetLocation(Request).http://ayende.com/156450/unprofessional-code#comment21http://ayende.com/156450/unprofessional-code#comment21Mon, 25 Jun 2012 14:47:42 GMTthe first line should check hostadrress == "", if so, returns the corresponding view, if not the do the whole work. http://ayende.com/156450/unprofessional-code#comment20http://ayende.com/156450/unprofessional-code#comment20Mon, 25 Jun 2012 14:03:36 GMTThe method does too much. Getting the location should be the job of some other service/object. It could be as simple as creating a view model for this action and then a custom model binder to wire it up.http://ayende.com/156450/unprofessional-code#comment19http://ayende.com/156450/unprofessional-code#comment19Mon, 25 Jun 2012 13:26:42 GMTI've never done it, but could you write the client ip out to a hidden field whilst rendering the page?http://ayende.com/156450/unprofessional-code#comment18http://ayende.com/156450/unprofessional-code#comment18Mon, 25 Jun 2012 12:39:50 GMTI wonder how slow the lookup for GetLocationByIp is -- if that is a remote call then it could really be ugly. Also should probably cache that once you've got it.http://ayende.com/156450/unprofessional-code#comment17http://ayende.com/156450/unprofessional-code#comment17Mon, 25 Jun 2012 12:38:10 GMTDepending on your context that might be a bit much to lump all that in the controller. You can't test it easily because of the dependency on request. Bet you're missing the catch all exceptions and log it to somewhere nobody ever checks. That's how the pros roll :)http://ayende.com/156450/unprofessional-code#comment16http://ayende.com/156450/unprofessional-code#comment16Mon, 25 Jun 2012 12:32:04 GMTUnprofessional? Really? This strikes me a much-a-do about nothing. I probably would not use a local for the ipString myself but it's a minor point. Running through Try.Parse on an empty string is maybe a a few cycles more but how often are you going to hit that case (maybe never depending on the calling code). It would be interesting to measure the actual performance checking for null/empty vs. the current method for the using the expected occurrence frequency for empty strings and publishing the results. Now that would be professional... http://ayende.com/156450/unprofessional-code#comment15http://ayende.com/156450/unprofessional-code#comment15Mon, 25 Jun 2012 12:05:46 GMT@Ayende: in one of our clients' situation, we have two Amazon EC2 instances behind an Amazon load-balancer. The Request.UserHostAddress is the one of the loadbalancer, and the X-Forwarded-For-header is set to the original clients' ip by the loadbalancer. We have installed an IIS module for that, called ARR Helper (http://blogs.iis.net/anilr/archive/2009/03/03/client-ip-not-logged-on-content-server-when-using-arr.aspx), which re-writes the X-Forwarded-For header into the UserHostAddress. http://ayende.com/156450/unprofessional-code#comment14http://ayende.com/156450/unprofessional-code#comment14Mon, 25 Jun 2012 11:19:43 GMTJames, If you are using a proxy, usually your internal IP is a local one, and thus not really relevant for GeoIP purposeshttp://ayende.com/156450/unprofessional-code#comment13http://ayende.com/156450/unprofessional-code#comment13Mon, 25 Jun 2012 10:56:14 GMTKeith, a) that is only available on the client side b) that requires user permission http://ayende.com/156450/unprofessional-code#comment12http://ayende.com/156450/unprofessional-code#comment12Mon, 25 Jun 2012 10:55:41 GMT