Oren Eini

aka Ayende Rahien

Oren Eini

CEO of RavenDB

a NoSQL Open Source Document Database

Get in touch with me:

oren@ravendb.net +972 52-548-6969

Posts: 7,575
|
Comments: 51,194

Copyright ©️ Ayende Rahien 2004 — 2025

Privacy Policy · Terms
filter by tags archive
stack view grid view
  • architecture (606) rss
  • bugs (450) rss
  • challanges (123) rss
  • community (377) rss
  • databases (481) rss
  • design (893) rss
  • development (640) rss
  • hibernating-practices (71) rss
  • miscellaneous (592) rss
  • performance (397) rss
  • programming (1085) rss
  • raven (1442) rss
  • ravendb.net (526) rss
  • reviews (184) rss
  • 2025
    • May (7)
    • April (10)
    • March (10)
    • February (7)
    • January (12)
  • 2024
    • December (3)
    • November (2)
    • October (1)
    • September (3)
    • August (5)
    • July (10)
    • June (4)
    • May (6)
    • April (2)
    • March (8)
    • February (2)
    • January (14)
  • 2023
    • December (4)
    • October (4)
    • September (6)
    • August (12)
    • July (5)
    • June (15)
    • May (3)
    • April (11)
    • March (5)
    • February (5)
    • January (8)
  • 2022
    • December (5)
    • November (7)
    • October (7)
    • September (9)
    • August (10)
    • July (15)
    • June (12)
    • May (9)
    • April (14)
    • March (15)
    • February (13)
    • January (16)
  • 2021
    • December (23)
    • November (20)
    • October (16)
    • September (6)
    • August (16)
    • July (11)
    • June (16)
    • May (4)
    • April (10)
    • March (11)
    • February (15)
    • January (14)
  • 2020
    • December (10)
    • November (13)
    • October (15)
    • September (6)
    • August (9)
    • July (9)
    • June (17)
    • May (15)
    • April (14)
    • March (21)
    • February (16)
    • January (13)
  • 2019
    • December (17)
    • November (14)
    • October (16)
    • September (10)
    • August (8)
    • July (16)
    • June (11)
    • May (13)
    • April (18)
    • March (12)
    • February (19)
    • January (23)
  • 2018
    • December (15)
    • November (14)
    • October (19)
    • September (18)
    • August (23)
    • July (20)
    • June (20)
    • May (23)
    • April (15)
    • March (23)
    • February (19)
    • January (23)
  • 2017
    • December (21)
    • November (24)
    • October (22)
    • September (21)
    • August (23)
    • July (21)
    • June (24)
    • May (21)
    • April (21)
    • March (23)
    • February (20)
    • January (23)
  • 2016
    • December (17)
    • November (18)
    • October (22)
    • September (18)
    • August (23)
    • July (22)
    • June (17)
    • May (24)
    • April (16)
    • March (16)
    • February (21)
    • January (21)
  • 2015
    • December (5)
    • November (10)
    • October (9)
    • September (17)
    • August (20)
    • July (17)
    • June (4)
    • May (12)
    • April (9)
    • March (8)
    • February (25)
    • January (17)
  • 2014
    • December (22)
    • November (19)
    • October (21)
    • September (37)
    • August (24)
    • July (23)
    • June (13)
    • May (19)
    • April (24)
    • March (23)
    • February (21)
    • January (24)
  • 2013
    • December (23)
    • November (29)
    • October (27)
    • September (26)
    • August (24)
    • July (24)
    • June (23)
    • May (25)
    • April (26)
    • March (24)
    • February (24)
    • January (21)
  • 2012
    • December (19)
    • November (22)
    • October (27)
    • September (24)
    • August (30)
    • July (23)
    • June (25)
    • May (23)
    • April (25)
    • March (25)
    • February (28)
    • January (24)
  • 2011
    • December (17)
    • November (14)
    • October (24)
    • September (28)
    • August (27)
    • July (30)
    • June (19)
    • May (16)
    • April (30)
    • March (23)
    • February (11)
    • January (26)
  • 2010
    • December (29)
    • November (28)
    • October (35)
    • September (33)
    • August (44)
    • July (17)
    • June (20)
    • May (53)
    • April (29)
    • March (35)
    • February (33)
    • January (36)
  • 2009
    • December (37)
    • November (35)
    • October (53)
    • September (60)
    • August (66)
    • July (29)
    • June (24)
    • May (52)
    • April (63)
    • March (35)
    • February (53)
    • January (50)
  • 2008
    • December (58)
    • November (65)
    • October (46)
    • September (48)
    • August (96)
    • July (87)
    • June (45)
    • May (51)
    • April (52)
    • March (70)
    • February (43)
    • January (49)
  • 2007
    • December (100)
    • November (52)
    • October (109)
    • September (68)
    • August (80)
    • July (56)
    • June (150)
    • May (115)
    • April (73)
    • March (124)
    • February (102)
    • January (68)
  • 2006
    • December (95)
    • November (53)
    • October (120)
    • September (57)
    • August (88)
    • July (54)
    • June (103)
    • May (89)
    • April (84)
    • March (143)
    • February (78)
    • January (64)
  • 2005
    • December (70)
    • November (97)
    • October (91)
    • September (61)
    • August (74)
    • July (92)
    • June (100)
    • May (53)
    • April (42)
    • March (41)
    • February (84)
    • January (31)
  • 2004
    • December (49)
    • November (26)
    • October (26)
    • September (6)
    • April (10)
