<div class="gmail_quote">On Sun, Mar 25, 2012 at 12:05 AM, Rouslan Placella <span dir="ltr"><<a href="mailto:rouslan@placella.com" target="_blank">rouslan@placella.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div><div>
<br>
</div></div>Challenge accepted :D<br>
<br>
------------------------>%------------------------<br>
$field_charset = '';<br>
if (<br>
    $extracted_fieldspec['binary'] == false<br>
    &&<br>
    preg_match("@^(char|varchar|text|tinytext|mediumtext|longtext|set|enum)", $field_type)<br>
<div>) {<br>
    if (strpos($type, ' character set ')) {<br>
        $type = substr($type, 0, strpos($type, ' character set '));<br>
    }<br>
</div>    if (! empty($row['Collation'])) {<br>
        $field_charset = $row['Collation'];<br>
    }<br>
}<br>
<br>
$displayed_type = $type;<br>
<div>if (strlen($type) > $GLOBALS['cfg']['LimitChars']) {<br>
</div>    $displayed_type  = "<abbr title='$type'>";<br>
    $displayed_type .= substr($type, 0, $GLOBALS['cfg']['LimitChars']);<br>
    $displayed_type .= "</abbr>";<br>
}<br>
------------------------>%------------------------<br>
<br>
Now let's discuss my solution:<br>
<br>
* The $type variable is used in tbl_structure.php below the code snippet for two purposes: display<br>
of the column type in the table and to make comparisons such as "if ($type == 'text'...". Of course,<br>
if someone (crazy) sets $cfg['LimitChars'] to a value of less than 4 none of those comparisons will<br>
ever be true. So it's better to have two variables, $type for comparison and $displayed_type for<br>
display purposes, just like is done with $field_name and $displayed_field_name in the nearby code.<br>
This, of course, gets rid of the necessity for those magic numbers for the string offset. Also, we<br>
will avoid the possibility of accidentally cutting out end tag for ABBR when we cut out the<br>
character set from $type.<br>
<br>
* Now that we have two variables for the type of the field, it's better to generate the "displayed"<br>
one at the bottom of the code snippet, after we have finished "fiddling" with $type.<br>
<br>
* That awful lot of if statements is replaced by a neat regex.<br>
<br>
* The comparison of $extracted_fieldspec is moved to the first position in the condition of the if<br>
statement. This is because if the assertion evaluates to false, we won't have to run the more<br>
expensive regex.<br>
<br>
* Instead of unsetting $field_charset, we just set it to an empty string. This gets rid of a few<br>
lines of code further down.<br>
<br>
On a side note, I went digging into the need for cutting out the "character set" from the $type<br>
variable and to my surprise I cannot reproduce a single situation where the character set is part of<br>
the $type string. I've tested about 50 different tables, on several versions of MySQL as far back as<br>
3.23 (whipped out in convenient sandboxes) and a handful of PMA versions as far back as 2.5. The<br>
original code that stripped out the character set was added by Alexander M. Turek 9 years ago and<br>
labelled "Nicer output for MySQL 4.1". I would dig deeper to find out if that code is dead, in which<br>
case the first three if statements could be reduced down to just one, but that's way out of scope<br>
for this.<br>
<br>
Feedback welcome :)<br>
<br>
Bye,<br>
Rouslan</blockquote></div><div><br></div>Hi,<div><br><div>Seems that I interpreted refactoring more as coding style than actual </div><div>refactoring. Rouslan, your solution seems elegant. I have a few small </div><div>
questions: </div><div><br></div><div>1. Is there a certain reason you chose to use</div>
<div>"$extracted_fieldspec['binary'] == false"</div><div>instead of</div><div>" ! $extracted_fieldspec['binary']" ?</div><div>2. By $field_type you were probably referring to $type ?</div>
<div>3. In preg_match, after "enum)", you probably forgot an ending '@' ?</div><div><br></div><div>These are pretty meaningless, but that's my all feedback right now :)</div><div><br></div><div>--</div>
<div>Alex</div>
</div>