Slash Boxes
NOTE: use Perl; is on undef hiatus. You can read content, but you can't post it. More info will be forthcoming forthcomingly.

All the Perl that's Practical to Extract and Report

The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.
More | Login | Reply
Loading... please wait.
  • What do you do if you see things like this in a script a co-worker wrote:

    Nothing at first. Do they work? Yes. Are they icky? Sure, but they also work. Are they wrong? No, not at all.

    Now, I'm not saying you ignore it, but be logical and methodical about it. "Seek first to understand, then to be understood." Try to understand why the code is written that way. For example:

        if ($dbh) {
            return 1;
        } else {
            return 0;
    It would be damn easy to change that to
    return defined $dbh;
    but what are the effects? What if the function relies on 1 or 0? It shouldn't, of course, but that doesn't mean you can go changing the code willy-nilly. If someone's going to return 1 or 0 for true or false, chances are he'll check those values explicitly, too.

    Also, when you examine why things are, look beyond the obvious. For example:

    $searchsql = sprintf("%s%s",$searchsql,$sql);
    Sure, that should be
    $searchsql .= $sql;
    or more likely (I'm guessing)
    $searchsql .= $where_clauses;
    but why stop there? Assuming that the 2nd $sql is conditions on the SQL, why is he building SQL by hand? Why isn't he using bind variables? Maybe that's the real improvement to be made.

    And before you go changing this stuff, do you have tests in place to catch the errors you're going to introduce? There's no such thing as a simple one line change. Don't fool yourself into thinking "I'll just [] change one line". All it takes is to type

    $searchsql = $sql;
    instead of
    $searchsql .= $sql;
    to make a simple, one-line change that didn't need to actually be made into a huge blunder on your part that breaks code.

    What about the human side of things? You say "I've tried to subtly suggest to him and my boss that I could help him with his programming. So far I have been ignored."

    First, don't be subtle. If it needs to be changed, then it needs to be changed, and it's your responsibility to make it clear. And if it doesn't actually need to be changed, then don't bother suggesting it.

    Second, explain what the benefits are. Explain to your boss the business reasons for helping your co-worker with his code. Does the current codebase hinder maintainability? Is it more prone to errors? Or is it just that you don't like it? If you can't come of with solid reasons to take a course of action, and you can't explain them clearly, then don't fight the fight.