in reply to Previous Line Matching Issues
Well, if it works, then so be it, but it would certainly flunk a code-review from me, and if you were on my team I would instruct you to rewrite it. Let me explain why.
I have certain pre-conceived notions about how such an algorithm “should” be implemented, such that I am unnecessarily thrown-off by strategies which depart from these norms. Specifically:
And please, don’t take this assessment personally. It’s not about you. This is engineering.
In any backward-looking loop like this one, the issue at hand is always how to correctly maintain the past-history, no matter what pathway is taken through the code. The next if idioms at the top of the loop are red-flags to me: you are not updating the history before you do next. There are other red-flags that jump out at me ... and basically, I don’t want code in the source-base that has any red-flags that jump out at me. I want to know that it is correct having proven that it is correct such that I never have to look at it again.
In closing, no, I did not exhaustively review the code as I would. Instead, I saw enough of it to realize that it was not obvious, therefore it could not be obviously-correct, therefore it could be a future source of problems. And so, that’s why I would instruct you to rewrite it and of course pay you to do so.
Replies are listed 'Best First'. | |
---|---|
Re^2: Previous Line Matching Issues
by ImJustAFriend (Scribe) on May 22, 2014 at 11:35 UTC |