[Phpmyadmin-devel] GSOC 2012 - Refactoring: Insert/edit, Privileges, Operations, Structure
Hi Marc, I did a feature request and uploaded the patch file [0] to Sourceforge. But it is relevant to Context. Therefor I think it will review after the deadline. So I like to much familiar with that Refactoring project. When I implementing above feature I went through the tbl_change.php file. I need to familiar with other files also which is relevant to above project. So if you have any idea, please let me know, I am grateful if I could get to know your suggestions.
[0] - https://sourceforge.net/tracker/?func=detail&atid=377410&aid=3507001...
Le 2012-03-17 10:30, Thilina Buddika Abeyrathna a écrit :
Hi Marc, I did a feature request and uploaded the patch file [0] to Sourceforge. But it is relevant to Context. Therefor I think it will review after the deadline. So I like to much familiar with that Refactoring project. When I implementing above feature I went through the tbl_change.php file. I need to familiar with other files also which is relevant to above project. So if you have any idea, please let me know, I am grateful if I could get to know your suggestions.
Hi Thilina, I have reviewed your patch.
About refactoring, I have an idea. In the master branch, tbl_structure.php, look at lines 199-209. Can you suggest some refactoring of these lines?
On Sat, Mar 17, 2012 at 8:38 PM, Marc Delisle marc@infomarc.info wrote:
Le 2012-03-17 10:30, Thilina Buddika Abeyrathna a écrit :
Hi Marc, I did a feature request and uploaded the patch file [0] to Sourceforge.
But
it is relevant to Context. Therefor I think it will review after the deadline. So I like to much familiar with that Refactoring project. When I implementing above feature I went through the tbl_change.php
file. I
need to familiar with other files also which is relevant to above
project.
So if you have any idea, please let me know, I am grateful if I could
get
to know your suggestions.
Hi Thilina, I have reviewed your patch.
About refactoring, I have an idea. In the master branch, tbl_structure.php, look at lines 199-209. Can you suggest some refactoring of these lines?
Hi,
Thanks Marc. I'll look at that file.
On Sat, Mar 17, 2012 at 9:30 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
On Sat, Mar 17, 2012 at 8:38 PM, Marc Delisle marc@infomarc.info wrote:
Le 2012-03-17 10:30, Thilina Buddika Abeyrathna a écrit :
Hi Marc, I did a feature request and uploaded the patch file [0] to Sourceforge.
But
it is relevant to Context. Therefor I think it will review after the deadline. So I like to much familiar with that Refactoring project. When I implementing above feature I went through the tbl_change.php
file. I
need to familiar with other files also which is relevant to above
project.
So if you have any idea, please let me know, I am grateful if I could
get
to know your suggestions.
Hi Thilina, I have reviewed your patch.
About refactoring, I have an idea. In the master branch, tbl_structure.php, look at lines 199-209. Can you suggest some refactoring of these lines?
Hi,
Thanks Marc. I'll look at that file.
Hi Marc,
Sorry for the delay to reply. I had a mid semester exam. So I look at the tbl_structure.php file and I have some suggestion for refactoring of line 199-209. First thing is it is better to have an underscore for id field <th id="th_<?php echo ++$i; ?>"></th>
Second one is, when using the if condition or for loop with html, it is better to use this style
<php? if(condition) : ?> <html> <html> <?php endif; ?>
Le 2012-03-22 21:57, Thilina Buddika Abeyrathna a écrit :
On Sat, Mar 17, 2012 at 9:30 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
On Sat, Mar 17, 2012 at 8:38 PM, Marc Delisle marc@infomarc.info wrote:
Le 2012-03-17 10:30, Thilina Buddika Abeyrathna a écrit :
Hi Marc, I did a feature request and uploaded the patch file [0] to Sourceforge.
But
it is relevant to Context. Therefor I think it will review after the deadline. So I like to much familiar with that Refactoring project. When I implementing above feature I went through the tbl_change.php
file. I
need to familiar with other files also which is relevant to above
project.
So if you have any idea, please let me know, I am grateful if I could
get
to know your suggestions.
Hi Thilina, I have reviewed your patch.
About refactoring, I have an idea. In the master branch, tbl_structure.php, look at lines 199-209. Can you suggest some refactoring of these lines?
Hi,
Thanks Marc. I'll look at that file.
Hi Marc,
Sorry for the delay to reply. I had a mid semester exam. So I look at the tbl_structure.php file and I have some suggestion for refactoring of line 199-209. First thing is it is better to have an underscore for id field <th id="th_<?php echo ++$i; ?>"></th>
Second one is, when using the if condition or for loop with html, it is better to use this style
<php? if(condition) : ?> <html> <html>
<?php endif; ?>
Thilina, In your suggestions, I don't see any refactoring techniques; yours are mostly about syntax. See [0] and [1] for references.
Also, I don't believe we use this "if" syntax in the current codebase.
[0] http://en.wikipedia.org/wiki/Refactoring [1] http://devzone.zend.com/1067/refactoring-php-code/
On Fri, Mar 23, 2012 at 5:26 PM, Marc Delisle marc@infomarc.info wrote:
Le 2012-03-22 21:57, Thilina Buddika Abeyrathna a écrit :
On Sat, Mar 17, 2012 at 9:30 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
On Sat, Mar 17, 2012 at 8:38 PM, Marc Delisle marc@infomarc.info
wrote:
Le 2012-03-17 10:30, Thilina Buddika Abeyrathna a écrit :
Hi Marc, I did a feature request and uploaded the patch file [0] to
Sourceforge.
But
it is relevant to Context. Therefor I think it will review after the deadline. So I like to much familiar with that Refactoring project. When I implementing above feature I went through the tbl_change.php
file. I
need to familiar with other files also which is relevant to above
project.
So if you have any idea, please let me know, I am grateful if I could
get
to know your suggestions.
Hi Thilina, I have reviewed your patch.
About refactoring, I have an idea. In the master branch, tbl_structure.php, look at lines 199-209. Can you suggest some refactoring of these lines?
Hi,
Thanks Marc. I'll look at that file.
Hi Marc,
Sorry for the delay to reply. I had a mid semester exam. So I look at the tbl_structure.php file and I have some suggestion for refactoring of line 199-209. First thing is it is better to have an underscore for id field <th id="th_<?php echo ++$i; ?>"></th>
Second one is, when using the if condition or for loop with html, it is better to use
this
style
<php? if(condition) : ?> <html> <html>
<?php endif; ?>
Thilina, In your suggestions, I don't see any refactoring techniques; yours are mostly about syntax. See [0] and [1] for references.
Also, I don't believe we use this "if" syntax in the current codebase.
[0] http://en.wikipedia.org/wiki/Refactoring [1] http://devzone.zend.com/1067/refactoring-php-code/
Thank you Marc. I will refer those articles.
On Fri, Mar 23, 2012 at 8:28 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
On Fri, Mar 23, 2012 at 5:26 PM, Marc Delisle marc@infomarc.info wrote:
Le 2012-03-22 21:57, Thilina Buddika Abeyrathna a écrit :
On Sat, Mar 17, 2012 at 9:30 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
On Sat, Mar 17, 2012 at 8:38 PM, Marc Delisle marc@infomarc.info
wrote:
Le 2012-03-17 10:30, Thilina Buddika Abeyrathna a écrit :
Hi Marc, I did a feature request and uploaded the patch file [0] to
Sourceforge.
But
it is relevant to Context. Therefor I think it will review after the deadline. So I like to much familiar with that Refactoring project. When I implementing above feature I went through the tbl_change.php
file. I
need to familiar with other files also which is relevant to above
project.
So if you have any idea, please let me know, I am grateful if I
could
get
to know your suggestions.
Hi Thilina, I have reviewed your patch.
About refactoring, I have an idea. In the master branch, tbl_structure.php, look at lines 199-209. Can you suggest some refactoring of these lines?
Hi,
Thanks Marc. I'll look at that file.
Hi Marc,
Sorry for the delay to reply. I had a mid semester exam. So I look at
the
tbl_structure.php file and I have some suggestion for refactoring of
line
199-209. First thing is it is better to have an underscore for id field <th id="th_<?php echo ++$i; ?>"></th>
Second one is, when using the if condition or for loop with html, it is better to use
this
style
<php? if(condition) : ?> <html> <html>
<?php endif; ?>
Thilina, In your suggestions, I don't see any refactoring techniques; yours are mostly about syntax. See [0] and [1] for references.
Also, I don't believe we use this "if" syntax in the current codebase.
[0] http://en.wikipedia.org/wiki/Refactoring [1] http://devzone.zend.com/1067/refactoring-php-code/
Thank you Marc. I will refer those articles.
Hi Marc,
In this code snippet, id attribute is not make any sense, I meant, id = th1, id = th2, etc. But class attribute is make sense. I checked the css file and js file, And I couldn't find any place where this class values used. I think it is better to change the id attribute values to meaning full one. as an example, <th id = "column" And can we use the same class for all this 'th' ? (line 199-209)
Le 2012-03-24 01:18, Thilina Buddika Abeyrathna a écrit :
On Fri, Mar 23, 2012 at 8:28 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
On Fri, Mar 23, 2012 at 5:26 PM, Marc Delisle marc@infomarc.info wrote:
Le 2012-03-22 21:57, Thilina Buddika Abeyrathna a écrit :
On Sat, Mar 17, 2012 at 9:30 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
On Sat, Mar 17, 2012 at 8:38 PM, Marc Delisle marc@infomarc.info
wrote:
Le 2012-03-17 10:30, Thilina Buddika Abeyrathna a écrit : > Hi Marc, > I did a feature request and uploaded the patch file [0] to
Sourceforge.
But > it is relevant to Context. Therefor I think it will review after the > deadline. So I like to much familiar with that Refactoring project. > When I implementing above feature I went through the tbl_change.php file. I > need to familiar with other files also which is relevant to above project. > So if you have any idea, please let me know, I am grateful if I
could
get > to know your suggestions.
Hi Thilina, I have reviewed your patch.
About refactoring, I have an idea. In the master branch, tbl_structure.php, look at lines 199-209. Can you suggest some refactoring of these lines?
Hi,
Thanks Marc. I'll look at that file.
Hi Marc,
Sorry for the delay to reply. I had a mid semester exam. So I look at
the
tbl_structure.php file and I have some suggestion for refactoring of
line
199-209. First thing is it is better to have an underscore for id field <th id="th_<?php echo ++$i; ?>"></th>
Second one is, when using the if condition or for loop with html, it is better to use
this
style
<php? if(condition) : ?> <html> <html>
<?php endif; ?>
Thilina, In your suggestions, I don't see any refactoring techniques; yours are mostly about syntax. See [0] and [1] for references.
Also, I don't believe we use this "if" syntax in the current codebase.
[0] http://en.wikipedia.org/wiki/Refactoring [1] http://devzone.zend.com/1067/refactoring-php-code/
Thank you Marc. I will refer those articles.
Hi Marc,
In this code snippet, id attribute is not make any sense, I meant, id = th1, id = th2, etc. But class attribute is make sense. I checked the css file and js file, And I couldn't find any place where this class values used. I think it is better to change the id attribute values to meaning full one. as an example,
<th id = "column" And can we use the same class for all this 'th' ? (line 199-209)
Thilina, there must be a reason to use the "id" attribute; in this case I could not find a reason so I removed them.
These class attributes were not used, except for the "action" class which is used in functions.js, displayMoreTableOpts(). There is no reason to use the same class for all these 'th', if we do nothing with the class, so I removed them (keeping "action").
Hello sir,
Im new to PHP code refactoring. Going through this thread i worked out for tbl_structure.php (line 199-209):
<?php $a =array("column","type","collation","attributes","null","default","extra","view"); $b = array("Name","Type","Collation","Attributes","Null","Default","Extra","View");
for($i=0;$i<8;$i++) { echo " <th id="th$i" class="$a[$i]" > __('$b[$i]') </th>" ;
} ?>
This code has the same output as the already existing code, plus this removes redundancy and makes maintenance easier. Does this qualifies as good refactoring?
thank you.
On 24 March 2012 18:31, Marc Delisle marc@infomarc.info wrote:
Le 2012-03-24 01:18, Thilina Buddika Abeyrathna a écrit :
On Fri, Mar 23, 2012 at 8:28 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
On Fri, Mar 23, 2012 at 5:26 PM, Marc Delisle marc@infomarc.info
wrote:
Le 2012-03-22 21:57, Thilina Buddika Abeyrathna a écrit :
On Sat, Mar 17, 2012 at 9:30 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
On Sat, Mar 17, 2012 at 8:38 PM, Marc Delisle marc@infomarc.info
wrote:
> Le 2012-03-17 10:30, Thilina Buddika Abeyrathna a écrit : >> Hi Marc, >> I did a feature request and uploaded the patch file [0] to
Sourceforge.
> But >> it is relevant to Context. Therefor I think it will review after
the
>> deadline. So I like to much familiar with that Refactoring project. >> When I implementing above feature I went through the tbl_change.php > file. I >> need to familiar with other files also which is relevant to above > project. >> So if you have any idea, please let me know, I am grateful if I
could
> get >> to know your suggestions. > > Hi Thilina, > I have reviewed your patch. > > About refactoring, I have an idea. In the master branch, > tbl_structure.php, look at lines 199-209. Can you suggest some > refactoring of these lines? > > Hi, Thanks Marc. I'll look at that file.
Hi Marc,
Sorry for the delay to reply. I had a mid semester exam. So I look at
the
tbl_structure.php file and I have some suggestion for refactoring of
line
199-209. First thing is it is better to have an underscore for id field <th id="th_<?php echo ++$i; ?>"></th>
Second one is, when using the if condition or for loop with html, it is better to use
this
style
<php? if(condition) : ?> <html> <html>
<?php endif; ?>
Thilina, In your suggestions, I don't see any refactoring techniques; yours are mostly about syntax. See [0] and [1] for references.
Also, I don't believe we use this "if" syntax in the current codebase.
[0] http://en.wikipedia.org/wiki/Refactoring [1] http://devzone.zend.com/1067/refactoring-php-code/
Thank you Marc. I will refer those articles.
Hi Marc,
In this code snippet, id attribute is not make any sense, I meant, id = th1, id = th2, etc. But class attribute is make sense. I checked the css file and js file, And I couldn't find any place where this class values used. I think it is better to change the id attribute values to meaning full one. as an example,
<th id = "column" And can we use the same class for all this 'th' ? (line 199-209)
Thilina, there must be a reason to use the "id" attribute; in this case I could not find a reason so I removed them.
These class attributes were not used, except for the "action" class which is used in functions.js, displayMoreTableOpts(). There is no reason to use the same class for all these 'th', if we do nothing with the class, so I removed them (keeping "action").
-- Marc Delisle http://infomarc.info
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
On Sat, Mar 24, 2012 at 7:13 PM, prakul agarwal discover.prakul@gmail.comwrote:
Hello sir,
Im new to PHP code refactoring. Going through this thread i worked out for tbl_structure.php (line 199-209):
<?php $a =array("column","type","collation","attributes","null","default","extra","view"); $b = array("Name","Type","Collation","Attributes","Null","Default","Extra","View"); for($i=0;$i<8;$i++) { echo " <th id=\"th$i\" class=\"$a[$i]\" > __('$b[$i]') </th>" ; } ?>
This code has the same output as the already existing code, plus thisremoves redundancy and makes maintenance easier. Does this qualifies as good refactoring?
thank you.
On 24 March 2012 18:31, Marc Delisle marc@infomarc.info wrote:
Le 2012-03-24 01:18, Thilina Buddika Abeyrathna a écrit :
On Fri, Mar 23, 2012 at 8:28 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
On Fri, Mar 23, 2012 at 5:26 PM, Marc Delisle marc@infomarc.info
wrote:
Le 2012-03-22 21:57, Thilina Buddika Abeyrathna a écrit :
On Sat, Mar 17, 2012 at 9:30 PM, Thilina Buddika Abeyrathna < thilinaabeyrathna@gmail.com> wrote:
> > > On Sat, Mar 17, 2012 at 8:38 PM, Marc Delisle marc@infomarc.info
wrote:
> >> Le 2012-03-17 10:30, Thilina Buddika Abeyrathna a écrit : >>> Hi Marc, >>> I did a feature request and uploaded the patch file [0] to
Sourceforge.
>> But >>> it is relevant to Context. Therefor I think it will review after
the
>>> deadline. So I like to much familiar with that Refactoring
project.
>>> When I implementing above feature I went through the
tbl_change.php
>> file. I >>> need to familiar with other files also which is relevant to above >> project. >>> So if you have any idea, please let me know, I am grateful if I
could
>> get >>> to know your suggestions. >> >> Hi Thilina, >> I have reviewed your patch. >> >> About refactoring, I have an idea. In the master branch, >> tbl_structure.php, look at lines 199-209. Can you suggest some >> refactoring of these lines? >> >> Hi, > Thanks Marc. I'll look at that file. > > Hi Marc,
Sorry for the delay to reply. I had a mid semester exam. So I look at
the
tbl_structure.php file and I have some suggestion for refactoring of
line
199-209. First thing is it is better to have an underscore for id field <th id="th_<?php echo ++$i; ?>"></th>
Second one is, when using the if condition or for loop with html, it is better to
use
this
style
<php? if(condition) : ?> <html> <html>
<?php endif; ?>
Thilina, In your suggestions, I don't see any refactoring techniques; yours are mostly about syntax. See [0] and [1] for references.
Also, I don't believe we use this "if" syntax in the current codebase.
[0] http://en.wikipedia.org/wiki/Refactoring [1] http://devzone.zend.com/1067/refactoring-php-code/
Thank you Marc. I will refer those articles.
Hi Marc,
In this code snippet, id attribute is not make any sense, I meant, id = th1, id = th2, etc. But class attribute is make sense. I checked the css file and js file, And I couldn't find any place where this class values used. I think it is better to change the id attribute values to meaning full one. as an example,
<th id = "column" And can we use the same class for all this 'th' ? (line 199-209)
Thilina, there must be a reason to use the "id" attribute; in this case I could not find a reason so I removed them.
Hi Marc,
I didn't mean that removing id attribute. I meant value of id attribute (eg. th1, th2). Sorry for the mis communication.
These class attributes were not used, except for the "action" class
which is used in functions.js, displayMoreTableOpts(). There is no reason to use the same class for all these 'th', if we do nothing with the class, so I removed them (keeping "action").
Anyway, thank you for your comment.
--
Marc Delisle http://infomarc.info
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
-- Prakul Agarwal, Indian Institute of Information Technology, Allahabad
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
Le 2012-03-24 11:19, Thilina Buddika Abeyrathna a écrit :
Thilina, there must be a reason to use the "id" attribute; in this case I could not find a reason so I removed them.
Hi Marc,
I didn't mean that removing id attribute. I meant value of id attribute (eg. th1, th2). Sorry for the mis communication.
I understood what you meant about the value of id attributes not making sense (th1, th2, etc); especially when the rest of code does not use these values.
These class attributes were not used, except for the "action" class
which is used in functions.js, displayMoreTableOpts(). There is no reason to use the same class for all these 'th', if we do nothing with the class, so I removed them (keeping "action").
Anyway, thank you for your comment.
My pleasure.
Le 2012-03-24 09:43, prakul agarwal a écrit :
Hello sir,
Im new to PHP code refactoring. Going through this thread i worked out for tbl_structure.php (line 199-209):
<?php $a =array("column","type","collation","attributes","null","default","extra","view"); $b = array("Name","Type","Collation","Attributes","Null","Default","Extra","View"); for($i=0;$i<8;$i++) { echo " <th id=\"th$i\" class=\"$a[$i]\" > __('$b[$i]') </th>" ; } ?>
This code has the same output as the already existing code, plus thisremoves redundancy and makes maintenance easier. Does this qualifies as good refactoring?
thank you.
Hi, yes, this is good refactoring, but better refactoring would be: - avoid generic names like $a, $b - avoid using the constant 8, instead counting the elements in the array
Using a loop, like you did, is what I had in mind when I proposed this challenge. Technically, after refactoring, the code had to produce the same output, as I suspect your example does.
In this discussion, however, code improvement was gained not just by refactoring, but by examining the reasons behing the code and simplifying it.
Hi, yes, this is good refactoring, but better refactoring would be:
- avoid generic names like $a, $b
- avoid using the constant 8, instead counting the elements in the array
Using a loop, like you did, is what I had in mind when I proposed this challenge. Technically, after refactoring, the code had to produce the same output, as I suspect your example does.
In this discussion, however, code improvement was gained not just by refactoring, but by examining the reasons behing the code and simplifying it.
-- Marc Delisle http://infomarc.info
Thanks for the input. and yes, i did appreciate the logical trimming of the code, in the discussion.
Op 24 maart 2012 16:26 heeft Marc Delisle marc@infomarc.info het volgende geschreven:
Le 2012-03-24 09:43, prakul agarwal a écrit :
Hello sir,
Im new to PHP code refactoring. Going through this thread i worked out for tbl_structure.php (line 199-209):
<?php $a =array("column","type","collation","attributes","null","default","extra","view"); $b = array("Name","Type","Collation","Attributes","Null","Default","Extra","View");
for($i=0;$i<8;$i++) { echo " <th id="th$i" class="$a[$i]" > __('$b[$i]') </th>" ;
} ?>
This code has the same output as the already existing code, plus this removes redundancy and makes maintenance easier. Does this qualifies as good refactoring?
thank you.
Hi, yes, this is good refactoring, but better refactoring would be:
- avoid generic names like $a, $b
- avoid using the constant 8, instead counting the elements in the array
Using a loop, like you did, is what I had in mind when I proposed this challenge. Technically, after refactoring, the code had to produce the same output, as I suspect your example does.
When using a single array instead of one (f.e. a key => value pair in this case, because there are only two lists, but there are other options), you can use a foreach. This has several advantages :
- no need to introduce $i - no need for the $i < count($array) to end the for loop. - foreach loop is better readable than a for loop. - the class name and caption are grouped in one array, making it easier to add a new column later on (with less change of a mistake, because only one array needs to be changed)
In this discussion, however, code improvement was gained not just by refactoring, but by examining the reasons behing the code and simplifying it.
-- Marc Delisle http://infomarc.info
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
-
Dieter Adriaenssens -
Marc Delisle -
prakul agarwal -
Thilina Buddika Abeyrathna