Ayende @ Rahien

bob commented on Challenge: This code should never hit production

Sun, 26 Dec 2010 22:29:43 GMT

I still come here because it's like tdwtf but a special Israeli edition .

Ayende Rahien commented on Challenge: This code should never hit production

Sat, 25 Dec 2010 14:34:50 GMT

Evereq, 1) You are not supposed to be able to understand it at first reading. There is a lot of missing context that you don't have available. 2) If it is an exception anyway, I wouldn't bother. This is deeply internal code, the code above it should make any required checks. 3) barrier to prod is a design problem. Bugs are easily fixed, design issues, not so much About Next(), not in the provided scenario, no. When you search for an item, it gets to the first item.

evereq commented on Challenge: This code should never hit production

Fri, 24 Dec 2010 12:57:31 GMT

@Ayende: 1) When I speak about hacks, I just mean that according to comments here, most of developers can't get it after first reading! I.e. it's hard to be sure that code works without bugs... Sure it's not really 'hacks', but for many developers it looks that way. And it always better to keep your code easy to read / understand by others :) Reread comments to figure out what most of guys here think as "hacks" - every time they point that something wrong, while it was not, it's a Hack! :) At least I think so :) 2) Sure, agree... i.e. it would not corrupt state probably (especially if you fix bug with TermEnum that can lead to leak of resources), but still it was possible to simplify future factoring if you add more static / dynamic checks and use design by contract or at least check more in pre-conditions! 3) 'bugs are not barrier for prod." :D It's depends how significant bugs and who is your customer :) ! P.S. Are you sure that really Next() should not be called before Term()??? I remember that, maybe from Java, but it was here :)

tytusse commented on Challenge: This code should never hit production

Fri, 24 Dec 2010 10:01:11 GMT

Not knowing the API I would guess: database.IndexStorage.GetCurrentIndexSearcher(index) might return null (I can imagine, that index with given name might not exist), which will result in NullPointerException in using() clause?

Ayende Rahien commented on Challenge: This code should never hit production

Fri, 24 Dec 2010 04:35:03 GMT

Steve, The code actually reads a lower-case only data.

Ayende Rahien commented on Challenge: This code should never hit production

Fri, 24 Dec 2010 04:34:09 GMT

James, Sorry, I didn't get that email, can you resend?

Ayende Rahien commented on Challenge: This code should never hit production

Fri, 24 Dec 2010 04:33:44 GMT

Evereq, 1) Sorry, but there are no hacks here. 2) The code would fail under certain condition. It would throw an exception. But it would not corrupt state, and the only action I could take would be to throw an exception myself. 3) There was one bug that I saw and fixed, but bugs are not barrier for prod.

Brad commented on Challenge: This code should never hit production

Fri, 24 Dec 2010 00:02:40 GMT

Actually Luke, I agree with NC. That code is pretty horrible.

Nadav commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 23:49:01 GMT

don't know if its the problem you're talking about, but it seems that the while statement expects the temsEnum to be sorted by this: termEnum.Term().Field() and i guess the condition in the while statement is expected to go over the terms with the field given as parameter and then stop when reaching terms with .Field() different than the given field, because if it is sorted, you can stop here, or, of course, when reached the end of the enumeration... And if i'm correct, the problem might be when the terms with .Field() that equals to the given field are in the middle (at least not right at the beggining), it won't get into the while statement at all, and it should have iterated until it finds something that equals to the given term before reaching the while statement: while (!field.Equals(termEnum.Term().Field()) && termEnum.Next() ); this will skip the terms with different .Field()

Luke Schafer commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 23:22:21 GMT

Diego and Brad - for people making commercial ventures, his insights are great. For people who need a break at work, the code challenges are great. I read more than I ever did. NC - no, I think the problem here is you.

Steve Py commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 23:07:30 GMT

Well my glaring obvious point would be that the comparison is case-sensitive. Not knowing the requirements & expected behaviour of the app since I didn't write it, this is the most obvious thing that comes to mind that might be unexpected.

NC commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 22:45:53 GMT

That code should never go into production because it's rubbish. It doesn't read at all. It makes absolutely no sense. This is the first piece of code you've posted that's made me say 'WTF' out loud. Code fails.

Andrey Titov commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 18:00:25 GMT

I guess termEnum.Next() should be called before accessing termEnum.Term(). It likely it should works as IEnumerable's MoveNext() and Current, otherwise termEnum should guarantee there is always at least one element.

James Curran commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 16:31:34 GMT

Ayende... I did suggest a means of spicing up this blog. (BTW, did you get the last email on the topic; I sent it about 10 days ago...)

evereq commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 15:48:44 GMT

