Hi Chanaka,
I stumbled upon this piece of code (random pick) :
PMA_CommonFunctions::getInstance()->backquote($_REQUEST['view']['name'])
and it made me wonder if using a singleton for PMA_CommonFunctions is necessary, because basicaly PMA_Commonfunctions is a collection of methods, not really a 'living' object. So then I had a look at the class, and I discovered that the class has no class variables (apart from the _instance variable) and an empty constructor. So basicly this should be a static class, with static methods, because no instance is needed for it to work.
And the above piece of code will become :
PMA_CommonFunctions::backquote($_REQUEST['view']['name'])
BTW: If you convert it to a static class, don't forget to replace the $this->method() calls by the static equivalent self::method(). BTW2 : you forgot to add the if (! defined('PHPMYADMIN')), check in the beginning of the file, see [0].
[0] http://wiki.phpmyadmin.net/pma/File_template
Hi Dieter,
First sorry for top posting. I'm using my phone to send this.
As I remember I discussed about this class structure in the mailing list before. At that time I decided to use singleton pattern for this class.
Anyway, I got the point you mentioned. Since the class itself have no properties singleton may not be essential here. I'll look more on what are the pros and cons between these two approaches in this kind background. Hope other also will share their ideas on this.
However, since I need to stop my work by tomorrow according to the GSoC timeline (Carol ask to do so) I'll take the responsibility of any necessary changes regarding this after GSoC session.
Regards !
Chanaka
On Aug 18, 2012 6:10 PM, "Dieter Adriaenssens" < dieter.adriaenssens@gmail.com> wrote:
Hi Chanaka,
I stumbled upon this piece of code (random pick) :
PMA_CommonFunctions::getInstance()->backquote($_REQUEST['view']['name'])
and it made me wonder if using a singleton for PMA_CommonFunctions is necessary, because basicaly PMA_Commonfunctions is a collection of methods, not really a 'living' object. So then I had a look at the class, and I discovered that the class has no class variables (apart from the _instance variable) and an empty constructor. So basicly this should be a static class, with static methods, because no instance is needed for it to work.
And the above piece of code will become :
PMA_CommonFunctions::backquote($_REQUEST['view']['name'])
BTW: If you convert it to a static class, don't forget to replace the $this->method() calls by the static equivalent self::method(). BTW2 : you forgot to add the if (! defined('PHPMYADMIN')), check in the beginning of the file, see [0].
[0] http://wiki.phpmyadmin.net/pma/File_template
Kind regards,
Dieter Adriaenssens
------------------------------------------------------------------------------
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@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
Just in case you guys decide to change this class from a singleton to something else, may I recommend that this gets done after GSoC? As IMO such a move will create a whole bunch of conflicts for everyone to resolve and yet there is no clear advantage other than shorter syntax in this move.
Bye, Rouslan
On 18/08/2012 18:22, Chanaka Dharmarathna wrote:
Hi Dieter,
First sorry for top posting. I'm using my phone to send this.
As I remember I discussed about this class structure in the mailing list before. At that time I decided to use singleton pattern for this class.
Anyway, I got the point you mentioned. Since the class itself have no properties singleton may not be essential here. I'll look more on what are the pros and cons between these two approaches in this kind background. Hope other also will share their ideas on this.
However, since I need to stop my work by tomorrow according to the GSoC timeline (Carol ask to do so) I'll take the responsibility of any necessary changes regarding this after GSoC session.
Regards !
Chanaka
On Aug 18, 2012 6:10 PM, "Dieter Adriaenssens" <dieter.adriaenssens@gmail.com mailto:dieter.adriaenssens@gmail.com> wrote:
Hi Chanaka,
I stumbled upon this piece of code (random pick) :
PMA_CommonFunctions::getInstance()->backquote($_REQUEST['view']['name'])
and it made me wonder if using a singleton for PMA_CommonFunctions is necessary, because basicaly PMA_Commonfunctions is a collection of methods, not really a 'living' object. So then I had a look at the class, and I discovered that the class has no class variables (apart from the _instance variable) and an empty constructor. So basicly this should be a static class, with static methods, because no instance is needed for it to work.
And the above piece of code will become :
PMA_CommonFunctions::backquote($_REQUEST['view']['name'])
BTW: If you convert it to a static class, don't forget to replace the $this->method() calls by the static equivalent self::method(). BTW2 : you forgot to add the if (! defined('PHPMYADMIN')), check in the beginning of the file, see [0].
[0] http://wiki.phpmyadmin.net/pma/File_template
Kind regards,
Dieter Adriaenssens
On Sat, Aug 18, 2012 at 10:52 PM, Rouslan Placella rouslan@placella.comwrote:
Just in case you guys decide to change this class from a singleton to something else, may I recommend that this gets done after GSoC? As IMO such a move will create a whole bunch of conflicts for everyone to resolve
I support it to be done after most GSoC development gets merged to upstream.
and yet there is no clear advantage other than shorter syntax in this move.
I think making it static would be a logical move as the class is just a collection of necessary functions and doesn't require instantiating in object-oriented sense.
Bye, Rouslan
On 18/08/2012 18:22, Chanaka Dharmarathna wrote:
Hi Dieter,
First sorry for top posting. I'm using my phone to send this.
As I remember I discussed about this class structure in the mailing list before. At that time I decided to use singleton pattern for this class.
Anyway, I got the point you mentioned. Since the class itself have no properties singleton may not be essential here. I'll look more on what are the pros and cons between these two approaches in this kind background. Hope other also will share their ideas on this.
However, since I need to stop my work by tomorrow according to the GSoC timeline (Carol ask to do so) I'll take the responsibility of any necessary changes regarding this after GSoC session.
Regards !
Chanaka
On Aug 18, 2012 6:10 PM, "Dieter Adriaenssens" <dieter.adriaenssens@gmail.com mailto:dieter.adriaenssens@gmail.com> wrote:
Hi Chanaka,
I stumbled upon this piece of code (random pick) :
PMA_CommonFunctions::getInstance()->backquote($_REQUEST['view']['name'])
and it made me wonder if using a singleton for PMA_CommonFunctions is necessary, because basicaly PMA_Commonfunctions is a collection of methods, not really a 'living' object. So then I had a look at the class, and I discovered that the class has no class variables (apart from the _instance variable) and an empty constructor. So basicly this should be a static class, with static methods, because no instance is needed for it to work.
And the above piece of code will become :
PMA_CommonFunctions::backquote($_REQUEST['view']['name'])
BTW: If you convert it to a static class, don't forget to replace the $this->method() calls by the static equivalent self::method(). BTW2 : you forgot to add the if (! defined('PHPMYADMIN')), check in the beginning of the file, see [0].
[0] http://wiki.phpmyadmin.net/pma/File_template
Kind regards,
Dieter Adriaenssens
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@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
Hi,
2012/8/18 Chanaka Dharmarathna pe.chanaka.ck@gmail.com:
Hi Dieter,
First sorry for top posting. I'm using my phone to send this.
As I remember I discussed about this class structure in the mailing list before. At that time I decided to use singleton pattern for this class.
OK, I think I missed that conversation at that time.
Anyway, I got the point you mentioned. Since the class itself have no properties singleton may not be essential here. I'll look more on what are the pros and cons between these two approaches in this kind background. Hope other also will share their ideas on this.
However, since I need to stop my work by tomorrow according to the GSoC timeline (Carol ask to do so) I'll take the responsibility of any necessary changes regarding this after GSoC session.
It was not my intention to have this solved before the end of GsoC. ;) Good to hear you are willing to participate in phpMyAdmin after GSoC.
Kind regards,
Dieter
Regards !
Chanaka
On Aug 18, 2012 6:10 PM, "Dieter Adriaenssens" dieter.adriaenssens@gmail.com wrote:
Hi Chanaka,
I stumbled upon this piece of code (random pick) :
PMA_CommonFunctions::getInstance()->backquote($_REQUEST['view']['name'])
and it made me wonder if using a singleton for PMA_CommonFunctions is necessary, because basicaly PMA_Commonfunctions is a collection of methods, not really a 'living' object. So then I had a look at the class, and I discovered that the class has no class variables (apart from the _instance variable) and an empty constructor. So basicly this should be a static class, with static methods, because no instance is needed for it to work.
And the above piece of code will become :
PMA_CommonFunctions::backquote($_REQUEST['view']['name'])
BTW: If you convert it to a static class, don't forget to replace the $this->method() calls by the static equivalent self::method(). BTW2 : you forgot to add the if (! defined('PHPMYADMIN')), check in the beginning of the file, see [0].
[0] http://wiki.phpmyadmin.net/pma/File_template
Kind regards,
Dieter Adriaenssens
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@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
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@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
Hi,
I stumbled upon this piece of code (random pick) :
PMA_CommonFunctions::getInstance()->backquote($_REQUEST['view']['name'])
and it made me wonder if using a singleton for PMA_CommonFunctions is necessary, because basicaly PMA_Commonfunctions is a collection of methods, not really a 'living' object. So then I had a look at the class, and I discovered that the class has no class variables (apart from the _instance variable) and an empty constructor.
Exactly, that class has no properties except _instance which needed for implement this pattern for refer to the class instance itself. So this is not a living object.
I search bit more in this kind of cases and get to know that, this kind of utility classes are making static. And it is faster than singleton class.
From the both approaches our work can be done. But not the exact need for
use singleton pattern here.
So basicly this should be a static class, with static methods, because no instance is needed for it to work.
And the above piece of code will become :
PMA_CommonFunctions::backquote($_REQUEST['view']['name'])
BTW: If you convert it to a static class, don't forget to replace the $this->method() calls by the static equivalent self::method().
I'll do the necessary modifications if others think this approach is good.
Regards !
On Sat, Sep 15, 2012 at 12:32 PM, Chanaka Dharmarathna < pe.chanaka.ck@gmail.com> wrote:
Hi,
I stumbled upon this piece of code (random pick) :
PMA_CommonFunctions::getInstance()->backquote($_REQUEST['view']['name'])
and it made me wonder if using a singleton for PMA_CommonFunctions is necessary, because basicaly PMA_Commonfunctions is a collection of methods, not really a 'living' object. So then I had a look at the class, and I discovered that the class has no class variables (apart from the _instance variable) and an empty constructor.
Exactly, that class has no properties except _instance which needed for implement this pattern for refer to the class instance itself. So this is not a living object.
I search bit more in this kind of cases and get to know that, this kind of utility classes are making static. And it is faster than singleton class. From the both approaches our work can be done. But not the exact need for use singleton pattern here.
So basicly this should be a static class, with static methods, because no instance is needed for it to work.
And the above piece of code will become :
PMA_CommonFunctions::backquote($_REQUEST['view']['name'])
BTW: If you convert it to a static class, don't forget to replace the $this->method() calls by the static equivalent self::method().
I'll do the necessary modifications if others think this approach is good.
I also agree that making the methods static would be the better approach.
Since the class contains a set of utility functions, how about renaming the class to 'Util' on the same time? This will save a bit of line space and help us fix some coding style violations.
2012/9/15 Madhura Jayaratne madhura.cj@gmail.com:
On Sat, Sep 15, 2012 at 12:32 PM, Chanaka Dharmarathna pe.chanaka.ck@gmail.com wrote:
Hi,
I stumbled upon this piece of code (random pick) :
PMA_CommonFunctions::getInstance()->backquote($_REQUEST['view']['name'])
and it made me wonder if using a singleton for PMA_CommonFunctions is necessary, because basicaly PMA_Commonfunctions is a collection of methods, not really a 'living' object. So then I had a look at the class, and I discovered that the class has no class variables (apart from the _instance variable) and an empty constructor.
Exactly, that class has no properties except _instance which needed for implement this pattern for refer to the class instance itself. So this is not a living object.
I search bit more in this kind of cases and get to know that, this kind of utility classes are making static. And it is faster than singleton class. From the both approaches our work can be done. But not the exact need for use singleton pattern here.
So basicly this should be a static class, with static methods, because no instance is needed for it to work.
And the above piece of code will become :
PMA_CommonFunctions::backquote($_REQUEST['view']['name'])
BTW: If you convert it to a static class, don't forget to replace the $this->method() calls by the static equivalent self::method().
I'll do the necessary modifications if others think this approach is good.
I also agree that making the methods static would be the better approach. Since the class contains a set of utility functions, how about renaming the class to 'Util' on the same time? This will save a bit of line space and help us fix some coding style violations.
Hi,
I agree with using a shorter class name, but it should be 'PMA_Util'.
On Sat, Sep 15, 2012 at 9:42 PM, Dieter Adriaenssens < dieter.adriaenssens@gmail.com> wrote:
2012/9/15 Madhura Jayaratne madhura.cj@gmail.com:
On Sat, Sep 15, 2012 at 12:32 PM, Chanaka Dharmarathna pe.chanaka.ck@gmail.com wrote:
Hi,
I stumbled upon this piece of code (random pick) :
PMA_CommonFunctions::getInstance()->backquote($_REQUEST['view']['name'])
and it made me wonder if using a singleton for PMA_CommonFunctions
is
necessary, because basicaly PMA_Commonfunctions is a collection of methods, not really a 'living' object. So then I had a look at the class, and I discovered that the class
has
no class variables (apart from the _instance variable) and an empty constructor.
Exactly, that class has no properties except _instance which needed for implement this pattern for refer to the class instance itself. So this
is
not a living object.
I search bit more in this kind of cases and get to know that, this kind
of
utility classes are making static. And it is faster than singleton
class.
From the both approaches our work can be done. But not the exact need
for
use singleton pattern here.
So basicly this should be a static class, with static methods,
because
no instance is needed for it to work.
And the above piece of code will become :
PMA_CommonFunctions::backquote($_REQUEST['view']['name'])
BTW: If you convert it to a static class, don't forget to replace
the
$this->method() calls by the static equivalent self::method().
I'll do the necessary modifications if others think this approach is
good.
I also agree that making the methods static would be the better approach. Since the class contains a set of utility functions, how about renaming
the
class to 'Util' on the same time? This will save a bit of line space and help us fix some coding style violations.
Hi,
I agree with using a shorter class name, but it should be 'PMA_Util'.
Yes, thanks for pointing that out.
Hi Michal,
I did related modifications on converting PMA_CommonFunctions class to a static class. Now that class has renamed as PMA_Util. My pull request can be found at [0].
[0] : https://github.com/phpmyadmin/phpmyadmin/pull/91
Regards !
2012/9/16 Chanaka Dharmarathna pe.chanaka.ck@gmail.com:
Hi Michal,
I did related modifications on converting PMA_CommonFunctions class to a static class. Now that class has renamed as PMA_Util. My pull request can be found at [0].
Patches look good (apart from a cosmetic remark, see comment in gituhub), OK for me to merge.
Hi Dieter,
Hi Michal,
I did related modifications on converting PMA_CommonFunctions class to a static class. Now that class has renamed as PMA_Util. My pull request
can be
found at [0].
Patches look good (apart from a cosmetic remark, see comment in gituhub), OK for me to merge.
First, thanks for reviewing it soon. :) I feel I need more clarification on the comment before doing modifications. Are you mentioning about the new lines just after starting brace of functions (all functions) ? In my point, I feel if any function have very few lines (less than 10 or like that), no need of empty line at the just after starting brace of that function. But for others the empty line will help to improve readability.
Please let me know am I misunderstood your point. And if I modify this, do I need to again make a pull request as I did before ? (That was my first pull request :) )
Regards !
Hi
Dne Mon, 17 Sep 2012 11:37:09 +0530 Chanaka Dharmarathna pe.chanaka.ck@gmail.com napsal(a):
Please let me know am I misunderstood your point. And if I modify this, do I need to again make a pull request as I did before ? (That was my first pull request :) )
The pull request will automatically update once you push changes to branch which you wanted to merge.
2012/9/17 Chanaka Dharmarathna pe.chanaka.ck@gmail.com:
Hi Dieter,
Hi Michal,
I did related modifications on converting PMA_CommonFunctions class to a static class. Now that class has renamed as PMA_Util. My pull request can be found at [0].
Patches look good (apart from a cosmetic remark, see comment in gituhub), OK for me to merge.
First, thanks for reviewing it soon. :) I feel I need more clarification on the comment before doing modifications. Are you mentioning about the new lines just after starting brace of functions (all functions) ? In my point, I feel if any function have very few lines (less than 10 or like that), no need of empty line at the just after starting brace of that function. But for others the empty line will help to improve readability.
I don't think there is need for an empty line after the opening brace of a function, in any case. The newline + indent should be enough to make it readable. I don't see why starting with a blank line for longer functions would improve readability. (I'm not talking about blank lines further along the function to group statements and thus add structure and improve readability and overview)
Please let me know am I misunderstood your point. And if I modify this, do I need to again make a pull request as I did before ? (That was my first pull request :) )
As Michal said, just push any commits to your branch and it will be added to your current pull request.
Hi,
First, thanks for reviewing it soon. :)
I feel I need more clarification on the comment before doing
modifications.
Are you mentioning about the new lines just after starting brace of functions (all functions) ? In my point, I feel if any function have very few lines (less than 10 or like that), no need of empty line at the just after starting brace of
that
function. But for others the empty line will help to improve readability.
I don't think there is need for an empty line after the opening brace of a function, in any case. The newline + indent should be enough to make it readable. I don't see why starting with a blank line for longer functions would improve readability. (I'm not talking about blank lines further along the function to group statements and thus add structure and improve readability and overview)
Thats okay Dieter. I'll do that modifications soon. :)
Regards !
-
Atul Pratap Singh -
Chanaka Dharmarathna -
Dieter Adriaenssens -
Madhura Jayaratne -
Michal Čihař -
Rouslan Placella