Code review of a simple contact form
Moderator: General Moderators
-
thinsoldier
- Forum Contributor
- Posts: 367
- Joined: Fri Jul 20, 2007 11:29 am
- Contact:
Code review of a simple contact form
I found myself giving out similar advice a lot last week to beginners trying to make a contact form.
I decided to try to make a contact form that is easy enough for beginners to understand (if they bother to read the code and look at the git commits) while also trying to nudge them in the right direction so that someday soon they'll try writing better organized code.
Please have a look and give suggestions if you have the time.
Remember it's meant to try to be beginner friendly.
https://github.com/thinsoldier/php-beginner-form
I decided to try to make a contact form that is easy enough for beginners to understand (if they bother to read the code and look at the git commits) while also trying to nudge them in the right direction so that someday soon they'll try writing better organized code.
Please have a look and give suggestions if you have the time.
Remember it's meant to try to be beginner friendly.
https://github.com/thinsoldier/php-beginner-form
Warning: I have no idea what I'm talking about.
- 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
I think it looks good. I have a few comments.
1. I might use URLs with PATH_INFO instead of a 'cmd' parameter. It would be clearer what was the route and what were the form parameters.
2. I would name the HTML functions using a 'html' prefix, so htmlList() rather than arrayToList() and htmlSelect() rather than buildSelect(). That would make it clearer what is an HTML generator and what is an array function.
3. Why are the HTML helpers functions, but the array helper a class? It might be better to be consistent.
4. I might split the Controllers out into separate PHP files, even if showSuccess ends up as a one line controller. That would show the difference between the Router/Front Controller and the Page Controllers. Or if in one file maybe have the URLs be 'emailform/validate/' to show multiple Actions within a Controller.
5. I think ( ! isset( $input[$field]) || empty($input[$field] ) can be just ( ! empty($input[$field] )
6. I would use filter_var() on every field.
1. I might use URLs with PATH_INFO instead of a 'cmd' parameter. It would be clearer what was the route and what were the form parameters.
2. I would name the HTML functions using a 'html' prefix, so htmlList() rather than arrayToList() and htmlSelect() rather than buildSelect(). That would make it clearer what is an HTML generator and what is an array function.
3. Why are the HTML helpers functions, but the array helper a class? It might be better to be consistent.
4. I might split the Controllers out into separate PHP files, even if showSuccess ends up as a one line controller. That would show the difference between the Router/Front Controller and the Page Controllers. Or if in one file maybe have the URLs be 'emailform/validate/' to show multiple Actions within a Controller.
5. I think ( ! isset( $input[$field]) || empty($input[$field] ) can be just ( ! empty($input[$field] )
6. I would use filter_var() on every field.
(#10850)
-
thinsoldier
- Forum Contributor
- Posts: 367
- Joined: Fri Jul 20, 2007 11:29 am
- Contact:
Re: Code review of a simple contact form
1. $_SERVER['PATH_INFO']? I've never used that before. I'll give it a shot.
2. Will do.
3. Why are the HTML helpers functions, but the array helper a class? It might be better to be consistent.
The array helper would have to use $_POST or another global variable to work the way I wanted it to work. Maybe I should rename the folder to "helpers"? Or should I keep classes separate from functions? Or should I put the html helpers into an HtmlHelpers class and then use Composers no-namespace autoloading feature to load classes from the helpers folder?
4. I'm not certain what you're sayin for #4. I wanted to stick with "contact.php" because most beginners I've encountered don't seem enthusiastic about defining routes in a front controller for some reason.
5. Will do.
6. I've honestly never used filter_var before this
I'll give it a shot.
2. Will do.
3. Why are the HTML helpers functions, but the array helper a class? It might be better to be consistent.
The array helper would have to use $_POST or another global variable to work the way I wanted it to work. Maybe I should rename the folder to "helpers"? Or should I keep classes separate from functions? Or should I put the html helpers into an HtmlHelpers class and then use Composers no-namespace autoloading feature to load classes from the helpers folder?
4. I'm not certain what you're sayin for #4. I wanted to stick with "contact.php" because most beginners I've encountered don't seem enthusiastic about defining routes in a front controller for some reason.
5. Will do.
6. I've honestly never used filter_var before this
Warning: I have no idea what I'm talking about.
- 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
3. I think being consistent would be good. I think if you can make it minimally OO that would be good too. Maybe put them all in a "contact/" folder so they go with the script if you want to keep everything easy to install.
4. I think it is fine to keep it all in one contact.php script. I would make clear what is Controller code and what is View code.
Have you thought about how they will style the page? Maybe you should put the form code output into the middle of a wrapper template that they can customize to look like their site?
4. I think it is fine to keep it all in one contact.php script. I would make clear what is Controller code and what is View code.
Have you thought about how they will style the page? Maybe you should put the form code output into the middle of a wrapper template that they can customize to look like their site?
(#10850)
-
thinsoldier
- Forum Contributor
- Posts: 367
- Joined: Fri Jul 20, 2007 11:29 am
- Contact:
Re: Code review of a simple contact form
#4 https://github.com/thinsoldier/php-begi ... ontact.php
That's what my contact.php is an example of. Lines 1, 2, 3, and 24 is what they would add to their own php page file.
That's what my contact.php is an example of. Lines 1, 2, 3, and 24 is what they would add to their own php page file.
Last edited by thinsoldier on Mon Feb 12, 2018 11:55 pm, edited 1 time in total.
Warning: I have no idea what I'm talking about.
Re: Code review of a simple contact form
Gave this a quick look this morning but didn't have time to review it until now. I'm trying to keep in mind that this is aimed at beginners, and have had a bit of a tough time not being able to review things inline, but here are a few thoughts:
1. Clean up the formatting. It's a mess. PSR-2 is probably best, but whatever you use, just be consistent.
2. Comment your code. Functions/methods should have a doc block explaining what they do, what parameters they take -- type, name, and description -- and a return type. This is perhaps doubly important if this is aimed at beginners.
3. route_to_appropriate_function just barfs out an exit. Might be a good place to introduce both exceptions and proper use of HTTP response codes.
4. validate should probably be split into two functions; one that performs validation and another that builds up the output.
5. ArrayCompare doesn't actually compare arrays. Naming matters.
5.a. Use meaningful method names. ArrayCompare::compare is only three more characters but much clearer.
6. phpmailer-gmail. Is there much value in this? Wouldn't a more generic helper be more useful? Gmail-specific properties can be passed in through config.
1. Clean up the formatting. It's a mess. PSR-2 is probably best, but whatever you use, just be consistent.
2. Comment your code. Functions/methods should have a doc block explaining what they do, what parameters they take -- type, name, and description -- and a return type. This is perhaps doubly important if this is aimed at beginners.
3. route_to_appropriate_function just barfs out an exit. Might be a good place to introduce both exceptions and proper use of HTTP response codes.
4. validate should probably be split into two functions; one that performs validation and another that builds up the output.
5. ArrayCompare doesn't actually compare arrays. Naming matters.
5.a. Use meaningful method names. ArrayCompare::compare is only three more characters but much clearer.
6. phpmailer-gmail. Is there much value in this? Wouldn't a more generic helper be more useful? Gmail-specific properties can be passed in through config.
-
thinsoldier
- Forum Contributor
- Posts: 367
- Joined: Fri Jul 20, 2007 11:29 am
- Contact:
Re: Code review of a simple contact form
My comment didn't save. Short version: Yes, Celauran... most of that, will do.
Warning: I have no idea what I'm talking about.
- 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
My point was if they already have a layout template, then they would have to duplicate that into your file -- which means double maintenance down the road. Better if your code was designed to put into an external template file. Then they could use their or modify yours. But, I understand this is for beginners.thinsoldier wrote:That's what my contact.php is an example of. Lines 1, 2, 3, and 24 is what they would add to their own php page file.
(#10850)
Re: Code review of a simple contact form
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. Another thing is that sooner or later the comment becomes a lie if not maintained together with code.Celauran wrote:2. Comment your code. Functions/methods should have a doc block explaining what they do, what parameters they take -- type, name, and description -- and a return type. This is perhaps doubly important if this is aimed at beginners.
Still, adding doc blocks for parameters/return type is OK especially for some IDEs that utilize it (e.g. Netbeans) and for such IDEs most of it is autogenerated, so it doesn't become a "lie" as code evolves.
There are 10 types of people in this world, those who understand binary and those who don't
-
thinsoldier
- Forum Contributor
- Posts: 367
- Joined: Fri Jul 20, 2007 11:29 am
- Contact:
Re: Code review of a simple contact form
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: 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.
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.VladSun wrote: Another thing is that sooner or later the comment becomes a lie if not maintained together with code.
Warning: I have no idea what I'm talking about.
-
thinsoldier
- Forum Contributor
- Posts: 367
- Joined: Fri Jul 20, 2007 11:29 am
- Contact:
Re: Code review of a simple contact form
Could you give me an example?Christopher wrote: 1. I might use URLs with PATH_INFO instead of a 'cmd' parameter.
I vaguely remember back in the day some sites would have urls like "example.com/index.php/services"
I'm trying that on my server and it loads index.php but without the css. It seems my browser treats the "/foo" after the .php file name as a directory which means I'll have to make any css and js files absolute urls or maybe adjust something in my apache config.
Warning: I have no idea what I'm talking about.
- 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
You can do it that way, but you may need to adjust your scripts and style paths. And use rewriting to allow even cleaner URLs. There are lots of example of the rewrite config settings.
(#10850)
-
thinsoldier
- Forum Contributor
- Posts: 367
- Joined: Fri Jul 20, 2007 11:29 am
- Contact:
Re: Code review of a simple contact form
So it sounds like what you were suggesting is something I'm not familar with. I've never used http://php.net/pathinfo before. Could you give me an example of how you meant I should be using it?Christopher wrote:You can do it that way
Warning: I have no idea what I'm talking about.
- 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
Well, there are a couple things relating to user "clean URLs" that you have run into. For example, I use a common outer template for most of my sites that has the <base> tag set in the header. You can use rewrite rules in the web server or not to eliminate the need for 'index.php/' in the URL.
The information for the route is available in $_SERVER['PATH_INFO'] from the request, not the pathinfo() function that parses paths.
The information for the route is available in $_SERVER['PATH_INFO'] from the request, not the pathinfo() function that parses paths.
(#10850)
-
thinsoldier
- Forum Contributor
- Posts: 367
- Joined: Fri Jul 20, 2007 11:29 am
- Contact:
Re: Code review of a simple contact form
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.
Warning: I have no idea what I'm talking about.