Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  • Only use include_once or require_once. There are very specific cases when you want to use include or require, connecting (to a database?) is not one of them.
  • Naming convention. $variableNames are always camelCased, ClassNames are always CamelCaps. That's how I do it, adopt one and stick with it.
  • Your MyFunctions class should not be a class. It should be a bunch of functions.
  • That first condition is redundant. You're checking if any of the conditions are true, and then you check them one by one. You can skip the initial check.
  • Please, don't use mysql_* functions in new code. They are no longer maintained and are officially deprecated. See the red box? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.
  • You are exposed to SQL Injection attacks. This is a critical issue. See How can I prevent SQL injection with PHP How can I prevent SQL injection with PHP .
  • If your PHP file returns a JSON string, don't construct it yourself. You should only have one echo, at the bottom of the file, and that's echo json_encode($results);. Everything you want to echo should be placed into the $results array.
  • Only use include_once or require_once. There are very specific cases when you want to use include or require, connecting (to a database?) is not one of them.
  • Naming convention. $variableNames are always camelCased, ClassNames are always CamelCaps. That's how I do it, adopt one and stick with it.
  • Your MyFunctions class should not be a class. It should be a bunch of functions.
  • That first condition is redundant. You're checking if any of the conditions are true, and then you check them one by one. You can skip the initial check.
  • Please, don't use mysql_* functions in new code. They are no longer maintained and are officially deprecated. See the red box? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.
  • You are exposed to SQL Injection attacks. This is a critical issue. See How can I prevent SQL injection with PHP .
  • If your PHP file returns a JSON string, don't construct it yourself. You should only have one echo, at the bottom of the file, and that's echo json_encode($results);. Everything you want to echo should be placed into the $results array.
  • Only use include_once or require_once. There are very specific cases when you want to use include or require, connecting (to a database?) is not one of them.
  • Naming convention. $variableNames are always camelCased, ClassNames are always CamelCaps. That's how I do it, adopt one and stick with it.
  • Your MyFunctions class should not be a class. It should be a bunch of functions.
  • That first condition is redundant. You're checking if any of the conditions are true, and then you check them one by one. You can skip the initial check.
  • Please, don't use mysql_* functions in new code. They are no longer maintained and are officially deprecated. See the red box? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.
  • You are exposed to SQL Injection attacks. This is a critical issue. See How can I prevent SQL injection with PHP .
  • If your PHP file returns a JSON string, don't construct it yourself. You should only have one echo, at the bottom of the file, and that's echo json_encode($results);. Everything you want to echo should be placed into the $results array.
added 1516 characters in body
Source Link
Madara's Ghost
  • 4.8k
  • 25
  • 46

PHP

  • Only use include_once or require_once. There are very specific cases when you want to use include or require, connecting (to a database?) is not one of them.
  • Naming convention. $variableNames are always camelCased, ClassNames are always CamelCaps. That's how I do it, adopt one and stick with it.
  • Your MyFunctions class should not be a class. It should be a bunch of functions.
  • That first condition is redundant. You're checking if any of the conditions are true, and then you check them one by one. You can skip the initial check.
  • Please, don't use mysql_* functions in new code . They are no longer maintained and are officially deprecated . See the red box ? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial .
  • You are exposed to SQL Injection attacks. This is a critical issue. See How can I prevent SQL injection with PHP .
  • If your PHP file returns a JSON string, don't construct it yourself. You should only have one echo, at the bottom of the file, and that's echo json_encode($results);. Everything you want to echo should be placed into the $results array.

PHP

  • Only use include_once or require_once. There are very specific cases when you want to use include or require, connecting (to a database?) is not one of them.
  • Naming convention. $variableNames are always camelCased, ClassNames are always CamelCaps. That's how I do it, adopt one and stick with it.
  • Your MyFunctions class should not be a class. It should be a bunch of functions.
  • That first condition is redundant. You're checking if any of the conditions are true, and then you check them one by one. You can skip the initial check.
  • Please, don't use mysql_* functions in new code . They are no longer maintained and are officially deprecated . See the red box ? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial .
  • You are exposed to SQL Injection attacks. This is a critical issue. See How can I prevent SQL injection with PHP .
  • If your PHP file returns a JSON string, don't construct it yourself. You should only have one echo, at the bottom of the file, and that's echo json_encode($results);. Everything you want to echo should be placed into the $results array.
added 222 characters in body
Source Link
Madara's Ghost
  • 4.8k
  • 25
  • 46

