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

Refactor -> Extract Method

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

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, use navigator.geolocation.getCurrentPosition?

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

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.

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 :(

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)

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

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

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

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.

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

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.

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

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,

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.

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.

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.

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.

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

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

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

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

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.

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.

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

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

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

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

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)

http://pastebin.com/1Ta7hBDy

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.

@Ayende "depending on your context"