[Phpmyadmin-devel] Refactoring challenge

[Alex Marin]

On Sun, Mar 25, 2012 at 12:05 AM, Rouslan Placella <rouslan at placella.com>wrote:
>
>
> 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


Hi,

Seems that I interpreted refactoring more as coding style than actual
refactoring. Rouslan, your solution seems elegant. I have a few small
questions:

1. Is there a certain reason you chose to use
"$extracted_fieldspec['binary'] == false"
instead of
" ! $extracted_fieldspec['binary']" ?
2. By $field_type you were probably referring to $type ?
3. In preg_match, after "enum)", you probably forgot an ending '@' ?

These are pretty meaningless, but that's my all feedback right now :)

--
Alex
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.phpmyadmin.net/pipermail/developers/attachments/20120325/a20c133d/attachment.html>