There are already seven return statements in this snippet.

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

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

Hard to mock out and test

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

Odd to see Linq used that way...

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.

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

IMHO this logic should be in a view generator.

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

it seems that you check every property of abstractViewGenerator.

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

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

No logging?

Where are you using indexName inside the lambda expression?

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

Feature envy on abstractViewGenerator, use Tell don't Ask

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

Six line of comments !

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.

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

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

Totally fucking shoddy.

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.

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

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

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.

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.

@Ken Egozi, +1

The code screams for Refactor -> Extract Method.

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) { } 

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

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

I feel it is missing some logging/tracing

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

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

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