RavenDB - High-Performance NoSQL Document Database
  previous post next post  
Jun 25 2012

Unprofessional code

time to read 1 min | 79 words

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?

Tweet Share Share 41 comments
Tags:
  • challanges

  previous post next post  

Comments

Stefan
25 Jun 2012
09:10 AM
Stefan

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

Xing Yang
25 Jun 2012
09:13 AM
Xing Yang

Refactor -> Extract Method

Marcin Floryan
25 Jun 2012
09:17 AM
Marcin Floryan

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

Paul Hadfield
25 Jun 2012
09:22 AM
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
25 Jun 2012
09:27 AM
Ayende Rahien

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

Keith Stenson
25 Jun 2012
09:30 AM
Keith Stenson

Ayende, use navigator.geolocation.getCurrentPosition?

James Simm
25 Jun 2012
09:43 AM
James Simm

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

Rikki Mongoose
25 Jun 2012
10:05 AM
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
25 Jun 2012
10:15 AM
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
25 Jun 2012
10:19 AM
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
25 Jun 2012
10:24 AM
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
25 Jun 2012
10:55 AM
Ayende Rahien

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

Ayende Rahien
25 Jun 2012
10:56 AM
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
25 Jun 2012
11:19 AM
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
25 Jun 2012
12:05 PM
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
25 Jun 2012
12:32 PM
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
25 Jun 2012
12:38 PM
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
25 Jun 2012
12:39 PM
Phil

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

Jesse
25 Jun 2012
13:26 PM
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
25 Jun 2012
14:03 PM
Ale Miralles

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

Nick Berardi
25 Jun 2012
14:47 PM
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).

Miguel Lira
25 Jun 2012
15:05 PM
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
25 Jun 2012
15:45 PM
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
25 Jun 2012
16:40 PM
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
25 Jun 2012
17:54 PM
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.

Ayende Rahien
25 Jun 2012
17:57 PM
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
25 Jun 2012
17:57 PM
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
25 Jun 2012
18:57 PM
Max Schmeling

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

bblack
25 Jun 2012
18:59 PM
bblack

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

bblack
25 Jun 2012
19:16 PM
bblack

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

bblack
25 Jun 2012
19:36 PM
bblack

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

Martin
25 Jun 2012
19:43 PM
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
25 Jun 2012
19:59 PM
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
25 Jun 2012
20:53 PM
Alwin

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

