Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl: the Markov chain saw
 
PerlMonks  

Re: Previous Line Matching Issues

by sundialsvc4 (Abbot)
on May 21, 2014 at 15:22 UTC ( #1086993=note: print w/replies, xml ) Need Help??


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:

  1. I expect there to be only one place where the next-line of the file is read.   Your version contains at least two.   I am therefore immediately suspicious that this logic will skip lines.
  2. Cryptic variable names such as $cl and $nl, in a substantial piece of code like this, are not acceptable.
  3. This logic looks ahead, where I expect to see state-machine like logic which remembers its past.
  4. I can anticipate that this business requirement might soon call for looking-back some arbitrary-n lines, and I do not readily see how this logic would be readily adapted to do this.
  5. Overall complexity.   This approach is harder than it needs to be, and I doubt that it is accompanied by a rigorous test-suite sufficient to prove that it operates correctly in each and in every edge-case that it might encounter in production.

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 (Beadle) on May 22, 2014 at 11:35 UTC

    sundialsvc4, thank you very much for your review. I don't take anything you said personally at all. I am always happy to get critique on my work, good or bad.

    In regards to your comments... I am not on a development team or producing enterprise level code in any way. Most of my scripts are used locally by my team in a very specific one-off scenario. In this case, we were using it to read the output of a tshark command that was always formatted the same way. Had this been something to use repeatedly, there would have been differences... among them fully commented code, sensible variable names (like $currentLine instead of $cl), etc.

    That said, I am also not a professional programmer, CS guy, or Perl expert - hence the original post looking for help. Coming into this script, I knew mostly *what* I wanted to do... but the *how* of moving around in a text file back and forth was new to me. So I Googled, read O'Reilly materials, and experimented until I got something that worked. What you don't see here is the 20 lines of "DEBUG: print..." I took back out! (( grin ))

    I would be very interested in how a programmer and Perl expert would tackle this. Could I ask you, if you have time, to re-do the algorithm in a "good" way? It would be purely for education...

    By the way, I think you're awesome for taking the time to look at my code and provide feedback!!

    Cheers!!

    ImJustAFriend

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://1086993]
help
Chatterbox?
and all is quiet...

How do I use this? | Other CB clients
Other Users?
Others cooling their heels in the Monastery: (3)
As of 2018-04-20 00:04 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?
    Notices?