It’s not quite as simple as that. You cannot simply replace “mysql_” with “mysqli_” as the argument order has also been altered.
Way to completely ignore people’s valid criticism. You have all of the experienced programmers here saying your code is poor quality. Objectively it is, it is full of bad practices… not all security but it’s needlessly difficult to maintain. The length of your methods is a serious indicator of problems without even digging deeper[1][2][3][4]. On inspection, it’s quick to see that your code is mixing all kinds of irrelevant logic as pointed out by Jeff Mott earlier. It mixes several concerns that really shouldn’t be. Let’s quickly deconstruct: http://pastebin.com/ujpUuN1r
$GLOBALS['task_id'] = 'logoff';There is not even potential for debate about global variables[5][6][7][8][9][10][11][12] so this is an instant and serious red flag.
RDCsingleton::getInstance('mnu_control');
The same is true of singletons[5][13][14][15][16] so that’s another serious design flaw… and we’re only a few lines in!
$this->skip_validationPresumably the skip_validation property is in the parent class. Inheritance breaks encapsulation[17][18]. While a much more minor infraction than singletons and global state it’s still badly designed code.
if (is_True($external_auth_off)) {You actually have a function called is_True? Redundant and pointless. I was going to carry on but I feel the more nuanced flaws will go over your head. Many of my second year students would have been able to identify several flaws in this code.
I’ll leave you with this: Ignoring the security flaws and bad practices. It’s impossible to reuse any of that code and use a postgre or mssql database. It’s also impossible to make your login class use OAUTH or similar. It’s inherently inflexible and difficult to change.
Claiming “experience” and “seniority” in your posts above and then admitting to authoring code like this instantly negates any claim you have surrounding skill and understanding.
[1] Fowler, M (2006) CodeSmell, http://martinfowler.com/bliki/CodeSmell.html
[2] Hevery, M (2008) Flaw: Class Does too Much. http://misko.hevery.com/code-reviewers-guide/flaw-class-does-too-much/
[3] Bryton, Sérgio, Fernando Brito e Abreu, and Miguel Monteiro. (2010) Reducing Subjectivity in Code Smells Detection: Experimenting with the Long Method. Quality of Information and Communications Technology (QUATIC), 2010 Seventh International Conference. IEEE.
[4] Carneiro, F., G., Silva, M., Mara, L., Figueiredo, E., Sant’Anna, C., Garcia, A., & Mendonça, M. (2010). Identifying code smells with multiple concern views. In Software Engineering (SBES), 2010 Brazilian Symposium on(pp. 128-137). IEEE.
[5] Sayfan, M (n.d.) Avoid Global Variables, Environment Variables, and Singletonshttps://sites.google.com/site/michaelsafyan/software-engineering/avoid-global-variables-environment-variables-and-singletons
[6] Hevery, M (2008) Flaw: Brittle Global State & Singletons. http://misko.hevery.com/code-reviewers-guide/flaw-brittle-global-state-singletons/
[7] IBM (2012) Avoid modification of global and static variables http://www-01.ibm.com/support/knowledgecenter/SSGU8G_11.70.0/com.ibm.dapip.doc/ids_dapip_0673.htm
[8] Koopman, P (2010) Better Embedded System Software. ISBN: 978-0-9844490-0-2. Drumnadrochit Education LLC.
[9] Svennervberg, G (2012) Global Variables Are Evil http://codecraftingblueprints.com/global-variables-are-evil/
[10] Zakas, N (2006) Global Variables Are Evil http://www.nczonline.net/blog/2006/06/05/global-variables-are-evil/
[11] Ferreira, G (2013) Best C Coding Practices – Global variables http://guilhermemacielferreira.com/2013/06/01/best-c-coding-practices-global-variables/
[12] Crockford, D (2006) Global Domination http://www.yuiblog.com/blog/2006/06/01/global-domination/
[13] Densmore, S (2004) Why Singletons are Evil http://blogs.msdn.com/b/scottdensmore/archive/2004/05/25/140827.aspx
[14] Radford, M (2003) SINGLETON - the anti-pattern! http://accu.org/index.php/journals/337
[15] Yegge, S (2004) Singleton Considered Stupid https://sites.google.com/site/steveyegge2/singleton-considered-stupid
[16] Hevery, M (2008) Singletons are Pathological Liars http://misko.hevery.com/2008/08/17/singletons-are-pathological-liars/
[17] Gamma, E., Helm, R., Johnson, R., Vlissides, J. Design Patterns: Elements of Reusable Object-Oriented Software. ISBN: 0201633612. Addison Wesley. P.19-20
[18] Bloch, J Effective Java. ISBN: 0321356683
1 Like
Incorrect. That article is restricted to a discussion on BC breaks in the language. For comments on the quality of my code I suggest you look at http://www.tonymarston.net/php-mysql/your-code-is-crap.html instead.
Yes there is. They exist, therefore they can be used. This is not to be confused with over-used, mis-used or ab-used. Just because SOME people don’t like them is no reason to say that nobody must use them.
I disagree. Lots of people use them, and I have a perfectly valid use for them. In fact “them” is incorrect as I don’t have a separate singleton method in every class, I have a separate singleton class which has a “getinstance” method. The fact that some people have a problem with singletons is their problem, not mine.
Don’t be ridiculous. Encapsulation, Inheritance and Polymorphism are the fundamental principles of OO, so to now say that one of those principles breaks one of the others is laughable. Where did this idea come from? On the back of a packet of cornflakes? A joke in a Christmas Cracker?
Why not? When you consider that “true” can be represented by a selection of possible values - TRUE, “T”, “Y”, “1” - it is easier to put all those tests into a function than to write out the longhand code each time. You have heard of the DRY principle, I presume?
OAUTH is something that you can use in a public-facing website, but it is inappropriate for a business-facing enterprise application. I have options for a RADIUS or LDAP server instead.
Which is exactly what a singleton is, a single instance of one class. “Some people” here is a red-herring. Singletons (and globals) have been objectively shown to cause problems (limited flexibility being the main one). Using them introduces problems in your codebase, which don’t need to be there.
You can hammer in a screw rather than screwing it in. It’ll work but it’ll be a pain if you need to unscrew it again. A skilled tradesman will tell you it’s a bodge job, the fact that you can do something in an objectively inferior way doesn’t mean you should.
[quote]Don’t be ridiculous. Encapsulation, Inheritance and Polymorphism are the fundamental principles of OO, so to now say that one of those principles breaks one of the others is laughable. Where did this idea come from? On the back of a packet of cornflakes? A joke in a Christmas Cracker?
[/quote]
Programmers far more experienced than you. I provided several references, the most notable being the highly regarded Gang of Four book, but here’s a few more:
http://eprints.uni-kiel.de/14597/1/HasselbringHandbookSE2002.pdf
http://research.microsoft.com/en-us/um/people/cszypers/pub/cioo96.pdf
http://www.javaworld.com/article/2073649/core-java/why-extends-is-evil.html
http://en.wikipedia.org/wiki/Composition_over_inheritance
Anyway it’s clear that you’re stuck in your ways and nothing is going to change that. I’ve provided references from academia, books and the blogs of professional programmers to back up my points. I suggest you read them.
As others in this topic have discovered, trying to have a meaningful discussion with you is futile so I’m out.
2 Likes
It’s the 1st time I see a return type in a scripting language.
Anyway, putting the return type after the closing parenthesis. It even makes sense to me because in same languages the type is written after the word “returns”.
Well I’ve gotta say this Tony user is clearly a very unique type, browsing through his sites I find many articles that are totally opposite to the mainstream opinion on good practices, such as this one:
First time I heard DI is bad, I wonder if anyone is interested in discussing this. XD
1 Like
Continuing the discussion from The PHP 7 Revolution: Return Types and Removed Artifacts:
haha wow. That article is so full of false equivalences and things that hint at poor separation of concerns. This line is a brilliant example of a fundamental lack of understanding of basic OOP principals:
If you’re not using an object that was passed in then your class is doing too much[1] and you need to redesign the class or move some responsibilities around.
However, this actually gives us an insight into tony’s position. He’s building on top of a fundamental lack of understanding, so for the problems he’s trying to solve (that exist because he is building on top of shaky foundations) his solutions to the (wrong) problems he presents are technically along the right lines: when you have cornered yourself by making a class that does too much, then in this example, it is better to load the dependency if it’s needed. Of course that’s only true if you take his premise as true that the problem is that dependency may not be used, rather than the original class is doing too much.
If you build your water pipes out of lead then a water filter in every house sounds like a good idea. Of course most people would choose to build their pipes out of something less harmful saving them time and money.
[1] http://misko.hevery.com/code-reviewers-guide/flaw-class-does-too-much/
Please let us concentrate on discussing the topic and try our best to not get into any discussion that approaches either trolling or personal attacks.
I’ll agree that this is veering off topic. I am not opposed, however, to an honest and detailed discussion about some of Tony’s work in a new topic. It feels to me like this is a dangerous situation which needs to be dealt with, and I think we should dissect the code and show the world what’s wrong with it - not as a personal attack, but followed by actual examples and references as @TomB has done in most replies here. Let’s, thus, leave Tony’s other posts be in this particular discussion.
2 Likes
umm this… I think it may be a good idea to split this thread into two, with one focusing on the actual PHP 7 revolution, and the other on Tony’s backward compatibility concerns and issues. Its easier to keep track this way, also makes this thread more clean and navigable.
I am not the only one to question the validity of Dependency Injection. If you look at http://www.tonymarston.net/php-mysql/dependency-injection-is-evil.html#references you will see a list of links to other articles which also criticise the idea of DI.
@tony_marston404 just curious, have you had any exposure to unit testing at all? If so, how did you properly test anything without dependency injections?
I am “old school”, so I do manual testing, then send it to the QA department. This means that I do not have any need for dependency injection, so I’m not going to amend my code to incorporate a feature that I am never going to use.
Ouch. How long does manual testing take? With unit tests, after a change I can test all code paths with various inputs/outputs in seconds.
How do you do that manually? Load the page in the browser and see if it works? What if it’s a complex page that does something bashed on a large form? Do you fill out the entire form (for each possible error condition) each time? That seems like a very poor way of working both in terms of time spent and the quality of the testing.
I’d highly recommend giving TDD a try as it will improve both your code and your efficiency as a developer.
1 Like
Wow. If Tony had his way we’d never have seen php 5. The jump from 4 to 5 was no picnic either, especially if you relied on register_globals.
Old php 4 constructors are NOT GOING AWAY. They are being DEPRECATED. That’s a HUGE difference.
Tony, you claim to be “OLD SCHOOL”. If you are OLD SCHOOL you’d know that php 4 constructors aren’t going away any time soon. How long has mysql_* been deprecated? 8 - 10 years? How long did it take that major security hole “register globals” to be removed… 5-8 years?
PHP 4 is dead. That’s just the way it is. Time to catch up with the rest of us or find yourself unemployable. It’s your choice.
I for one am very happy with the direction php 7 is going. I can’t wait to install this on my machines.
The original RFC called for PHP 4 constructors to be REMOVED, not DEPRECATED as the author had already assumed that they had been deprecated.
Incorrect. I am in favour of improving the language, but not if it breaks BC for trivial reasons. The removal of register_globals was for a legitimate security issue, and it did not affect me as I had already changed my code after reading about the problem.
I no longer use PHP 4 on any of my development machines as everything runs quite happily with PHP 5. What runs now with PHP 5 I expect to run with PHP 7 without having to make any amendments.
You have not factored in the time that it takes to create all those unit tests. You also fail to realise that automated unit testing and TDD only became fashionable quite recently (within the last five years). I created my framework and my main application much earlier than that, which means that it was done without any automated unit tests, and I do not have the time (or the inclination) to learn about all the new testing tools and create retrospective tests.
I am paid to be a designer/developer, not a tester, so I cannot justify doing what I am not paid to do. The QA department is responsible for testing and that department has its own budget, its own staff and its own set of tools.
That’s how it’s been done for years, and it is quicker for me to do it this way than the “new” way.
You also fail to realise that my framework contains a huge amount of reusable code, so that when I test a new script I don’t have to test everything, just the places where I have added custom code.
It is only your opinion that it will improve my code and efficiency as a developer. I strongly disagree. It will not improve my code if I have to take an existing production component and refactor it just because the unit testing framework is unable to work with it as it is. It will not improve my efficiency as a developer if I have to spend half my time in non-development work, and writing unit tests is separate from actual development. Instead of spending 8 hours each day on development you are advocating that I spend 4 hours on development and 4 hours on testing. That means that 8 hours of development will now take me two days instead of one. How is this “more efficient”?
Tony,
First let me start by apologizing for my harsh comment. I should have slept on it before commenting. With that said, the overall point still stands.
Well then I stand corrected on this one point. Still I can’t say I disagree with the move, php 4 constructors have been considered bad practice since php 5 was released.
You’re previous posts talk about being in enterprise, which I am as well. I get that enterprise code tends to stay in production till the “end of time.” With that said, enterprise companies also won’t be upgrading to PHP 7 for some time to come.
The fact that some people consider them to be bad practice is irrelevant. What about those who don’t? Why should the opinions of the few be enforced on the many?
It would be a shame if they could not take advantage of the speed improvements and bug fixes in PHP 7 without having to refactor their entire codebase. If the core developers cannot improve PHP without breaking BC then they have only themselves to blame if the adoption rate for PHP is slow. Remember how long it took for PHP 4 sites to upgrade to PHP 5, then look at how many sites are still running 5.2 because of the amount of work required to upgrade to 5.3 and beyond. Other suppliers who treated their customers which such contempt would not last long in the commercial world, so why should we have to deal with it in the open source world?