Rafal
25 Jun 2012
20:58 PM
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
25 Jun 2012
21:04 PM
Stan Kazakh

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

Robert Slaney
25 Jun 2012
21:13 PM
Robert Slaney

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

Valeriob
25 Jun 2012
22:13 PM
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
25 Jun 2012
23:06 PM
JV

http://pastebin.com/1Ta7hBDy

Kamran Ayub
26 Jun 2012
02:06 AM
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
26 Jun 2012
08:50 AM
nick

@Ayende "depending on your context"

Comment preview

Comments have been closed on this topic.

Markdown formatting

ESC to close

Markdown turns plain text formatting into fancy HTML formatting.

Phrase Emphasis

*italic*   **bold**
_italic_   __bold__

Links

Inline:

An [example](http://url.com/ "Title")

Reference-style labels (titles are optional):

An [example][id]. Then, anywhere
else in the doc, define the link:
  [id]: http://example.com/  "Title"

Images

Inline (titles are optional):

![alt text](/path/img.jpg "Title")

Reference-style:

![alt text][id]
[id]: /url/to/img.jpg "Title"

Headers

Setext-style:

Header 1
========
Header 2
--------

atx-style (closing #'s are optional):

# Header 1 #
## Header 2 ##
###### Header 6

Lists

Ordered, without paragraphs:

1.  Foo
2.  Bar

Unordered, with paragraphs:

*   A list item.
    With multiple paragraphs.
*   Bar

You can nest them:

*   Abacus
    * answer
*   Bubbles
    1.  bunk
    2.  bupkis
        * BELITTLER
    3. burper
*   Cunning

Blockquotes

> Email-style angle brackets
> are used for blockquotes.
> > And, they can be nested.
> #### Headers in blockquotes
> 
> * You can quote a list.
> * Etc.

Horizontal Rules

Three or more dashes or asterisks:

---
* * *
- - - - 

Manual Line Breaks

End a line with two or more spaces:

Roses are red,   
Violets are blue.

Fenced Code Blocks

Code blocks delimited by 3 or more backticks or tildas:

```
This is a preformatted
code block
```

Header IDs

Set the id of headings with {#<id>} at end of heading line:

## My Heading {#myheading}

Tables

Fruit    |Color
---------|----------
Apples   |Red
Pears	 |Green
Bananas  |Yellow

Definition Lists

Term 1
: Definition 1
Term 2
: Definition 2

Footnotes

Body text with a footnote [^1]
[^1]: Footnote text here

Abbreviations

MDD <- will have title
*[MDD]: MarkdownDeep

 

FUTURE POSTS

No future posts left, oh my!

RECENT SERIES

  1. RavenDB News (2):
    02 May 2025 - May 2025
  2. Recording (15):
    30 Apr 2025 - Practical AI Integration with RavenDB
  3. Production Postmortem (52):
    07 Apr 2025 - The race condition in the interlock
  4. RavenDB (13):
    02 Apr 2025 - .NET Aspire integration
  5. RavenDB 7.1 (6):
    18 Mar 2025 - One IO Ring to rule them all
View all series

RECENT COMMENTS

  • Scooletz, Yes, we look at other stuff. Most of those are _not_ allowing you to build themselves incrementally nor are they...
    By Oren Eini on Scaling HNSW in RavenDB: Optimizing for inadequate hardware
  • Have you considered other approaches, DiskSpan, Spann etc.? HNSW, as far as I know, is terrible in regards to updates and det...
    By Scooletz on Scaling HNSW in RavenDB: Optimizing for inadequate hardware
  • Steve, The check is for _equality_ - if the current version matches the value in the versions array. When the `_version` ...
    By Oren Eini on Optimizing the cost of clearing a set
  • I'm not following that part, the first 65K times (until ushort.MaxValue) it will be smaller so you'll enter the if/return, bu...
    By Steve on Optimizing the cost of clearing a set
  • Steve, No, I don't need to. It is going to wrap around anyway, after all
    By Oren Eini on Optimizing the cost of clearing a set

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats
}