Bad code readability

It surprises me how people can write the sort of code I've seen recently without finding the obvious way to improve readability and usability. Here's one simple example.

        $sSql = "INSERT INTO article (" .
            "article_id, site_id, article_code, name, publication_id, " .
            "rating_id, status_id, section_id, template_id, headline1, " .
            "headline2, headline3, live, modified_date, original_date, " .
            "flash, expiry_date, embargo_date, embargo_hour, " .
            "embargo_day, message, section_front, front_page, " .
            "author_id, is_urgent, live_date, page_number, is_free, " .
            "master_article, edition, source_id, newspapersection_id" .
            ") VALUES (" .
            $this->iArticleId .
            ", " . $this->oSite->getSiteId() .
            ", " . addQuotes($this->sArticleCode) .
            ", " . addQuotes($this->sName) .
        ...
            ", " . nullOrId($this->oNewspaperSection) .
            ")";

Actually, this was a lot worse before I did my first pass, if you believe that. Anyway, first pass involved writing things to simplify the right-hand side of the equation - helpers to ensure a value is an integer, quote strings properly, or convert to MySQL boolean enumerations. This second pass changes this from one big string to using the natural structure for this - a dictionary.

Now, it looks more like:

        $createMap = array(
            'article_id' => $this->iArticleId,
            'site_id' => $this->oSite->getSiteId(),
            'article_code' => addQuotes($this->sArticleCode),
            'rating_id' => $this->oRating->getRatingId(),
            'status_id' => $this->oStatus->getStatusId(),
            'section_id' => $this->oSection->getSectionId(),
            'headline1' => addQuotes($this->sHeadLine),
            'headline2' => addQuotes($this->sHeadLine2),
            'headline3' => addQuotes($this->sHeadLine3),
            'live' => boolToSql($this->bIsLive),
            'flash' => boolToSql($this->bFlash),
            'expiry_date' => nullOrDateString($this->dExpiryDate),
            'embargo_date' => nullOrDateString($this->dEmbargoDate),
            'embargo_hour' => nullOrInteger($this->iEmbargoHour),
            'front_page' => boolToSql($this->bIsFrontPage),
            'source_id' => nullOrId($this->oSource),
        );
        $sSql = QueryUtilPackage::mapToCreateSql('article', $createMap);
        $oResult =& DBUtil::runQuery($sSql);

The simple magic is in mapToCreateSql:

   function mapToCreateSql ($table, $createMap) {
        $sSql  = 'INSERT INTO ' . $table . ' ( ';
        $sSql .= join(', ', array_keys($createMap));
        $sSql .= ' ) VALUES ( ';
        $sSql .= join(', ', array_values($createMap));
        $sSql .= ' )';
        return $sSql;
    }

For simple cases, UPDATE statements can get the same treatment:

    function mapToUpdateSql ($table, $updateMap, $idField, $id) {
        $updateFields = array_keys($updateMap);
        $updateValues = array_values($updateMap);

        $sSql  = 'UPDATE ' . $table . ' SET ';
        while (list($i, $v) = each($updateFields)) {
            if ($i > 0) {
                $sSql .= ', ';
            }
            $sSql .= $v . ' = ' . $updateValues[$i];
        }
        $sSql .= ' WHERE ' . $idField . ' = ' . $id;
        return $sSql;
    }

Now you just mention the identity field and the identity value, and your UPDATE statement is generated.

My Object-Relational Mapping (ORM) system at work uses these internally (well, that's what I wrote it for), but porting legacy classes to it is unfortunately non-trivial in a few cases, such as this one.

If you're still reading, here's what I meant about the right-hand side before:

    .", ".addQuotes( ( $this->bFlash ? "Y" : "N" ) )
    .", ".addQuotes( ( is_null( $this->dEmbargoDate ) ? null : $this->dEmbargoDate->getDateString( ) ) )
    .", ".( is_null( $this->iEmbargoDay ) ? " NULL " : $this->iEmbargoDay )

The new right-hand side looks like:

    boolToSql($this->bFlash),
    nullOrDateString($this->dEmbargoDate),
    nullOrInteger($this->iEmbargoDay),

You'll note this fixes a bug in the old version - it puts the null value (becoming an empty string) into the statement instead of the string "NULL", as is necessary in SQL. This inconsistency is another reason to simplify these sorts of things. I can't quite figure why noone did it before.