[Phpmyadmin-devel] Refactoring challenge

Rouslan Placella rouslan at placella.com
Sun Mar 25 21:33:32 CEST 2012


On 25/03/12 09:26, Rouslan Placella wrote:
> On 25/03/12 00:53, Alex Marin wrote:
>> 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']" ?
> Just aesthetics, really, I thought that it read better that
> way in this particular circumstance.
> 
>> 2. By $field_type you were probably referring to $type ?
> Yes, that was a typo.
> 
>> 3. In preg_match, after "enum)", you probably forgot an ending '@' ?
> Also a typo. I tinkered with the code after running it for the last time :S
> 
>> These are pretty meaningless, but that's my all feedback right now :)

Just adding an updated version of the code snippet with fixes for things that Alex found wrong with it:

------------------------>%------------------------
$field_charset = '';
if (
   $extracted_fieldspec['binary'] == false
   &&
   preg_match("@^(char|varchar|text|tinytext|mediumtext|longtext|set|enum)@", $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>";
}
------------------------>%------------------------

Bye,
Rouslan




More information about the Developers mailing list