[Phpmyadmin-devel] Refactoring challenge

[Marc Delisle]

Le 2012-03-25 15:33, Rouslan Placella a écrit :
> 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

Hi,
thanks for participating in the challenge. Let me share my views (some
of these had already been found by Rouslan)

1. In the original code, I did not like that $type was possibly modified
to generate an abbreviated version, and then there was code to
compensate for this alteration. It's a good idea to use a distinct
variable to hold the displayable type.

2. Using a regexp is a good idea.

3. I agree that checking for "character set" string is dead code.

4. Look at $extracted_fieldspec; this is an array coming from
PMA_extractFieldSpec(). I suggest to move into this function the
equivalent of

if ($extracted_fieldspec['binary'] == false
 &&
preg_match("@^(char|varchar|text|tinytext|mediumtext|longtext|set|enum)@",
$type)

so that the function can return a new array element,
'can_contain_collation'.

5. The same function could return a new array element
'abbreviated_type', with the <abbr> stuff, and which would be used at
display time.

6. This function should be renamed PMA_extractColumnSpec() because now
we use the SQL terminology of a 'column' and not a 'field'.

If there is general agreement, I can prepare a patch to make this clearer.

-- 
Marc Delisle
http://infomarc.info

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 898 bytes
Desc: OpenPGP digital signature
URL: <http://lists.phpmyadmin.net/pipermail/developers/attachments/20120326/0322f04c/attachment.sig>