On Sat, Mar 24, 2012 at 9:59 PM, Alex Marin alex.ukf@gmail.com wrote:

On Sat, Mar 24, 2012 at 6:20 PM, Alex Marin alex.ukf@gmail.com wrote:

...

Hello,

I find these refactoring guidelines useful for everyone, so I'll have a go:

$start = 0;
if ( strlen( $type ) > $GLOBALS['cfg']['LimitChars'] ) {
    $start = 13; // strlen( '<abbr title="' );
    $type = '<abbr title="' . $type . '">'
              . substr( $type, 0, $GLOBALS['cfg']['LimitChars'] )
               . '</abbr>';
}

unset( $field_charset );
$matches_type = ( substr($type, $start, 4)   == 'char'
                          || substr($type, $start, 7)   == 'varchar'
                          || substr($type, $start, 4)   == 'text'
                          || substr($type, $start, 8)   == 'tinytext'
                          || substr($type, $start, 10) == 'mediumtext'
                          || substr($type, $start, 8)   == 'longtext'
                          || substr($type, $start, 3)   == 'set'
                          || substr($type, $start, 4)   == 'enum'
 );
$field_charset = '';
if ( $matches_type && ! $extracted_fieldspec['binary'] ) {
    if ( strpos( $type, ' character set ' ) ) {
        $type = substr( $type, 0, strpos( $type, ' character set ' )

); } if ( ! empty( $row['Collation'] ) ) { $field_charset = $row['Collation']; } }

These are some refactoring modifications I would see fit, but I am sure there are others, so I would like to see different approaches.

-- Alex

Without the 3 unnecessary newlines of course ( that were inserted before my copy-pasted sections).

-- Alex

---

This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure _______________________________________________ Phpmyadmin-devel mailing list Phpmyadmin-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel

Hi Marc,

I have not learn yet the PHP code refactoring techniques. But I like to learn them. I tried to do some improvements on that code (tbl_structure.php : in current master, these start at line 263 of the script).

changes : * Create an array for store data types ($dataTypesArray) * Introduced a Boolean to check whether the string manipulating condition is true or false ($isTypeMatched) * Since there was a common pattern in conditions, create those condition check inside a loop using $isTypeMatched boolean * Use meaningful non-common names to new variables * Keep empty spaces between code snippets to increase the readability of the code

$start = 0; if (strlen($type) > $GLOBALS['cfg']['LimitChars']) { $start = 13; $type = '<abbr title="' . $type . '">' . substr($type, 0, $GLOBALS['cfg']['LimitChars']) . '</abbr>'; }

unset($field_charset); $dataTypesArray = array('char', 'varchar', 'text', 'tinytext', 'mediumtext', 'longtext', 'set', 'enum'); $isTypeMatched = false;

foreach ($dataTypesArray as $dataType) { if (substr($type, $start, strlen($dataType)) == $dataType) { $isTypeMatched = true; break; } }

if ($isTypeMatched && !$extracted_fieldspec['binary']) { if (strpos($type, ' character set ')) { $type = substr($type, 0, strpos($type, ' character set ')); } if (!empty($row['Collation'])) { $field_charset = $row['Collation']; } else { $field_charset = ''; } } else { $field_charset = ''; }

With this, though the number of data types need to increase, only needed change is adding new data types to $dataTypesArray. No need of adding more conditions. Does the above mentioned changes are make the code better ?

Regards !

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:

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

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

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