Beefy Boxes and Bandwidth Generously Provided by pair Networks
Problems? Is your data what you think it is?
 
PerlMonks  

Re: Code review on script site

by Jazz (Curate)
on Nov 27, 2001 at 01:34 UTC ( #127630=note: print w/ replies, xml ) Need Help??


in reply to Code review on script site

I've thought long and hard about this over the weekend, and agree with nearly all of the suggestions noted. I was also thinking of the flip side of the issue. (People who know me know that I always look on all sides before undertaking as large a project as this; some may call it procrastination, but I digress).

To date, no other perl archive-type site is planning a code review process. This means that everyone who comes to the site may think that the code listed on the site is crap and go to another site (same crap, but no claims one way or the other).

Not only may the viewership decrease, but methinks that an archive where 80% or more of the scripts are dubbed as "unworthy for use" may reflect poorly on the language it was written in. Perhaps even extending to the books or notables that recommend the site? Not to actual programmers mind you, but to the webmasters or beginners who are just looking for a little script that does x? Or to potential or newer programmers who will find the learning curve for "more acceptable programming skills" too steep? Whose first lessons in Perl was that "Baby Talk is okay"? Note that the current target audience of the guide is webmasters and newer programmers, not seasoned Perl programmers.

Ahhh, the joys of breaking new ground :)

Perhaps the solution would be to formulate guidelines that would make the review process a bit gentler on all parties, including the reviewers (/me braces herself for the reaction on that one). More in-depth code reviews can be solicited by each script's author for a fee (to be determined by an available reviewer or by consensus) or on an online community venue such as this one.

If I were a webmaster looking for a script, my primary concerns would be 1) cost (not a reviewable item), 2) will it work? 3) is it safe? 4) can I easily make it look the way I want it to? and 5) if something goes wrong, will I be able to understand and maybe fix the error?. I wouldn't necessarily be worried about mnemonic naming, scoping, or style.

tilly's outline addresses a lot of the concerns that I didn't originally recognize the importance of, so, let me try this again :)

What I've done here is reorganize many of the points noted above to categories. Each section will be granted equal weight and the overall score will be the aggregate of all categories. The only exception to the scoring is Security, which in addition to the basic score will be prominently red flagged if it fails.

Obviously, green = pass; red = fail.

  1. Information
    1. Documentation
      1. Accurate and comprehensive description
      2. Inaccurate, incomplete, or no documentation
    2. Installation Instructions
      1. Accurate instructions for defined platform(s)
      2. Explanation and description of dependencies (of non-standard modules)
      3. Inaccurate or no installation instructions
      4. No warning of dependencies
  2. Security
    1. Uses -T in conjunction with valid checks on all user input for potential security breaches or other damage
    2. Does not appear to allow arbitrary commands
    3. open( FILE, "$unchecked_user_input" )
    4. system `$unchecked_user_submitted_command` (or exec or eval)
    5. Error pages that include login or other sensitive information
    6. Bonus Points
      1. Using $CGI::POST_MAXor otherwise limiting maximum post size
  3. Customization
    1. Comments where necessary to clarify the actions being performed
    2. Comments that include customization instructions (either html or code behaviour)
    3. A templating system, such as HTML::Template, Mason, or even an html output section that the user can easily edit
    4. Extensive html/code integration for output
    5. Little or no useful comments
    6. Useless commenting (as noted here
  4. Error Management
    1. Error Trapping
      1. Uses die or other trapping where applicable
      2. Providing useful (and safe) error messages
      3. Not checking the return values of important system calls (particularly open)
      4. Not providing useful (or unsafe) error messages
    2. Error prevention
      1. Uses strict
      2. Uses warnings
      3. Doesn't use strict
      4. Doesn't use warnings
  5. Overall Style
    1. A semblance of consistent style
    2. At-a-glance adherers to perlstyle pass
    3. Margin huggers
    4. Inconsistent or no style
    5. Code Obfuscation (e.g. an attempt to make source code unreadable and/or unmodifiable)

Again, please allow me to assert that site is little more than an archive of scripts written in Perl. We want to provide basic code reviews in an effort to protect users and less-than-paranoid server admins from dangerous code (or code that does not meet even a minimal set of standards) as well as act as a starting point for more intensive Perl study on behalf of the code authors. The review must be comparatively basic in order for it to be done on that many scripts, particularly with a staff of volunteers. There will definitely be a comments section where reviewers can elaborate, if they wish.

In most cases, we do not have a (valid) email address for authors whose programs are listed on the site, so most will not even be aware that the code has been reviewed unless they stumble onto it themselves. It is probably safe to assume that the code reviews are being done for the users rather than the authors. In one respect this is ideal, because if a program written in Perl just trashed a server/database/etc., who would want to learn or use it (or continue to)?

Many thanks for all of your help, comments and suggestions :)
Jasmine


Comment on Re: Code review on script site

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://127630]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others contemplating the Monastery: (11)
As of 2014-07-25 07:42 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    My favorite superfluous repetitious redundant duplicative phrase is:









    Results (169 votes), past polls