On 25/03/12 00:53, Alex Marin wrote:
On Sun, Mar 25, 2012 at 12:05 AM, Rouslan Placella rouslan@placella.comwrote:
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:
- 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.
- By $field_type you were probably referring to $type ?
Yes, that was a typo.
- 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