http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660Sorry, Just got confused by the comment for the "if".http://ayende.com/156993/unprofessional-code-take-ii#comment38http://ayende.com/156993/unprofessional-code-take-ii#comment38Wed, 18 Jul 2012 07:25:55 GMTOskar, If it has a where clause, we can't use it as an index for dynamic queries. The code is good there.http://ayende.com/156993/unprofessional-code-take-ii#comment37http://ayende.com/156993/unprofessional-code-take-ii#comment37Wed, 18 Jul 2012 07:01:09 GMTJust one unrelated question: Why is it if (a.HasWhereClause) ? Or is it just the renderer that have removed the "!"?http://ayende.com/156993/unprofessional-code-take-ii#comment36http://ayende.com/156993/unprofessional-code-take-ii#comment36Wed, 18 Jul 2012 06:29:47 GMTI 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. :) http://ayende.com/156993/unprofessional-code-take-ii#comment35http://ayende.com/156993/unprofessional-code-take-ii#comment35Mon, 16 Jul 2012 09:32:51 GMTI feel it is missing some logging/tracing http://ayende.com/156993/unprofessional-code-take-ii#comment34http://ayende.com/156993/unprofessional-code-take-ii#comment34Mon, 16 Jul 2012 03:48:09 GMTand Rule should be something with business meaning instead of a summary of the filter implementationhttp://ayende.com/156993/unprofessional-code-take-ii#comment33http://ayende.com/156993/unprofessional-code-take-ii#comment33Sun, 15 Jul 2012 08:16:09 GMTIn 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 maintainabilityhttp://ayende.com/156993/unprofessional-code-take-ii#comment32http://ayende.com/156993/unprofessional-code-take-ii#comment32Sun, 15 Jul 2012 08:07:58 GMTNadav, There is a difference between BAD code and unprofessional code.http://ayende.com/156993/unprofessional-code-take-ii#comment31http://ayende.com/156993/unprofessional-code-take-ii#comment31Sun, 15 Jul 2012 07:01:31 GMTI 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) { } http://ayende.com/156993/unprofessional-code-take-ii#comment30http://ayende.com/156993/unprofessional-code-take-ii#comment30Sat, 14 Jul 2012 21:01:40 GMT@Ken Egozi, +1 The code screams for Refactor -> Extract Method.http://ayende.com/156993/unprofessional-code-take-ii#comment29http://ayende.com/156993/unprofessional-code-take-ii#comment29Sat, 14 Jul 2012 01:49:58 GMTYou 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.http://ayende.com/156993/unprofessional-code-take-ii#comment28http://ayende.com/156993/unprofessional-code-take-ii#comment28Fri, 13 Jul 2012 23:41:03 GMTAgree 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.http://ayende.com/156993/unprofessional-code-take-ii#comment27http://ayende.com/156993/unprofessional-code-take-ii#comment27Fri, 13 Jul 2012 19:37:47 GMTObvious - the code is cut in the righthand side, so it would probably not compilehttp://ayende.com/156993/unprofessional-code-take-ii#comment26http://ayende.com/156993/unprofessional-code-take-ii#comment26Fri, 13 Jul 2012 17:23:05 GMTLogic 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 http://ayende.com/156993/unprofessional-code-take-ii#comment25http://ayende.com/156993/unprofessional-code-take-ii#comment25Fri, 13 Jul 2012 17:18:11 GMTIt 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. http://ayende.com/156993/unprofessional-code-take-ii#comment24http://ayende.com/156993/unprofessional-code-take-ii#comment24Fri, 13 Jul 2012 17:02:59 GMTSorry mate. You just didn't abstract your database underneath enough layers. Totally fucking shoddy.http://ayende.com/156993/unprofessional-code-take-ii#comment23http://ayende.com/156993/unprofessional-code-take-ii#comment23Fri, 13 Jul 2012 15:53:20 GMT@Petr I agree, it should have been text so that we could copy-paste it ;-)http://ayende.com/156993/unprofessional-code-take-ii#comment22http://ayende.com/156993/unprofessional-code-take-ii#comment22Fri, 13 Jul 2012 13:34:33 GMTIn 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.http://ayende.com/156993/unprofessional-code-take-ii#comment21http://ayende.com/156993/unprofessional-code-take-ii#comment21Fri, 13 Jul 2012 13:23:22 GMTSix line of comments ! http://ayende.com/156993/unprofessional-code-take-ii#comment20http://ayende.com/156993/unprofessional-code-take-ii#comment20Fri, 13 Jul 2012 12:50:27 GMTdacanetdev & victor, Unfortunately the first line inside the Where(..) statement is obscured, it reads: var abstractViewGenerator = database.IndexDefinitionStorage.GetViewGenerator(indexName); So indexName does get used ;-)http://ayende.com/156993/unprofessional-code-take-ii#comment19http://ayende.com/156993/unprofessional-code-take-ii#comment19Fri, 13 Jul 2012 12:16:13 GMTFeature envy on abstractViewGenerator, use Tell don't Askhttp://ayende.com/156993/unprofessional-code-take-ii#comment18http://ayende.com/156993/unprofessional-code-take-ii#comment18Fri, 13 Jul 2012 12:12:31 GMTToo much green writing. I'd extract methods with descriptive nameshttp://ayende.com/156993/unprofessional-code-take-ii#comment17http://ayende.com/156993/unprofessional-code-take-ii#comment17Fri, 13 Jul 2012 11:54:10 GMTWhere are you using indexName inside the lambda expression?http://ayende.com/156993/unprofessional-code-take-ii#comment16http://ayende.com/156993/unprofessional-code-take-ii#comment16Fri, 13 Jul 2012 11:49:23 GMTNo logging?http://ayende.com/156993/unprofessional-code-take-ii#comment15http://ayende.com/156993/unprofessional-code-take-ii#comment15Fri, 13 Jul 2012 11:39:37 GMTI agree with Nieve. The whole thing looks like it only works with abstract view generator and should be refactored as it's method.http://ayende.com/156993/unprofessional-code-take-ii#comment14http://ayende.com/156993/unprofessional-code-take-ii#comment14Fri, 13 Jul 2012 10:53:31 GMTIn the first place, posting code as a screenshot is what I call unprofessional..http://ayende.com/156993/unprofessional-code-take-ii#comment13http://ayende.com/156993/unprofessional-code-take-ii#comment13Fri, 13 Jul 2012 10:43:43 GMTTomas, Why? It will still have the line numberhttp://ayende.com/156993/unprofessional-code-take-ii#comment12http://ayende.com/156993/unprofessional-code-take-ii#comment12Fri, 13 Jul 2012 10:33:33 GMTit seems that you check every property of abstractViewGenerator. http://ayende.com/156993/unprofessional-code-take-ii#comment11http://ayende.com/156993/unprofessional-code-take-ii#comment11Fri, 13 Jul 2012 10:11:26 GMTI 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...http://ayende.com/156993/unprofessional-code-take-ii#comment10http://ayende.com/156993/unprofessional-code-take-ii#comment10Fri, 13 Jul 2012 10:04:17 GMTIMHO this logic should be in a view generator.http://ayende.com/156993/unprofessional-code-take-ii#comment9http://ayende.com/156993/unprofessional-code-take-ii#comment9Fri, 13 Jul 2012 10:02:02 GMT