Re: [Phpmyadmin-devel] Refactoring challenge

On 24/03/12 13:24, Marc Delisle wrote:

Hi,

As refactoring ideas are numerous in the GSoC 2012 ideas list, I propose a challenge. The goal is to discuss on this list an existing code snippet from phpMyAdmin and to arrive at the best possible refactored version.

At the same time, potential students can show their refactoring skills or learn what refactoring is about.

Let's discuss these lines from tbl_structure.php (in current master, these start at line 263 of the script):

$start = 0;
if (strlen($type) > $GLOBALS['cfg']['LimitChars']) {
    $start = 13;
    $type = '<abbr title="' . $type . '">' . substr($type, 0,

$GLOBALS['cfg']['LimitChars']) . '</abbr>'; }

unset($field_charset);
if ((substr($type, $start, 4) == 'char'
    || substr($type, $start, 7) == 'varchar'
    || substr($type, $start, 4) == 'text'
    || substr($type, $start, 8) == 'tinytext'
    || substr($type, $start, 10) == 'mediumtext'
    || substr($type, $start, 8) == 'longtext'
    || substr($type, $start, 3) == 'set'
    || substr($type, $start, 4) == 'enum')
    && !$extracted_fieldspec['binary']
) {
    if (strpos($type, ' character set ')) {
        $type = substr($type, 0, strpos($type, ' character set '));
    }
    if (!empty($row['Collation'])) {
        $field_charset = $row['Collation'];
    } else {
        $field_charset = '';
    }
} else {
    $field_charset = '';
}

Challenge accepted :D

------------------------>%------------------------ $field_charset = ''; if ( $extracted_fieldspec['binary'] == false && preg_match("@^(char|varchar|text|tinytext|mediumtext|longtext|set|enum)", $field_type) ) { if (strpos($type, ' character set ')) { $type = substr($type, 0, strpos($type, ' character set ')); } if (! empty($row['Collation'])) { $field_charset = $row['Collation']; } }

$displayed_type = $type; if (strlen($type) > $GLOBALS['cfg']['LimitChars']) { $displayed_type = "<abbr title='$type'>"; $displayed_type .= substr($type, 0, $GLOBALS['cfg']['LimitChars']); $displayed_type .= "</abbr>"; } ------------------------>%------------------------

Now let's discuss my solution:

* The $type variable is used in tbl_structure.php below the code snippet for two purposes: display of the column type in the table and to make comparisons such as "if ($type == 'text'...". Of course, if someone (crazy) sets $cfg['LimitChars'] to a value of less than 4 none of those comparisons will ever be true. So it's better to have two variables, $type for comparison and $displayed_type for display purposes, just like is done with $field_name and $displayed_field_name in the nearby code. This, of course, gets rid of the necessity for those magic numbers for the string offset. Also, we will avoid the possibility of accidentally cutting out end tag for ABBR when we cut out the character set from $type.

* Now that we have two variables for the type of the field, it's better to generate the "displayed" one at the bottom of the code snippet, after we have finished "fiddling" with $type.

* That awful lot of if statements is replaced by a neat regex.

* The comparison of $extracted_fieldspec is moved to the first position in the condition of the if statement. This is because if the assertion evaluates to false, we won't have to run the more expensive regex.

* Instead of unsetting $field_charset, we just set it to an empty string. This gets rid of a few lines of code further down.

On a side note, I went digging into the need for cutting out the "character set" from the $type variable and to my surprise I cannot reproduce a single situation where the character set is part of the $type string. I've tested about 50 different tables, on several versions of MySQL as far back as 3.23 (whipped out in convenient sandboxes) and a handful of PMA versions as far back as 2.5. The original code that stripped out the character set was added by Alexander M. Turek 9 years ago and labelled "Nicer output for MySQL 4.1". I would dig deeper to find out if that code is dead, in which case the first three if statements could be reduced down to just one, but that's way out of scope for this.

Feedback welcome :)

Bye, Rouslan