Code review of a simple contact form

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Code review of a simple contact form

Post by VladSun »

thinsoldier wrote:
VladSun wrote: I would always vote against using comments for function/method/params description. The function/method/param name should be sufficient (and the only place) to describe what it does. The need of adding any description comment is a code smell IMHO.
An infrequently used function with several different outputs based on the input params definitely benefits from a brief reminder of how that function works and a lengthy description of how it's being used in that location. If you're going to say "well that function is obviously smelly" don't blame me, blame the php devs.
VladSun wrote: Another thing is that sooner or later the comment becomes a lie if not maintained together with code.
This is only true of a few functions. I have a few dozen projects between 6 and 12 years old where I had to maintain a few comments, but over 99% of the comments never needed to be touched, even on functions that have been totally rewritten for clarity/security/performance.
So, in general you just agreed with me :P
It's all about code readability. You don't (and you must not have to) read the execution flow by reading code meta data, but simply by reading to code itself. Should be as easy as reading a book in plain English.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Code review of a simple contact form

Post by Christopher »

thinsoldier wrote:Ok I'll skip that feature for this project. The people I had in mind when I started this are still at basic "I need a form for my contact.php" level.
That makes sense. Using PATH_INFO usually implies that you will implement a single point of entry (index.php) and a Front Controller that will dispatch to Action Controllers. The overhead and complexity can be minimal. It has the benefit allowing centralized functionality like access control and sitewide layouts. It also allows all the site's code to be placed outside of the public directory. It is definitely a step up from just using all individual PHP files as Transaction Scripts.

I have coded simple MVC implementations for projects like this in the past. It is possible. However, we are at a point where there are many features that are considered standard in MVC these days, and by implementing them the micro framework quickly becomes complex in order to keep the Action Controllers and templates simple.
(#10850)
thinsoldier
Forum Contributor
Posts: 367
Joined: Fri Jul 20, 2007 11:29 am
Contact:

Re: Code review of a simple contact form

Post by thinsoldier »

Christopher wrote: It is definitely a step up from just using all individual PHP files as Transaction Scripts.
I find some people have a mental block ( I had it myself in 2005 ) where the desire for user friendly urls does not overcome the reluctance to have to write and maintain more lines of code (php router or .htaccess mod-rewrite) for _every_ url in their website. I think the first step to getting them past that hurdle is basic regular expression knowledge.
Warning: I have no idea what I'm talking about.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Code review of a simple contact form

Post by Christopher »

thinsoldier wrote:I find some people have a mental block ( I had it myself in 2005 ) where the desire for user friendly urls does not overcome the reluctance to have to write and maintain more lines of code (php router or .htaccess mod-rewrite) for _every_ url in their website. I think the first step to getting them past that hurdle is basic regular expression knowledge.
I'm not clear how regular expression knowledge matters for this? The main benefit is centralization by changing from each page is a Transaction Script where every file includes duplicate code for the configuration, access control, header and footer -- to a centralized system where each page is a Controller and Template that are just focused on displaying the content part of the page, all the configuration, access control, header and footer are centralized and not duplicated in every file.
(#10850)
thinsoldier
Forum Contributor
Posts: 367
Joined: Fri Jul 20, 2007 11:29 am
Contact:

Re: Code review of a simple contact form

Post by thinsoldier »

Christopher wrote: I'm not clear how regular expression knowledge matters for this?
The fear comes from lack of understanding "how does it work" and the most common complex string pattern matching tool is regular expressions. Mod-rewrite is a common top search result when looking for how to do friendly urls. So before I could convince some people to use a routing library (scary oop code) or mod-rewrite (super scary non-php syntax) I had to teach them regex.
Christopher wrote: The main benefit is centralization by changing from each page is a Transaction Script where every file includes duplicate code for the configuration, access control, header and footer -- to a centralized system where each page is a Controller and Template that are just focused on displaying the content part of the page, all the configuration, access control, header and footer are centralized and not duplicated in every file.
Yes, but I was helping out people who already had their basic *.php pages and just wanted to add a contact form to what they already had.

It's going to take a lot to ease some of these people into another architectural concept. One of these people has been writing a lot of php almost daily since 2012 yet it all looks like something from the early days of PHP4. If I push them too hard too fast into things that need them to be up to speed on OOP I'll lose them. I'm already being risky by using git diffs to explain changes/improvements to code.
Warning: I have no idea what I'm talking about.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Code review of a simple contact form

Post by Christopher »

thinsoldier wrote:Yes, but I was helping out people who already had their basic *.php pages and just wanted to add a contact form to what they already had.

It's going to take a lot to ease some of these people into another architectural concept. One of these people has been writing a lot of php almost daily since 2012 yet it all looks like something from the early days of PHP4. If I push them too hard too fast into things that need them to be up to speed on OOP I'll lose them. I'm already being risky by using git diffs to explain changes/improvements to code.
Yeah, this is probably the best for this kind of site and client. Certainly the script per page style is easy to conceptualize and manage -- especially for small sites like these.
(#10850)
thinsoldier
Forum Contributor
Posts: 367
Joined: Fri Jul 20, 2007 11:29 am
Contact:

Re: Code review of a simple contact form

Post by thinsoldier »

Christopher wrote:small sites like these.
lol. your jaw would drop and you'd get a headache in disbelief if you saw the size and/or business importance what some of these people have been working on daily for the last 3 or more years. One of them is using php for the back end of online gambling games that use real money, the employee hours worked tracking of several web cafés that let people play those games, generating weekly reports about player count, hours played, money made/lost, and a bunch of other things. It also does money transfers from one player to another from from a shop in 1 town to a shop in another town so users can send money to themselves. All built with super-amateur php and this https://www.matchware.com/education/mediator.htm and this https://www.scirra.com/construct2 and similar things.
Warning: I have no idea what I'm talking about.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Code review of a simple contact form

Post by Christopher »

Yikes. Sites like those should really centralize security. Using a Front Controller certainly simplifies that.
(#10850)
thinsoldier
Forum Contributor
Posts: 367
Joined: Fri Jul 20, 2007 11:29 am
Contact:

Re: Code review of a simple contact form

Post by thinsoldier »

Christopher wrote:Yikes. Sites like those should really centralize security. Using a Front Controller certainly simplifies that.
Yes. But not going to happen on that current project. Eventually a larger gambling network will buy those shops and replace everything with their own stuff.
Warning: I have no idea what I'm talking about.
protopatterns
Forum Newbie
Posts: 9
Joined: Sun Jan 28, 2018 11:18 am

Re: Code review of a simple contact form

Post by protopatterns »

VladSun wrote:Should be as easy as reading a book in plain English.
How long did it take you before you were able to do this, assuming the code is written cleanly?

(And, hi all)
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Code review of a simple contact form

Post by Christopher »

I think VladSun's point was that if you know the programming language and the code is written clearly -- it should be easy to read. That said, VladSun is an very experienced programmer so it may take some time. Like "plain English", code can communicate explicit information just from the sequence of statements, especially if clearly implemented and formatted. However also like language, code can communicate implicit choices made by the implementer, that should also be clear to a programmer reading the code (at least after some experience).
(#10850)
Post Reply