Ayende @ Rahien

Ayende @ Rahien

Oren Eini aka Ayende Rahien CEO of Hibernating Rhinos LTD, which develops RavenDB, a NoSQL Open Source Document Database.

Get in touch with me:

oren@ravendb.net

+972 52-548-6969

Posts: 7,408 | Comments: 50,842

Copyright ©️ Ayende Rahien 2004 — 2023

Privacy Policy Terms
filter by tags archive
stack view grid view
  • architecture (545) rss
  • bugs (434) rss
  • challanges (116) rss
  • community (356) rss
  • databases (434) rss
  • design (847) rss
  • development (568) rss
  • hibernating-practices (60) rss
  • miscellaneous (590) rss
  • performance (359) rss
  • programming (1040) rss
  • raven (1352) rss
  • ravendb.net (424) rss
  • reviews (182) rss
  • uberprof (41) rss
  • 2023
    • March (4)
    • 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)
Couchbase vs RavenDB Performance at Rakuten Kobo Whitepaper
  previous post next post  
Sep 30 2008

More code review errors

time to read 1 min | 45 words

Take a look at this method:

image

Now, let us make this simple, shall we?

image

Same meaning, and a significant reduction of complexity. Damn, but this is annoying.

Tweet Share Share 20 comments
Tags:
  • C#

  previous post next post  

Comments

meowth
30 Sep 2008
07:49 AM
meowth

Simply detect and explicitly express preconditions instead of trying to keep one return point, yep?

Frank Quednau
30 Sep 2008
07:51 AM
Frank Quednau

It's funny you're coming with those, yesterday I talked with 2 colleagues about just that kind of stuff you see. Quite often it is paired with the weird obsession to have only 1 exit point. People are out there who actually TALK like that.

While being at it, any reason for comparing to false and not using '!' as well as any reason why the last if'd one-liner is in braces, while the other 1-liners aren't ?

Cheers...:)

Ayende Rahien
30 Sep 2008
07:56 AM
Ayende Rahien

comparing to false is the way I like to do it.

As for the last, it just looks better to me that way

Laurent Kempé
30 Sep 2008
08:08 AM
Laurent Kempé

Thanks to Resharper those code review are easiness!

JonR
30 Sep 2008
08:50 AM
JonR

i strongly agree with all these points (and the ones in the previous post), but isn't it a little OTT to call them "errors"? i mean, if they really were errors, they wouldn't compile.

Romain Verdier
30 Sep 2008
09:02 AM
Romain Verdier

I prefer using "!" and putting one liners in braces every time, but yes, guard clauses make code more easy to read/maintain.

Lars Hundertwasser
30 Sep 2008
09:17 AM
Lars Hundertwasser

I agree with Romain, put one liners in braces. At least you have to be consistent, i.e. you have 3 one liners w/o braces and 1 with...

Markus Zywitza
30 Sep 2008
09:38 AM
Markus Zywitza

This type of code usually exists because code isn't refactored. I don't know who wrote this code, but this was only red/green, not red/green/refactor.

The human mind thinks like the first code snippet: Do the main options first and wrap them with the required conditionals and handle exceptional conditions after that.

The latter code snippet is not how we think (well, at least not as I do): I don't think up all invariants and restrictions up front, but add them as if clauses as I go by. As a result, I refactor every method directly after I have finished it.

Frans Bouma
30 Sep 2008
09:40 AM
Frans Bouma

fieldBridge is casted twice to ITwoWayFieldBridge, one should use 'as' and test for null.

It might be my C background, but I would test for < 0 instead of == -1.

The most weird thing still is that the method header accepts IFieldBridge, but effectively crashes (exception) if the parameter isn't an ITwoWayFieldBridge, i.o.w.: the method should have its fieldBridge parameter be typed as ITwoWayFieldBridge and let the compiler deal with calls to the method.

Also, 1 line if clauses should IMHO also be inside {}.

Ayende Rahien
30 Sep 2008
09:55 AM
Ayende Rahien

Frans,

This method is called in one code path where the IFieldBridge impl should also be ITwoWayFieldBridge. If you don't enter this code path, you don't have to be IFieldBridge.

That is why the check is made dynamically.

