in reply to For Review: DB Import

Without actually reading the last line,

I would also be curious to hear how this might be tranformed into an OOP style

I thought that "hmmm, perhaps it would be good to investigate some and perhaps use OO instead." :)

I have no formal education either, but I still might be able to give some constructive and useful critique.

Anyway, back to the issue.

Initially I'd like to say that first I though "Oh no, another do-it-all-in-a-row." Of course things happen after each other, but that doesn't mean that the code has to be in the same order. I'd prefer to see some subroutines, even if they were just procedural. First of all, it enables encapsulation and reduces the noise so you can concentrate on what's happening right there. For example, the argument checking code. That would easily be abstracted to a subroutine. That would make it clearer what the program really does. (For maintainability.)

If you don't like that style, or that style seems inappropriate every now and then (I haven't looked that closely at the code), don't be afraid of adding extra blocks to create scoping tricks. This way you know where a variable isn't used anymore, and you're free to manipulate code without fearing far-fetched side-effects.

I've noticed that C programmers that program Perl often declare variables that contain "tmp" in them. And they're declared within a lexical scope that is much larger than the scope where the temporary variable is used. I use temporary variables too, of course, but I often add an extra block around them. Scoping is very important for maintainability, at least that's my belief. I've only had to maintain my own code, but that's enough. "Think about the guy that comes after you. After six month you'll be that guy." For instance, when I glanced through your code, I saw that @insert_fields was declared at the very top. Then I found code that uses that variable -- but that was 93 line below. So I started to look for other places where it was used that would justify the early declaration. (I failed.)

So basically my main objection to your code is that it's too accumulative. (Or at seems it looks that way.)

Except that, I think it's pretty decent code. And to modulize the code is probably not worth the effort, since it's an application, not a general thingy/programming interface.

And oh, I found a little piece of actual code to comment on. Yay! ;)
130: my $placeholders = join(',', map {'?'} @insert_fields); First I thought, "why use a block when a simple statement would do?". I.e. map '?', @insert_fields. Then I suddenly realized the misuse of map(). The x operator would be perfect here.
my $placeholders = join(',', ('?') x @insert_fields); Hope this was helpful.

Replies are listed 'Best First'.
Re: Re: For Review: DB Import
by impossiblerobot (Deacon) on Jan 25, 2002 at 22:34 UTC
    Thanks. I agree that most of your comments don't really apply to this particular application. What little exposure to formal Computer Science methodology was in Structured Programming, but I have trouble convincing myself to write subroutines that will only be used once in a specific order. :-)

    (Though I'm sure there is some room for increased modularity. Any specific suggestions?)

    You're absolutely right that I should have used an expression rather than a block in my 'map' statement, and I would never have thought of constructing my list using the 'x' operator (though I had originally tried to use it to construct the $placeholders string). I will definitely incorporate that change.

    Impossible Robot