[Phpmyadmin-devel] Refactoring: Displaying query results

Fri May 11 12:48:09 CEST 2012

On 11/05/12 08:26, Chanaka Dharmarathna wrote:
> On Fri, May 11, 2012 at 11:12 AM, Chanaka Dharmarathna<
> pe.chanaka.ck at gmail.com>  wrote:
>
>>
>>
>> On Fri, May 11, 2012 at 7:05 AM, Chanaka Dharmarathna<
>> pe.chanaka.ck at gmail.com>  wrote:
>>
>>>
>>>
>>> On Fri, May 11, 2012 at 12:39 AM, Rouslan Placella<rouslan at placella.com>wrote:
>>>
>>>> On 09/05/12 21:08, Chanaka Dharmarathna wrote:
>>>>> On Wed, May 9, 2012 at 2:27 PM, Rouslan Placella<rouslan at placella.com
>>>>> wrote:
>>>>>
>>>>>> On 09/05/12 09:39, Chanaka Dharmarathna wrote:
>>>>>>> On Wed, May 9, 2012 at 9:21 AM, Chanaka Dharmarathna<
>>>>>>> pe.chanaka.ck at gmail.com>    wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, May 9, 2012 at 1:19 AM, Rouslan Placella<
>>>> rouslan at placella.com
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> On 08/05/12 20:32, Chanaka Dharmarathna wrote:
>>>>>>>>>> On Tue, May 8, 2012 at 7:23 PM, Chanaka Dharmarathna<
>>>>>>>>>> pe.chanaka.ck at gmail.com>     wrote:
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Tue, May 8, 2012 at 7:08 PM, Marc Delisle<marc at infomarc.info>
>>>>>>>>>     wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Le 2012-05-08 09:25, Chanaka Dharmarathna a écrit :
>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> See "Class Variables and Methods" in [0].
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [0] http://pear.php.net/manual/en/standards.naming.php
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Marc,
>>>>>>>>>>>>>
>>>>>>>>>>>>> I went through that document.
>>>>>>>>>>>>> If the function is global, it will be like
>>>>>>>>> 'PMA_getDivForSliderEffect()'
>>>>>>>>>>>>> And if it is a local function (for particular class), it will
>>>> be
>>>>>> like
>>>>>>>>>>>>> 'getDivForSliderEffect()'.
>>>>>>>>>>>>
>>>>>>>>>>>> If this is a public method, yes. If a private one, prefix its
>>>> name
>>>>>>>>> with
>>>>>>>>>>>> an underscore.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Though I went through that document before this, I couldn't get
>>>>>> that
>>>>>>>>>>>> point.
>>>>>>>>>>>>> Thanks for pointing out that.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> Marc Delisle
>>>>>>>>>>>> http://infomarc.info
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>> ------------------------------------------------------------------------------
>>>>>>>>>>>> Live Security Virtual Conference
>>>>>>>>>>>> Exclusive live event will cover all the ways today's security
>>>> and
>>>>>>>>>>>> threat landscape has changed and how IT managers can respond.
>>>>>>>>> Discussions
>>>>>>>>>>>> will include endpoint security, mobile security and the latest
>>>> in
>>>>>>>>> malware
>>>>>>>>>>>> threats.
>>>> http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>> Phpmyadmin-devel mailing list
>>>>>>>>>>>> Phpmyadmin-devel at lists.sourceforge.net
>>>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>> Got it Marc.
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> ____________________________________
>>>>>>>>>>>
>>>>>>>>>>> Chanaka Indrajith
>>>>>>>>>>> Bsc.Computer Engineering Undergraduate
>>>>>>>>>>> Faculty of Engineering
>>>>>>>>>>> University of Peradeniya
>>>>>>>>>>> Sri Lanka
>>>>>>>>>>> ____________________________________
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> Hi Michal,
>>>>>>>>>>
>>>>>>>>>> I just did my first commit to my repository [0].
>>>>>>>>>> In that commit I have done some refactoring on
>>>>>>>>> "PMA_displayTableHeaders()"
>>>>>>>>>> function in "display_tbl.php" file.
>>>>>>>>>> All HTML renderings by "echo"s were removed and those HTML was
>>>>>> rendered
>>>>>>>>> at
>>>>>>>>>> the bottom of the function at once.
>>>>>>>>>> As well some related functions in "common.lib.php" file also were
>>>>>>>>> modified
>>>>>>>>>> in order to return a string of HTML content.
>>>>>>>>>> But the tests were not yet modified according to my modifications.
>>>>>>>>>> Since there was a conflict after my first local commit [1] (git
>>>>>>>>> commit), I
>>>>>>>>>> had to resolve conflicts and commit again. (that is [0])
>>>>>>>>>>
>>>>>>>>>> I have already installed phpUnit framework.
>>>>>>>>>> It will be very helpful if you can suggest me some documentation
>>>> to
>>>>>>>>> setup
>>>>>>>>>> and run tests in PMA (selenium etc).
>>>>>>>>>>
>>>>>>>>>> I'm intending to do the above refactoring procedure to all the
>>>>>>>>> functions in
>>>>>>>>>> "display_tbl.lib.php" file before moving to any other task.
>>>>>>>>>>
>>>>>>>>>> [0] :
>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>> https://github.com/Chanaka/phpmyadmin/commit/dfc73d3494445ff9430e5d0b4f33f800c6e480e0
>>>>>>>>>> [1] :
>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>> https://github.com/Chanaka/phpmyadmin/commit/004f4f5258a0820975e2da9fa02a6d3ba6622368
>>>>>>>>>>
>>>>>>>>>> Regards !
>>>>>>>>>
>>>>>>>>> I had a very quick look at your commit. I think that you're
>>>> indenting
>>>>>>>>> some lines incorrectly (but I stand to be corrected on this one).
>>>>>> AFAIK,
>>>>>>>>> you don't need to line up the operators, just indent by 4 spaces.
>>>>>> Example:
>>>>>>>>>
>>>>>>>>> // THIS IS BAD
>>>>>>>>> $radio_html .= '<label for="' . $html_field_id . '">'
>>>>>>>>>                . $choice_label
>>>>>>>>>                . '</label>';
>>>>>>>>>
>>>>>>>>> // THIS IS GOOD
>>>>>>>>> $radio_html .= '<label for="' . $html_field_id . '">'
>>>>>>>>>        . $choice_label
>>>>>>>>>        . '</label>';
>>>>>>>>>
>>>>>>>>> // THIS IS ALSO GOOD
>>>>>>>>> $radio_html .= '<label for="' . $html_field_id . '">';
>>>>>>>>> $radio_html .= $choice_label;
>>>>>>>>> $radio_html .= '</label>';
>>>>>>>>>
>>>>>>>>> Bye,
>>>>>>>>> Rouslan
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>> ------------------------------------------------------------------------------
>>>>>>>>> Live Security Virtual Conference
>>>>>>>>> Exclusive live event will cover all the ways today's security and
>>>>>>>>> threat landscape has changed and how IT managers can respond.
>>>>>> Discussions
>>>>>>>>> will include endpoint security, mobile security and the latest in
>>>>>> malware
>>>>>>>>> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>>>>>>>>> _______________________________________________
>>>>>>>>> Phpmyadmin-devel mailing list
>>>>>>>>> Phpmyadmin-devel at lists.sourceforge.net
>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Rouslan,
>>>>>>>>
>>>>>>>> Thanks for your suggestion.
>>>>>>>> I'll follow the way you mentioned as "THIS IS GOOD".
>>>>>>>>
>>>>>>>> Regards !
>>>>>>>>
>>>>>>>> --
>>>>>>>> ____________________________________
>>>>>>>>
>>>>>>>> Chanaka Indrajith
>>>>>>>> Bsc.Computer Engineering Undergraduate
>>>>>>>> Faculty of Engineering
>>>>>>>> University of Peradeniya
>>>>>>>> Sri Lanka
>>>>>>>> ____________________________________
>>>>>>>>
>>>>>>>>
>>>>>>> Hi Michal,
>>>>>>>
>>>>>>> In some functions there are pure HTML code snippets. (Ex :-
>>>>>>> PMA_displayTableNavigationOneButton() function in
>>>> display_tbl.lib.php)
>>>>>>>
>>>>>>> As I feel, its not good to convert all these snippets as PHP string
>>>> and
>>>>>>> render them. So I'm intending to get those pure HTML snippets into
>>>>>> separate
>>>>>>> function. That function can be named as
>>>> 'PMA_renderTableNavigationForm'
>>>>>>> (for above case).
>>>>>>> What do you think about this ?
>>>>>>>
>>>>>>> As well is there any reason for the phrase 'One' in the function
>>>>>>> "PMA_displayTableNavigation*One*Button()" ?
>>>>>>>
>>>>>>> Regards !
>>>>>>
>>>>>> I think that the above example is *exactly* the kind of stuff that
>>>> needs
>>>>>> to be converted to strings.
>>>>>>
>>>>>> Bye,
>>>>>> Rouslan
>>>>>>
>>>>>>
>>>>>>
>>>> ------------------------------------------------------------------------------
>>>>>> Live Security Virtual Conference
>>>>>> Exclusive live event will cover all the ways today's security and
>>>>>> threat landscape has changed and how IT managers can respond.
>>>> Discussions
>>>>>> will include endpoint security, mobile security and the latest in
>>>> malware
>>>>>> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>>>>>> _______________________________________________
>>>>>> Phpmyadmin-devel mailing list
>>>>>> Phpmyadmin-devel at lists.sourceforge.net
>>>>>> https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
>>>>>>
>>>>>
>>>>> Hi Rouslan,
>>>>>
>>>>> I have little doubt of the way I understand your point.
>>>>>
>>>>> You are suggesting to render HTML content by PHP string, instead of
>>>> using
>>>>> pure HTML, though there is nicely placed HTML code snippets. (around
>>>> 5, 6
>>>>> lines)
>>>>> Am I correct Rouslan ? Sorry if I'm repeat the wheel.
>>>>
>>>> Yes, that's correct. And I don't thing that those lines are nice at all.
>>>>
>>>> Bye,
>>>> Rouslan
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>> Live Security Virtual Conference
>>>> Exclusive live event will cover all the ways today's security and
>>>> threat landscape has changed and how IT managers can respond. Discussions
>>>> will include endpoint security, mobile security and the latest in malware
>>>> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>>>> _______________________________________________
>>>> Phpmyadmin-devel mailing list
>>>> Phpmyadmin-devel at lists.sourceforge.net
>>>> https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
>>>>
>>>
>>> Hi Rouslan,
>>>
>>> I was already thinking of do the same thing you pointed.
>>> It will cause some sort of problems if I do the previous thing I
>>> suggested.
>>> Thanks for your help.
>>>
>>>
>>> Regards !
>>> --
>>> ____________________________________
>>>
>>> Chanaka Indrajith
>>> Bsc.Computer Engineering Undergraduate
>>> Faculty of Engineering
>>> University of Peradeniya
>>> Sri Lanka
>>> ____________________________________
>>>
>>>
>> Hi Michal,
>>
>> The function "PMA_displayTableBody()" in display_tbl.lib.php is always
>> return true.
>> But it does not seems like, useful for other place where it have been
>> used. (Used only in "PMA_displayTable()" function in same file)
>>
>> Inside "PMA_displayTable()" function it just assign the return value
>> (true) to variable called '$clause_is_unique'.
>> And variable is only used at one place. it is,
>>
>> echo '<input type="hidden" name="clause_is_unique"'
>>              .' value="' . *$clause_is_unique* . '" />' . "\n";
>>
>> So, if this always return true, why don't we hard code true in the above
>> case.
>> And modify the "PMA_displayTableBody()" function to return string of html
>> content.
>>
>> Am I going wrong ?
>>
>>
>> Regards !
>> --
>> ____________________________________
>>
>> Chanaka Indrajith
>> Bsc.Computer Engineering Undergraduate
>> Faculty of Engineering
>> University of Peradeniya
>> Sri Lanka
>> ____________________________________
>>
>>
> Hi Michal,
>
> I think I was wrong.
> Though the block comments of PMA_displayTableBody function, mentioned that
> it always return true,
> when digging the code, and looking at the bottom comments, seems like its
> very important.
>
> I'll modify the function to return an array, having two elements which are
> string of HTML content and boolean ($clause_is_unique)
> The name will change from "PMA_displayTableBody" to
> "PMA_getTableBodyParams".
> I hope to see your suggestions.
>
> Regards !

It looks to me like you could easily get a value for $clause_is_unique 
outside of that function, so that you can leave its name and purpose as 
is. And you'll be able return a single string from it.

Bye,
Rouslan