Ayende @ Rahien

It's a girl

Unprofessional code

This code bugs me.

image

It isn’t wrong, and it is going to produce the right result. But is ain’t pro code, in the sense that this code lacks an important *ities.

Just to cut down on the guessing, I marked the exact part that bugs me. Can you see the issue?

Comments

Stefan
06/25/2012 09:10 AM by
Stefan

Why would one do IPAddress.TryParse on en empty string?

Xing Yang
06/25/2012 09:13 AM by
Xing Yang

Refactor -> Extract Method

Marcin Floryan
06/25/2012 09:17 AM by
Marcin Floryan

Dependency on the Request for a start and it all feels like it happens in the wrong place anyway.

Paul Hadfield
06/25/2012 09:22 AM by
Paul Hadfield

If you are behind a corporate proxy then this code is always next to useless! I might be a remote office and the proxy is held in the HQ 'x' miles away, or in a different country! I want to see events near me, not the proxy I'm going through!

Ayende Rahien
06/25/2012 09:27 AM by
Ayende Rahien

Paul, Great, can you figure out another way to do that?

Keith Stenson
06/25/2012 09:30 AM by
Keith Stenson

Ayende, use navigator.geolocation.getCurrentPosition?

James Simm
06/25/2012 09:43 AM by
James Simm

Use Request["X-Forwarded-For"] and hope the proxy is configured to pass this header through?

Rikki Mongoose
06/25/2012 10:05 AM by
Rikki Mongoose

Why not

if( !String.IsNullOrEmpty(Request.UserHostAddress) && IPAddress.TryParse(Request.UserHostAddress, out address) )

Request.UserHostAddress is already string, we don't need to create var's to get it.

Justin A
06/25/2012 10:15 AM by
Justin A

I'm with Paul and James :

I would create an extension method on an HttpRequestBase which checks for the X-Forwarded-For before it checks for the UserHostAddress.

I've had sites behind a load balancers and have to get them to add an X-Forwaded-For header with the source IP.

Of course, X-Forwarded-For is not compulsory so if you get it, you're lucky! Lots of time, it's just the IP of the previous 'hop' .. not always the clientIp.

And why bother checking for an IP that is empty (oh. pet peeve. please using string.Empty or String.Empty instead of "").

TL;DR; never assume you -really- have a user's IP address :(

Paul Hadfield
06/25/2012 10:19 AM by
Paul Hadfield

Hi Ayende,

Are you looking for an automated version, or an improvement in the UI/Process. If you wanted an automated version then see if the other recommendations would work / investigate more (I've not had to do this)

For the later: If you're logged in / registered then maybe you've already recorded the location(s) you're interested in. Otherwise, maybe use the IP address resolution as a default and display a prompt that says "are you interested in this location?" with a quick an easy way to change the location if not (rather than assuming they are always interested)

Paul Hadfield
06/25/2012 10:24 AM by
Paul Hadfield

Sorry, hit enter too early. It goes without saying (like previous comments) that the code highlighted in red should be extracted ideally into a component that could be replaced / extended. Maybe you'd have a list of "locators" with the most precise called first, failing back to the least precise. If a component can't determine your location from the supplied information it returns a null, so code can call next one in the list.

At that point how you resolve the location just becomes a mere detail that can be constantly improved without having to change this code.

Ayende Rahien
06/25/2012 10:55 AM by
Ayende Rahien

Keith, a) that is only available on the client side b) that requires user permission

Ayende Rahien
06/25/2012 10:56 AM by
Ayende Rahien

James, If you are using a proxy, usually your internal IP is a local one, and thus not really relevant for GeoIP purposes

SanderD
06/25/2012 11:19 AM by
SanderD

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

Mike
06/25/2012 12:05 PM by
Mike

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

nick
06/25/2012 12:32 PM by
nick

Depending 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 :)

Wyatt Barnett
06/25/2012 12:38 PM by
Wyatt Barnett

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

Phil
06/25/2012 12:39 PM by
Phil

I've never done it, but could you write the client ip out to a hidden field whilst rendering the page?

Jesse
06/25/2012 01:26 PM by
Jesse

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

Ale Miralles
06/25/2012 02:03 PM by
Ale Miralles

the first line should check hostadrress == "", if so, returns the corresponding view, if not the do the whole work.

Miguel Lira
06/25/2012 03:05 PM by
Miguel Lira

Ayende,

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.

Matthew Douglass
06/25/2012 03:45 PM by
Matthew Douglass

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

Dave Walker
06/25/2012 04:40 PM by
Dave Walker

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

Ayende Rahien
06/25/2012 05:54 PM by
Ayende Rahien

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

Nick Berardi
06/25/2012 02:47 PM by
Nick Berardi

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

Ayende Rahien
06/25/2012 05:57 PM by
Ayende Rahien

Nick, 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?

Ayende Rahien
06/25/2012 05:57 PM by
Ayende Rahien

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

Max Schmeling
06/25/2012 06:57 PM by
Max Schmeling

Why have an extra if check when you could use the one you already have?

bblack
06/25/2012 06:59 PM by
bblack

add override = > GeoSpace.GetLocationByIP(string ip) which returns null location if ip is incorrect ?

bblack
06/25/2012 07:16 PM by
bblack

sorry for re-post (edit to above) overload method preferred?: GeoSpace.GetLocationByIP(string ipAddress) which returns null location if ipAddress is incorrect ?

bblack
06/25/2012 07:36 PM by
bblack

might be nice to know that your results were effected by a lack of or a malformed ip address.

Martin
06/25/2012 07:43 PM by
Martin

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

Thomas Krause
06/25/2012 07:59 PM by
Thomas Krause

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

Alwin
06/25/2012 08:53 PM by
Alwin

The action is using the Request, but this is not clear from outside the function.

Rafal
06/25/2012 08:58 PM by
Rafal

Can 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

Stan Kazakh
06/25/2012 09:04 PM by
Stan Kazakh

What about an overloaded version of GetLocationByIp() that takes a string?

Robert Slaney
06/25/2012 09:13 PM by
Robert Slaney

My immediate thought was about IPv4 and IPv6. The IPAddress.TryParse is expecting an IPv4 format

Valeriob
06/25/2012 10:13 PM by
Valeriob

I 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)

JV
06/25/2012 11:06 PM by
JV

http://pastebin.com/1Ta7hBDy

Kamran Ayub
06/26/2012 02:06 AM by
Kamran Ayub

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

nick
06/26/2012 08:50 AM by
nick

@Ayende "depending on your context"

Comments have been closed on this topic.