On Monday 21 Sep 2009 01:30:33 Steve Bertrand wrote: > Hi everyone, > > On my ISP project, I've done a personal 'code freeze'. My objective is > to go through every sub and ensure tests and POD are accurate. Where > they are not, I'll (re)write them. > > During this procedure, I noticed that many methods did not have decent > return values... so I added them. > > I know it's much to ask, but I'm asking for review on my 'style'. I want > to ensure that what I do is understandable to most. > > If you have time to do a quick browse, I'm looking to know: > > - does the POD make sense > - does my coding writing style look ok (I still haven't found my style yet) > - if my tests are ok > > With that said, I'd like to say: > > - forgive the dirty way that I munge the sym table. I'll be changing this > - my tests were written hastily. I'm finding it hard to put tests into a > test file and not into a 'throwaway' file. > > I do want feedback/advice. I especially want advice on writing tests. > > Please be harsh. Although it's sometimes hard to swallow, I learn best > when comments are direct, accurate, and truthful (thanks Uri). >
On http://acct.eagle.ca:8020/ISP/Error : <<<<<<<<<< unless defined $error { $error = ISP::Error->new; $error->bad_api(); } >>>>>>>>>>> As far as I know, you cannot use unless without the parenthesis this way. You need: <<<<<<<<< unless (defined $error) { } >>>>>>>>> Furthermore, I should not that using "unless" instead of "if (!" or "if (not" is less natural in many human languages. Whenever I (as a Hebrew speaker) see an unless, I keep have to think what it is. Also see: http://www.shlomifish.org/humour/fortunes/shlomif.html#n-uple-negative <<<<<<<<< add_trace(BACK) add_message(MESSAGE) Etc. >>>>>>>>>> I prefer to write something like: <<<<<<<<<< add_trace($back) add_message($message) >>>>>>>>>> Possibly with a better Perl 5 data structure type. You also seem to describe what the function does internally rather than why and how someone should use it. For example: <<<<<<<<<<<<<< Pushes onto @{ $error->{ data }} the data that triggered the error process. This method acts as both a setter and a getter. >>>>>>>>>>>>>> I don't care what how it does it. I want to know why should I call it and how I should use it. Finally, perhaps your synopsis should be longer and more meaningful. I hope I don't come up as too harsh. Regards, Shlomi Fish > pm http://ipv6canada.com/Error.pm > tests http://ipv6canada.com/Error.t > pod http://acct.eagle.ca:8020/ISP/Error > > Steve > > ps. ISP::Error was first up, due to alphabetical order. Feedback on this > module will pave my way for future progress. > > pps. Thanks. If you've read this far, you've probably done review. I am > ready to be broken, and brought back up. > -- ----------------------------------------------------------------- Shlomi Fish http://www.shlomifish.org/ Funny Anti-Terrorism Story - http://shlom.in/enemy Chuck Norris read the entire English Wikipedia in 24 hours. Twice. -- To unsubscribe, e-mail: beginners-unsubscr...@perl.org For additional commands, e-mail: beginners-h...@perl.org http://learn.perl.org/
