[Phpmyadmin-devel] Refactoring challenge

[Rouslan Placella]

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 :)

Bye,
Rouslan

> 
> --
> Alex
>