HTML

  • Lack of consistent naming conventions and code style. Sometimes you see <script src= "js/drivers.js"> and sometimes id = 'drivers_name'. Pick one, and stick with it. It makes your code that much more readable.

  • Your <form> element lacks the method= and action= attributes.

  • You have redundant code.

  • <label>s must be associated with inputs. So the two labels at the top are not valid.

  • You have too many HTML tags per input, don't forget that you can nest the input inside a label, thus associating it implicitly, and also saving you the need for using IDs for every single input. For example:

     <label class="uk-form-label">Name
     <input type="text" name="driver_name" class="uk-form-small autocomplete_with_new_value_driver PascalCase SelectOnFocus width400">
     </label>
    
  • You don't need <div>s for each label and each input inside of a label. Use display: block; on labels and inputs to your advantage!

  • Don't use <br>s to control vertical spacing. Use margin-top and padding-top to your advantage!

  • You are using redundant attributes, for example <div align="center"> should be a div with text-align: center; set for it in a CSS stylesheet. tabindex is redundant if you code your form properly with order of elements.

  • The reset <button type="reset"> is redundant. No one uses it for any real purpose (I can just refresh) and it's extremely annoying to try and hit the submit button, only to accidentally reset the form. Please don't.

JavaScript

  • You don't need jQuery. At a glance, and especially since you've given IDs for everything, you don't really need jQuery for most of the things you do there.
  • Leverage the use of HTML5's validation. Use Feature detection to determine when to use your validation, and when to let the browser do it for you. It's semantic, and native.
  • Generalize the buttons. Why do you have 3 blocks of code for 3 buttons? They should all submit to the same page, with slightly different parameters. All processing is done at the server, client only displays the results.

HTML

  • Lack of consistent naming conventions and code style. Sometimes you see <script src= "js/drivers.js"> and sometimes id = 'drivers_name'. Pick one, and stick with it. It makes your code that much more readable.

  • Your <form> element lacks the method= and action= attributes.

  • You have redundant code.

  • <label>s must be associated with inputs. So the two labels at the top are not valid.

  • You have too many HTML tags per input, don't forget that you can nest the input inside a label, thus associating it implicitly, and also saving you the need for using IDs for every single input. For example:

     <label class="uk-form-label">Name
     <input type="text" name="driver_name" class="uk-form-small autocomplete_with_new_value_driver PascalCase SelectOnFocus width400">
     </label>
    
  • You don't need <div>s for each label and each input inside of a label. Use display: block; on labels and inputs to your advantage!

  • Don't use <br>s to control vertical spacing. Use margin-top and padding-top to your advantage!

  • You are using redundant attributes, for example <div align="center"> should be a div with text-align: center; set for it in a CSS stylesheet. tabindex is redundant if you code your form properly with order of elements.

HTML

  • Lack of consistent naming conventions and code style. Sometimes you see <script src= "js/drivers.js"> and sometimes id = 'drivers_name'. Pick one, and stick with it. It makes your code that much more readable.

  • Your <form> element lacks the method= and action= attributes.

  • You have redundant code.

  • <label>s must be associated with inputs. So the two labels at the top are not valid.

  • You have too many HTML tags per input, don't forget that you can nest the input inside a label, thus associating it implicitly, and also saving you the need for using IDs for every single input. For example:

     <label class="uk-form-label">Name
     <input type="text" name="driver_name" class="uk-form-small autocomplete_with_new_value_driver PascalCase SelectOnFocus width400">
     </label>
    
  • You don't need <div>s for each label and each input inside of a label. Use display: block; on labels and inputs to your advantage!

  • Don't use <br>s to control vertical spacing. Use margin-top and padding-top to your advantage!

  • You are using redundant attributes, for example <div align="center"> should be a div with text-align: center; set for it in a CSS stylesheet. tabindex is redundant if you code your form properly with order of elements.

  • The reset <button type="reset"> is redundant. No one uses it for any real purpose (I can just refresh) and it's extremely annoying to try and hit the submit button, only to accidentally reset the form. Please don't.

JavaScript

  • You don't need jQuery. At a glance, and especially since you've given IDs for everything, you don't really need jQuery for most of the things you do there.
  • Leverage the use of HTML5's validation. Use Feature detection to determine when to use your validation, and when to let the browser do it for you. It's semantic, and native.
  • Generalize the buttons. Why do you have 3 blocks of code for 3 buttons? They should all submit to the same page, with slightly different parameters. All processing is done at the server, client only displays the results.
Source Link
Madara's Ghost
  • 4.8k
  • 25
  • 46
Loading
lang-php

AltStyle によって変換されたページ (->オリジナル) /