ha! The question was "This code should never have the chance to go to production, it is horribly broken in a rather subtle way, do you see it?" So if Jason right, we can't put such code into production? What if we know that all our documents are small and contain very small amount of terms (say max few thousands - such code WILL work with that amount!)? What about rule that such optimizations should be done when try are really required? etc! I.e. I am not so happy with that answer if it is correct one: it's _not_ horribly broken code because of his performance - it's more likely that code broken because of a) Hacks when it was possible to made task without hacks! b) Code not safe enough - absence of right pre-conditions, post-conditions, invariants, asserts.. i.e. no design by contract, its not "defensive" style of programming etc c) BUGS in code (like the one others and me point out) d) Code is 10s lines of code, but actually do same as Lucene method IndexReader.Terms(term) :D I.e. all that huge code with bugs was possible to replace with one single line without ANY hacks :D (Ok, if you need additional where you feel free to use LinQ here) So, really disappointed with "question / answer" pair! Perfect question, perfect answer (i.e. Jason do correct!), but just both did not feet each other IMHO :)

Ayende Rahien commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 15:17:54 GMT

Jason, Yeah!

Oleksii commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 15:14:11 GMT

Hi Ayende, I think you get a NullReference exception at this line of code (as the searcher has never been initialized): var termEnum = searcher.GetIndexReader().Terms(new Term(field)); Oleksii

David Pendray commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 14:23:06 GMT

Diego, Brad, And yet this is one of the most well-commented .net blogs around... especially posts such as these... go figure

Brad White commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 14:07:18 GMT

Diego, I fully agree with you. I keep the blog in my RSS reader on the off chance that something interesting is posted. It use to be that all the posts were very relevant to software developers, and of general interest. Now it has become very commercial and when we are not being subtly marketed to we get "code challenges," which for the most part are out of context chunks of code, are just used as SEO fodder and filler between the marketing. Sigh. Another good blog bites the dust.

evereq commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 14:02:01 GMT

@Naiem etc: seems nothing wrong with while (field.Equals(termEnum.Term().Field())) because previous line (i.e. var termEnum = searcher.GetIndexReader().Terms(new Term(field));) extracts all terms STARTING at a given term (in our case starting with a empty term but in given field). But agree - code looks more like a hack, than as normal one :D

Jason Meckley commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 14:01:40 GMT

There is no limit to the size of the result. performance would suffer if the result returned 1000s or millions of items.

Felix commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 13:56:43 GMT

termEnum.Term() probably evaluate to null if there is no term...

Kevin Fairclough commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 13:46:39 GMT

Are duplicates based on case important/required?

Naiem commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 13:29:43 GMT

I don't know what the API does but the whole while condition looks wierd to me. while (field.Equals(termEnum.Term().Field())) If the searcher returns terms that relate to the given field, then what is the purpose of this condition. If it doesn't and Term() has other fields, then the loop can break anytime, since I don't think searcher returns sorted results. However, the rest of the code is too simple to be terribly broken!

Tom commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 13:21:57 GMT

The resultlist is compared item by item to parameter called field. When it does not match the function returns. So it does not necessarily go through the whole resultset and therefore might return an incomplete result.

evereq commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 13:20:50 GMT

@Ayende and Dalibor! Any code like that contains a lot of issues (actually ANY code contain issues :D, ALL the code!), so it's just interesting to try to catch some of them! Most of issues can be founded even without deep knowledge of context or even C# etc! :) So Ayende +1 to post such questions! I found it interesting to at least see how other people think, what direction they go etc :) Regarding question: maybe it's HashSet? I.e. it's cannot contain duplicates? ;-) P.S. about 3) from my comment about: it's very questionable question what is terms! In Lucene term is OBJECT (or better to say class), i.e. it's not only text, but it's also some other data, at least 'field' in current Lucene version! So because it's just made sense to continue with "object thinking", I am sure it's better to return Terms collection (terms objects) than just strings from method with name "GetTerms" - i.e. behavior should be expressed in domain terms, not on primitive data types. And Lucene / storage domain consists from "Terms", not just strings :) So sure it's completely valid to return strings, but it have a "smell" for me :)

Ryan Heath commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 13:18:36 GMT

Ok, I'll give it another shot: Should you return IEnumerable

Ayende Rahien commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 13:14:41 GMT

Diego, I am sorry to hear that, but you are of course free to stop reading me

Diego commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 13:08:35 GMT

Hi. I've been reading your blog from a long time, and I must say that your blog became really boring. All you do is: 1) publish your commercial products (Uber Prof, the key-value DB, etc) and 2) paste code fragments as challenges. I still know you're a really good programmer, it's just your blog isn't good anymore (it's my opinion of course). Diego

Ayende Rahien commented on Challenge: This code should never hit production

Thu, 23 Dec 2010 13:00:35 GMT

Dalibor, Yep, that is the case. I consider the problem glaringly obvious, and no one sees it