Hello,
I’m brand new here and was hoping for some feedback or input.
I just finished the first version of a simple PHP/mySQL band scheduling webapp Preformatted textand was going to start using it with my band to organize our schedule.
It is just Vanilla PHP/JavaScript/CSS
I’m curious if anyone has any criticisms or input on improvements to UI and code.
It’s on gitHub
Also, I don’t have an iPhone so I was curious how it works on safari mobile..(if anyone felt like installing it).
I have not secured it with login yet. I’m doing that next.
It is available for download and install with setup instruction.
My site is running on cPanel host.
https://github.com/tacoFiesta/Simple-PHP-mySQL-Band-Schedule
github / tacoFiesta / Simple-PHP-mySQL-Band-Schedule
I’ve never shared anything on github.
Let me know if I screwed anything up.
Also if I’m out of place I apologize.
Rather than committing the zip onto GitHub, you should be committing the actual project files. So may I suggest you take down the zip, unzip it and commit those files instead.
Before doing that, also read about and use a .gitignore file to make sure that you only commit files that are important to run the project and not commit needless files.
Then perhaps people will give you more feedback as they will be able to browse the source code directly.
1 Like
Here’s a list of list of programming points (many repeated throughout the code) -
- require_once is not a function. The () around the path/file do nothing and should be removed.
- Modern php (8+) uses exceptions for database errors by default (and if you are not using php8+, you should be, and if using php <8, you should be using exceptions for database errors.) When using exceptions, none of the discrete database statement error handling logic, which is inconsistently present, will get executed upon an error and needs to be removed. Your main code only ‘sees’ error free execution. If execution continues past a statement that can throw an exception, you know that no error occurred without needing any logic to test.
- The only database exceptions you should catch and handle in your code are for user recoverable errors, such as when inserting/updating duplicate user submitted data. For all other query errors, all other type of queries, and for the connection, simply do nothing in your code and let php catch and handle any database exception.
- When you make the database connection, you need to set the character set to match your database tables. This is not optional.
- You need to EITHER use OOP mysqli notation (the error response for procedural notation for fatal errors is to just produce a warning and continue execution) OR use the much simpler and better designed PDO extension (over half of the database specific lines of code will go away).
- There’s generally no need to close database connections or free up result sets in your code since php destroys all resources when your script ends.
- There’s generally no need to close prepared query handles in your code, since well written code deals with the result from one query before going on to perform other queries.
- Except for the final variable that data is fetched into, there’s no need to come up with unique names for the variables for sql query statements, prepared query handles, result resources. Just reuse simple, $sql, $stmt, $result, … names. This will also reduce memory usage since each reuse of the variable destroys the previous memory used by it.
- I recommend that you build sql query statements in a php variable. This makes debugging easier and separates the sql query syntax as much as possible from the php code. It will also allow you to see the different sets of repetitive database code, that you can then convert to use functions or class methods.
- If your database tables are properly indexed, you don’t need the LIMIT 1 when the WHERE clause will match a single row of data via an index.
- I saw at least one case where you are querying for the MAX() value from a table and adding 1 to it yourself. This is not concurrent safe. I did not follow through with what this got used for, but you should only rely on the database to auto increment a value for you, as it perform the necessary table locking to insure that concurrent requests are handled correctly.
- Most database character set collations are case-insensitive and you don’t need to use things like LOWER(column) in comparisons.
- Don’t try to SELECT existing data to decided if you are going to INSERT new data. This is not concurrent safe and takes 2x the amount of code. Instead, you should have a unique index defined for the column(s) that must be unique, then simply attempt to insert the data and detect in the exception error handling for the query if a duplicate index error (number) occurred. If there was no duplicate index error, the data was unique and was inserted. If there is a duplicate index error, the data already existed.
- There is a lot of repetitive processing, where the only thing that changes from one instance to the next is the name/meaning of the data. you should create a function/class method containing the unique processing, then reuse the processing code by suppling just the information that’s different as its input when you call it or in the case of an OOP class, when you create an instant of the class.
- When the last thing in a file is php code, you should leave out the closing ?> tag.
- When the last thing in a file is php code, there’s no need for an exit; statement to stop php code execution, since there is no more php code.
- Database DELETE operations need to be handled via post method requests, not links.
- One of the points of prepared queries that will get executed with different sets of data, is that you only prepare and bind the inputs once. Any bind statement needs to be outside of and before the start of any looping. The code inside the loop is only populating the already bound variables and executing the query.
- Defining php variables for the database connection credentials, only to switch to defined constants, which only the connection code uses, is pointless. Simply use the defined variables in the connection code.
- The database server value should be a variable. Not every installation will use localhost for the database connection.
- The page navigation, if not using a database based CMS (Content Management System), should have a defining array with the choices, then simply loop over the array to dynamically produce the output.
- Post method form processing should keep the input data as a set, in a php array variable, then operate on elements in this array variable throughout the rest of code, i.e. don’t write out line after line of code creating discrete variables for every field.
- Once you do item #22 on this list, you can trim all the input data at once, using one single line of code.
- Except for unchecked checkbox/radio fields, all other form fields will be set once the form has been submitted. By throwing null-coalescing operators at these ‘always set’ fields, you are hiding simple typo mistakes, making debugging harder, and doing a lot of unnecessary typing.
- The msyqli and PDO extension have ‘fetch all’ methods. You don’t need to loop to fetch all the data from a query.
- To get a form to submit to the exact same URL of page it is on, you can leave out the entire action attribute. Doing this will also include any existing get parameters on the end of the URL, so you don’t need any php code to cause this to happen.
- For
<label></label> tags to work, you must either have a for=‘…’ attribute in the opening label tag and a corresponding id=‘…’ in the field, or you can simply put the closing </label> tag after the field it corresponds to.
- You need to validate the resulting web pages at validator.w3.org
- You can apply htmlspecialchars() to all elements in an array variable using array_map(). There’s no need to repeat the htmlspecialchars() call for each element in the array.
- mysqli_real_escape_string() is NOT used when you are using prepared queries. Remove the mysqli_real_escape_string() calls.
- As an advanced programming exercise, for the CrUD (Create, Update, and Delete) operations, you can define an array of the expected fields and their - label, type, validation rules, and processing rules, for each operation, then simply loop over this defining array to dynamically validate and process the form data and dynamically produce and populate the corresponding forms.
3 Likes
WOW! Thank you for your time, I really appreciate it.
This is the first time anyone has looked over my code and I think it’s obvious that I am self taught after reading your response.
I will have to read through it a few more times.
Thank you!
1 Like
See now that you have the files and people can see the code, they can make these kinds of suggestions. Not only that, others may then fork your code, make corrections and then issue pull requests to you to help you fix up issues you are encountering. In other words, others can help you with your project! 
1 Like
Maybe this should be a pinned programming tips post
1 Like
I have updated everything and uploaded to github.
I read over your response many times, spent a long time researching.
DB Functions was difficult trying figure out class or functions.
I went with function because of the small size of the project.
Thank you for the input.