Ayende @ Rahien

Refunds available at head office

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
07/13/2012 09:23 AM by
Giorgi

There are already seven return statements in this snippet.

Christophe
07/13/2012 09:25 AM by
Christophe

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

Tudor
07/13/2012 09:35 AM by
Tudor

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

Chris
07/13/2012 09:37 AM by
Chris

Hard to mock out and test

Nieve
07/13/2012 09:39 AM by
Nieve

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

Giedrius
07/13/2012 09:41 AM by
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
07/13/2012 09:42 AM by
Tomas Herceg

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

Ender
07/13/2012 10:02 AM by
Ender

IMHO this logic should be in a view generator.

Victor
07/13/2012 10:04 AM by
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
07/13/2012 10:11 AM by
Olcay Seker

it seems that you check every property of abstractViewGenerator.

Ayende Rahien
07/13/2012 10:33 AM by
Ayende Rahien

Tomas, Why? It will still have the line number

Petr K.
07/13/2012 10:43 AM by
Petr K.

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

Radek Falhar
07/13/2012 10:53 AM by
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
07/13/2012 11:49 AM by
@dacanetdev

Where are you using indexName inside the lambda expression?

James L
07/13/2012 11:54 AM by
James L

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

Martin Lj
07/13/2012 12:12 PM by
Martin Lj

Feature envy on abstractViewGenerator, use Tell don't Ask

Matt Warren
07/13/2012 12:16 PM by
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
07/13/2012 01:23 PM by
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
07/13/2012 01:34 PM by
Marcus Swope

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

nick
07/13/2012 03:53 PM by
nick

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

Totally fucking shoddy.

jg
07/13/2012 05:02 PM by
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
07/13/2012 05:18 PM by
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
07/13/2012 05:23 PM by
Ken Egozi

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

Tim Burga
07/13/2012 07:37 PM by
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
07/13/2012 11:41 PM by
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
07/14/2012 01:49 AM by
Victor Kornov

@Ken Egozi, +1

The code screams for Refactor -> Extract Method.

Nadav
07/14/2012 09:01 PM by
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
07/15/2012 07:01 AM by
Ayende Rahien

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

Andrea Balducci
07/15/2012 08:07 AM by
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
07/15/2012 08:16 AM by
Andrea Balducci

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

Douglas
07/16/2012 03:48 AM by
Douglas

I feel it is missing some logging/tracing

Icer
07/16/2012 09:32 AM by
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
07/18/2012 06:29 AM by
oskar

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

Ayende Rahien
07/18/2012 07:01 AM by
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
07/18/2012 07:25 AM by
oskar

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

Comments have been closed on this topic.