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