Beefy Boxes and Bandwidth Generously Provided by pair Networks
No such thing as a small change
 
PerlMonks  

Re (tilly) 1: Code review on script site

by tilly (Archbishop)
on Nov 26, 2001 at 08:47 UTC ( [id://127488]=note: print w/replies, xml ) Need Help??


in reply to Code review on script site

I dislike this list.

It misses too many important things. And takes too many picky positions on irrelevant details. For instance take the HERE doc suggestion. Personally I don't use HERE docs because I find that synchronizing them against my own indentation tends to be too fragile. Instead I use qq(). Am I therefore going to be docked?

Other things are inappropriate for scripts. Using templating modules is great for websites, but for a script it adds a big dependency, forces you to break out and manage several files, and gives little useful return. As I like to say, every good thing you can do has a corresponding cost, and it is the job of good programmers to decide the tradeoffs. I remain to be convinced that adding that dependency is typically a good choice for a stand-alone script. And I am likewise unconvinced that adding the indirection of CGI HTML generation is good for code that you hope someone will learn from.

And of course there is the commenting question. Well good programmers disagree strongly on whether or not to comment, let alone how to comment appropriately. That is a mine-field I would recommend leaving well alone.

So what would a better list be? Well if I was trying to judge a stand-alone CGI script, my list of things to look at would go something like this:

  1. Let's start with the externals. Does it have documentation?
  2. Does the documentation accurately say what it does?
  3. Does it say how to install it? (Including what all dependencies are.)
  4. Does it say how to customize it? (Effective comments in code are sufficient customization instructions.)
  5. Does it work?
  6. Now we move to code quality. Does the code have a consistent and somewhat reasonable indentation style? (People may think this is minor, just run perltidy. I think that anyone who doesn't know to do the obvious basics is unlikely to know how to do anything else right.)
  7. Is their code broken up into managable chunks? A good rule of thumb is that any function or straight-line piece should be 50 lines or less unless there is an obvious reason for the length.
  8. Are the functions and variable clearly named? (Names like do_stuff fail this test.)
  9. Do the functions look like they do what the name says?
  10. Does the code use strict.pm?
  11. Are variables tightly scoped where possible?
  12. Is it made clear up front what all of the globals are?
  13. Is the list of globals used kept to a reasonable length?
  14. Is the return of important system calls (particularly open) consistently checked?
  15. Is -T used?
  16. Are the taint checks reasonably restrictive?
Note that I have left out file locking. Why? Because auditing them takes a lot of work, and most people who try to use file locking (including most potential auditors) consistently get it wrong. Before even considering looking at issues which are that difficult to analyze, I would use a checklist like the above to fail out 99% of the scripts. If you have never heard of modularizing your code or if you use globals exclusively, you have bigger problems than a few race conditions in a script that is likely to be under light load...
  • Comment on Re (tilly) 1: Code review on script site

Log In?
Username:
Password:

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

How do I use this?Last hourOther CB clients
Other Users?
Others having a coffee break in the Monastery: (10)
As of 2024-04-18 15:18 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found