Paul Hatcher
30 Sep 2008
10:47 AM
Paul Hatcher

Oren

The reason I wrote it that way was that it is a direct port of the Java code and I'd prefer to keep the code bases the same until the port it complete.

Once it's all there, then refactoring makes sense, but until it is, trying to figure out if someone's refactored code does the same job as the original Java can be really problematic.

Michael McCurrey
30 Sep 2008
11:31 AM
Michael McCurrey

" __i strongly agree with all these points (and the ones in the previous post), but isn't it a little OTT to call them "errors"? i mean, if they really were errors, they wouldn't compile. "

Just because something compiles, doesn't mean it won't error.

Robert Taylor
30 Sep 2008
12:07 PM
Robert Taylor

The difference between the 1-line If-statements with vs. without braces is that for the ones without braces no code after those statements could ever possibly be executed. If someone needed to come along and add some additional statements it should be clear that it can't go after the throw statement in the same if-block.

jonnii
30 Sep 2008
13:50 PM
jonnii

I tend to put error conditions at the top of my methods, as they're the first thing I write tests for so my code ends up looking like this by default.

I find it far easier to read.

Ayende Rahien
30 Sep 2008
14:29 PM
Ayende Rahien

Paul,

I should have been more clear.

I am showing the C# code because it is what most of my readers will understand.

The fault lies in the Java code, I agree. It is bad in either language.

meisinger
30 Sep 2008
14:45 PM
meisinger

interesting...

i would have tested "store" and "fieldBridge" first so that i wouldn't waste a call to "GetFieldPosition"

lord only knows what that method is doing

(i guess i have been bitten by too many "bad" calls in the past)

the other thing that interests me is the fact that there is no check to see if "matchingPosition" is within the bounds of the "result" array

lastly... (this is my personal "downfall")

i would use a string.Format for the exception messages

just my 2 cents

Bruno
30 Sep 2008
15:15 PM
Bruno

@meisinger

String.Format is too much for simple string concatenations following the pattern 1 literal + 1 variable. It uses StringBuilder. You can check the stack trace causing an error due to bad formating.

JonR
01 Oct 2008
09:30 AM
JonR

__Just because something compiles, doesn't mean it won't error.

:rolleyes:

i'll give you that, but i'm sure you see what i'm getting at.

Manav
01 Oct 2008
16:42 PM
Manav

Disagree whole-heartedly with multiple return value point. There is another easy way to refactor the code i..e using goto Cleanup. That is the standard way we use in C/C++ but I have not used C# so don't know if goto exists or not.

Mohammad Tayseer
02 Oct 2008
01:40 AM
Mohammad Tayseer

@Manav

The error case should be handled first, to free your mind from remembering if the error was handled or not.

In C++, you shouldn't use goto Cleanup. Use destructors & smart pointers instead. You don't want your resource cleanup to be all over the place

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. Recording (8):
    17 Feb 2023 - RavenDB Usage Patterns
  2. Production postmortem (48):
    27 Jan 2023 - The server ate all my memory
  3. Answer (12):
    05 Jan 2023 - what does this code print?
  4. Challenge (71):
    04 Jan 2023 - what does this code print?
  5. RavenDB Indexing (2):
    20 Oct 2022 - exact()
View all series

RECENT COMMENTS

  • Tobias, The sharding features are meant for _very_ large databases. They are going to be available in the development and en...
    By Oren Eini on RavenDB 6.0 live instance is now up & running: Come test it out!
  • are any of the sharding features available in the community edition?
    By Tobias Zürcher on RavenDB 6.0 live instance is now up & running: Come test it out!
  • peter, It means that you cannot import a CSV file directly into RavenDB. Something that very few people are actually using...
    By Oren Eini on RavenDB 6.0 live instance is now up & running: Come test it out!
  • Can you explain this limitation? >> Import documents from a CSV file into a collection is not available for sharded databa...
    By peter on RavenDB 6.0 live instance is now up & running: Come test it out!
  • Trayan, Oh, I see. In your case, yes, you will add the external finalizer when the item is removed from the cache, and ...
    By Oren Eini on ExternalFinalizer: Adding a finalizer to 3rd party objects

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats