Changing Modules in Bad Code

[Kozz]

Most esteemed monks:

I have been asked to evaluate some CGI code for revisions. The code in question is written *horribly*, without "use strict" in most of them, no modularization or custom packages for easy configuration, lots of HTML inside the CGI scripts.

It also uses the somewhat antiquated Postgres module (which I dislike). However, I've been asked to update these scripts to use the DBI module (which I am familiar with -- I think it works with Postgres?).

Now I'm wrestling with two possible solutions, and would like your opinions.

I've been told that I should not go to any heroic efforts to make all the code "better", because so long as it is working, they don't want to spend the money rewriting it from scratch. Basically, make it use the new DBI module, keep it working, but not much else.

I thought about two different solutions:

1. The straight-forward editing of every single perl CGI script, replacing "use Postgres" with "use DBI", then fixing up all the code to use DBI methods for connecting, executing, fetching results, error handling
2. OR, the more challenging (but maybe easier, too??) of writing my own "Postgres" module that actually acts as a *wrapper* to the current DBI module

The first one could be done, but would be exceedingly tedious, and there would surely be a higher chance of missing some code somewhere, or messing up other things as I try to "fix" the code.

The second one sounds more intriguing, so that if I wrote wrapper methods that made all the proper DBI method calls, the "use Postgres" would then call *my* module instead, but everything would go through the DBI module.

The drawback of #2 is that I have never found the need / excuse to create my own package or module, ever. I don't (yet) know how to do it. I've looked at perltoot and perlmod (the latter of which is by no means a "how-to" document IMO).

I'm not sure what sort of problems I would be making for myself if I used solution #2. I would vastly appreciate all monks' advice offered.

[davorg]

the more challenging (but maybe easier, too??) of writing my own "Postgres" module that actually acts as a *wrapper* to the current DBI module

I don't think this is a good idea. To access a Postgres database using DBI, you'd need the DBD::Pg module. This converts standardised DBI calls to the Postgres API. I assume that the existing Postgres module looks a lot like the Postegres API (that's how most of these older database access modules work). Your suggested module would therefore convert Postgres API calls to DBI calls, only to have DBD::Pg turn them straight back again. That pretty much seems to defeat the object of using DBI to me.

--
<http://www.dave.org.uk>

"The first rule of Perl club is you do not talk about Perl club."
-- Chip Salzenberg

[hardburn]

Looking at it from an engineering point of view, you're right. But it's possible that development time is more important than making it look right, in which case modifying the existing Postgres module will probably take less time.

----
I wanted to explore how Perl's closures can be manipulated, and ended up creating an object system by accident.
-- Schemer

Note: All code is untested, unless otherwise stated

[Zaxo]

Option 1, hands down.

You'll find it's not as bad as you fear to translate, and you'll gain in portability of the code - only SQL dialect changes needed to port to another db if you haven't used any DBI methods that are not widely supported.

It would be a remarkable feat to roll your own tested Pg interface more cheaply than rewriting the application from scratch. I don't believe in it.

After Compline,
Zaxo

[dragonchild]

The second solution also has the benefit of not actually touching the existing codebase. But, as someone who's done this kind of work (GL -> OpenGL), it's not as easy as it sounds.

Problems you will encounter:

1. There isn't a 1-1 mapping between functions in the old and functions in the new.
2. You'll probably have to write a bunch of state maintenance.
3. Testing this will be absolutely imperative.
You might find it easier to take the existing Postgres module and rewriting its calls to use DBI. That way you know you have every possible call to Postgres covered and you don't have to worry about state maintenance and the like. Remember - all you have to change is the communication with the DB - the external stuff. You do not (and should not) have to change how Postgres works, internally. In fact, keeping it the same will improve your success potential.

------
We are the carpenters and bricklayers of the Information Age.

Don't go borrowing trouble. For programmers, this means Worry only about what you need to implement.

Please remember that I'm crufty and crochety. All opinions are purely mine and all code is untested, unless otherwise specified.

[l2kashe]

I'll also throw out a vote for option 1.

Option 2 could lead to *very* interesting situations on down the road. Say you do end up wrapping the module, or rewriting it's methods. Someone else comes along later, and upgrades the module. oops!

MMMMM... Chocolaty Perl Goodness.....

[Kozz]

Thank you all very much for your helpful advice and insight. I have once again been reassured of the invaluable experience upon which I can draw here at PerlMonks. Not having written a module or package, I must take heed the many warnings of stumbling blocks in solution #2.

I believe I will indeed try to recode the scripts using DBD::Pg as suggested.

To my helpful monks, I've deposited a ++ into your PM account. ;)

[sgifford]

I'm sure you already know this, but when you're looking to make sure you've stomped out every last occurence of use Postgres, find and grep are you friends....

[grantm]

Renaming Postgres.pm would probably be a good idea too :-)

[Jenda]

I think it would be better to do the 1. but to make it easy you might install Devel::TraceSubs and add something like:

use Devel::TraceSubs;
open my $_LOG, ">> $0-Postgres.log";
my $_dbg = Devel::TraceSubs->new(
    verbose => 0,
    pre => '',
    post => '<',
    level => '',
    params => 0,
    logger => sub {
      my ($package, $filename, $line, $subroutine, $hasargs,
          $wantarray, $evaltext, $is_require, $hints, $bitmask) = call
+er(1);
      return if $_[2] eq '<' or $filename =~ m{Postgres.pm$};
      print $_LOG "Called $_[5] in $filename at $line\n";
    },
);

$_dbg->trace(
    'Postgres::'
);

on top of the scripts and run them. It should create a report with all the places where you call the Postgres.pm's functions/methods. Of course you may need to run the scripts several times to make sure all branches are covered, but still this should help in preparing the list of places you need you change.

HTH, Jenda
Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live.
   -- Rick Osborne

Edit by castaway: Closed small tag in signature

[chunlou]

For maintainability, a third option, like a combination of your option 1 and 2, is to write a wrapper module (DB::MyDB or something) with methods behaving like the ones being used. That way, if you make backend changes, you modify the implementation of MyDB, not the scripts that use it, as long as the methods' input-output spec stays the same.