in reply to Framework.pm 0.01a

People often wish that PerlMonks did more code reviews.

They want a special section for that.

I think that this post is an excellent example of why it doesn't happen. This is code that boo_radley clearly put work and thought into. He asked directly for feedback. While plenty duly voted him up, so far nobody was willing to put their neck on the line and put out the energy to give him any.

My opinion is that this happens because enough code to be useful for review is enough code to be more work than most people want to put out. Also when dealing with code that is meant to be good, and has nothing glaringly wrong with it, people don't generally feel comfortable criticizing.

Unfortunately I don't have energy to follow this up with a real dialog. But I will put down a baker's dozen of things that I noticed and think could be done differently. I sincerely hope that someone else follows up with a third opinion on some of these issues (or anything else you see)...

  1. Why the prototypes? As you know I am not a fan of them, but since you clearly intentionally using them, I am curious what led you to do so.
  2. I prefer seeing stuff in @EXPORT_OK rather than @EXPORT. (Just like Exporter recommends.) Particularly if (like get_var and set_var) the names are generic with no relation to the module at hand.
  3. You do a lot of passing by reference and passing of references. Compared to the work it takes to process arguments, the work to pass them is minor. I prefer to not worry about that.
  4. A related API note. You seem to do (see the get and set methods) a lot of modification of variables through references. To my eyes this is a disguised global. OK, it isn't called a global, but the same locality of reference issue exists.
  5. If you are going to access by reference, I prefer to see $foo->{bar} rather than $$foo{bar} because that way the code reads more naturally "left to right".
  6. In your 2 InsertPage* methods I count the same list of 10 items appearing no less than 4 times. That is repeated information that can and should be abstracted into an array and used internally. Otherwise it is a potential maintainance problem. (It would also be nice if one of those methods called the other behind the scenes.)
  7. Some of your arguments should be able to default to reasonable things. You have 10 variables, some of which you admit are useless...
  8. I don't like passing errors in a global variable that may or may not get checked. I would suggest making those errors be raised. If the user page wants to it can catch them with an eval block (which is efficient). At the top level you can trap that and do something reasonable.
  9. Similarly numerical return codes tend to be a lot harder to track down when things go wrong than nicely formatted string errors. At the best the string gives you a description of what went wrong. At worse the contents of the string are easy to grep for. This is (of course) in addition to the fact that it is easier to type 13 when you meant 14 than it is to type Permission denied when you meant Bad address. (Example error messages taken from what I get for system errors 13 and 14 on my machine.)
  10. Your load_lib routine will probably not work as you intend since confess is a fatal error. (In my example code that you borrowed, I intended that result. The only soft error was an undefined return value without $! or $@ being set.)
  11. I would wonder about loading code into package main. I think it would be better to require the code and let the user make sure that their libraries are valid modules, which are loaded into the package that they should be. Something like this:
    # Load a package sub load_lib { my $pkg = shift; my $file = $pkg; $file =~ s/::/\//g; $file .= ".pm"; require $file; }
    Note that this does not import. If the modules are OO, then that should not matter. Note that if the module was already loaded, then the above will avoid redoing work.
  12. I would be inclined to have a worse dump_struct method. Just use Data::Dumper and print out a plain text page. That may not be pretty, but it is easy to do, handles difficult issues with recursive references et al, and is likely to be useful in debugging.
  13. In your example code, I notice no locking logic. I would suspect race conditions where your simple database handling will cause data corruption. For a personal journal, regular backups are good enough. But it still is an issue to be aware of.

Replies are listed 'Best First'.
Re: Re (tilly) 1: Framework.pm 0.01a
by boo_radley (Parson) on May 06, 2001 at 21:42 UTC
    Tilly,
    I really do appreciate the time you took to provide these thoughts. For the moment, I will focus on the list you provided. If you have further interest in discussing what you mentioned in the preambled, I'd like that as well.

    1. Actually, I'm not so hot on prototypes myself. I misinterpreted some information in the cookbook to mean that they were required when creating a module... I asked about this in the CB one day, and have since learned that prototypes are not required for module creation. I have since dropped them.
    2. OK, I understand this, although I think I take it more as a criticism of my naming skills rather than methodology.
    3. Agreed. Although I do plan on removing the passing altogether, to allow the user to call get_* and set_* in order to remove the need shift in and set such variables. Alternately, I could just...
    4. Make these items globals and export them.
    5. Acknowledged, but largely as a issue of style. I find $$foo{bar} just as easy to read.
    6. Excellent idea. I'll put this in the todo list.
    7. Agreed, and I'll probably take errorhandler out of the structure; it was put in there to placate a coworker, and never was clear on how it would be used.
    8. I've never handled errors in this fashion, but it makes sense.
    9. I used numeric codes as a way to keep the templates clean, and because the idea is familiar to me. Additionally, I don't see support for regexes in tt2's directives, although I could replace the get_bit sub with something like match_string (string, regex_contents)..
      [% IF get_bit (error_level, 1) == 1 %] <FONT COLOR=RED> [% END %] versus
      [% IF match_string (error_string, "first name") == 1 %] <FONT COLOR=RE +D> [% END %]
      hmm...
    10. Yes, I had noticed this occuring when I had typos in modules so loaded.
    11. Noted. I really don't have anything to add, I'm not brushing you off.
    12. OK, I'll take a look at it, although I'll probably try to massage it back into an HTML table of some sort. :)
    13. This is true, too. I expected some flak on this since I realized my DBI stuff was pretty light; I was concentrating on debugging framework rather than making sure the csv got locked appropriately.
    Again, thanks for taking the time to turn a critical eye to my submission. I plan on incorporating much of what you've suggested into the source code.