Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 5,953 | Comments: 44,409

filter by tags archive

Unprofessional code, take II


Since the previous time that I posted unprofessional code, I got a lot of comments about how I am cruel to poor developers, I decided to post some of my own code that isn’t professional.

The following is part of the RavenDB Dynamic Query Optimizer:

image

Hint, this single expression goes on for about 120 lines (the length isn’t the reason it is not professional) and is quite important for RavenDB.


Comments

Giorgi

There are already seven return statements in this snippet.

Christophe

I'm guessing the amount of it-statements + returns has something to do with it?

Tudor

Probably all those IFs should be in a method/function of abstractViewGenerator..

Chris

Hard to mock out and test

Nieve

Feature envy code smell on abstractViewGeneretor? looks like all these calls could be extracted into abstractViewGeneretor as one sole method?

Giedrius

I don't see the problem with many returns. I would say it would be hard to trace why index name was skipped, so may be comments should transform somehow in skipping reason result.

Tomas Herceg

When the code in lambda throws an exception it will be difficult to track on which line the problem is.

Ender

IMHO this logic should be in a view generator.

Victor

I dont see the point of all the checks done inside the Where clause. Are they related to indexName at all? If they are not they should be outside the Where clause so they get executed only once...

Olcay Seker

it seems that you check every property of abstractViewGenerator.

Ayende Rahien

Tomas, Why? It will still have the line number

Petr K.

In the first place, posting code as a screenshot is what I call unprofessional..

Radek Falhar

I agree with Nieve. The whole thing looks like it only works with abstract view generator and should be refactored as it's method.

@dacanetdev

Where are you using indexName inside the lambda expression?

James L

Too much green writing. I'd extract methods with descriptive names

Martin Lj

Feature envy on abstractViewGenerator, use Tell don't Ask

Matt Warren

dacanetdev & victor,

Unfortunately the first line inside the Where(..) statement is obscured, it reads:

var abstractViewGenerator = database.IndexDefinitionStorage.GetViewGenerator(indexName);

So indexName does get used ;-)

Sam Naseri

In snapshot, all the return statements are returning false value, so the where method basically is filtering out all the items and will return an empty sequence. Unless the side effect was desired this code is actually doing nothing expect than return database.IndexDefinitionStorage.IndexNames.Where(i=>false)

But it seems that there is more code later.

Marcus Swope

@Petr I agree, it should have been text so that we could copy-paste it ;-)

nick

Sorry mate. You just didn't abstract your database underneath enough layers.

Totally fucking shoddy.

jg
jg

It has an awful lot of logic and I can't see the point of it immediately, so I would personally prefer it to be in a method that told me why all this logic is being performed, although having said that it may well be the implementation of said method given it returns the results.

I would guess the primary reason you say this is unprofessional is that if something showed up/didn't show in the results that was expected there is no easy way to find out (without debugging) why it is/isn't in the result set.

Slava

Logic backed in into IFs - unverifiable by compiler or runtime. No one except you would be able to verify it. You would forget it sooner or later. Its a sin to use staticly typed language i totaly untyped way. It probably would be faster and safer to turn this code into composable functions in some untype language as Javascript. :)

Good solution "might be" to use typed Expression trees (or just typed dispatch). Not only you would gain guarantees of compiler, logic backed into types - readable and self documenting, you would also beneft in live support testing as well, since you would be able to easily put hooks to construct and inject your logic into expression tree dynamicly while still enoying safety of types

Ken Egozi

Obvious - the code is cut in the righthand side, so it would probably not compile

Tim Burga

Agree with the diagnosis but not the solutions. I'd probably push the logic further back than just the abstractViewGenerator.. perhaps by adding a GetIndexNamesForEntity(entityName) method to IndexDefinitionStorage.

David

You don't get automatic function documentation for free if it's buried in a lambda within a body - better to relegate the predicate to it's own method.

Victor Kornov

@Ken Egozi, +1

The code screams for Refactor -> Extract Method.

Nadav

I gotta say that both examples are not that unprofessional, and I think you're using this term too freely. I've seen much much worse examples, like hundreds of pl-sql/t-sql statements, logic in triggers, etc.

Talking about unprofessional code, here's one of mine:

warning Busy Wait!! use auto reset event

        while (paused) { } 
Ayende Rahien

Nadav, There is a difference between BAD code and unprofessional code.

Andrea Balducci

In my code would be (line 55) .Where(i => IsIndexMatchingRule1AndRule2AndSoOn.. I've to read in a simple way what the code is supposed to do, it's all about maintainability

Andrea Balducci

and Rule should be something with business meaning instead of a summary of the filter implementation

Douglas

I feel it is missing some logging/tracing

Icer

I think the implementation whether the index shall be returned should be decided in the abstractViewGenerator, so that the consumer of the abstractViewGenerator does not need to know about all the implementation details.

Result would be something like: var abstractViewGenerator = database.IndexDefinitionStorage.GetViewGenerator(indexName); if(absctractViewGenerator == null) {return false;} if(!abstractViewGenerator.FullfillsSomeConditions()) {return false;} return true;

This makes the code much more readable and the logic is encapsulated in the class that knows what to do. :)

oskar

Just one unrelated question: Why is it if (a.HasWhereClause) ? Or is it just the renderer that have removed the "!"?

Ayende Rahien

Oskar, If it has a where clause, we can't use it as an index for dynamic queries. The code is good there.

oskar

Sorry, Just got confused by the comment for the "if".

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

No future posts left, oh my!

RECENT SERIES

  1. The RavenDB Comic Strip (3):
    28 May 2015 - Part III – High availability & sleeping soundly
  2. Special Offer (2):
    27 May 2015 - 29% discount for all our products
  3. RavenDB Sharding (3):
    22 May 2015 - Adding a new shard to an existing cluster, splitting the shard
  4. Challenge (45):
    28 Apr 2015 - What is the meaning of this change?
  5. Interview question (2):
    30 Mar 2015 - fix the index
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats