I would like to get my code rated

  • This topic has 17 replies, 1 voice, and was last updated 1 week, 5 days ago by chikabalagamer.
Viewing 15 posts - 1 through 15 (of 18 total)
  • Author
    Posts
  • #5577
    KaryM711
    Participant

    Hi everybody,

    I am a 17 years old self-taught programmer and I have created a forum using php and mysql a couple of months ago. As title says I would like to get my code rated (I already know it’s terrible actually but I wonder how much and how to improve) [https://github.com/adri711/M711Forum](https://github.com/adri711/M711Forum)

    I took a couple of screenshots [https://imgur.com/a/kUoyVHK](https://imgur.com/a/kUoyVHK)

    Should I continue working on this or learn to use a framework and start over again? Thanks.

    #5578
    Flibbertygibbety22
    Guest

    Good on you for posting this! Making something that works, and then asking for feedback are two of the hardest steps in programming. I imagine it feels a bit scary posting your code on somewhere like Reddit, so good on you.

    A few things:

    * There’s something called “MVC” (Model View Controller) which is a way of separating your code into different parts. Nearly all PHP frameworks work like this, in fact many languages use it. Your HTML goes in the views. The code for loading data from the database goes in the models. And the code that connects the two goes in “Controllers”. This neatly separates your code out into different parts, which makes it a lot easier.
    * Composer is now the standard way of managing your files. So long as you put a ‘namespace’ on your files, and put the files in the right folders, Composer will automatically load all the files for you, so you no longer need to have `require` or `include` statements.
    * Use the PDO library for working with the database. There’s a big security risk (called “SQL injection”) which is pretty dangerous – your code is susceptible to it, but can be easily fixed with PDO. It also makes it easier to manage your queries, so it’s worth it.

    It is true that these days, we tend to use frameworks to do a lot of the work for us – working out which files to load on which URL, working with the database, etc. That said, one of the downsides of frameworks is that you can miss a lot of the important stuff. But you’ve made such a good start here, I wouldn’t suggest throwing everything away.

    My suggestion would be to adapt your code to work with a framework.

    * If you have a project you need to finish, use Laravel. It’s easy to get going, and provides a lot of functionality out of the box.
    * If you’ve got time to learn, use Symfony. It will teach you more of the underlying concepts, that Laravel tends to hide from you with magic.

    Good luck, feel free to send me a message if you want more feedback, help or private mentoring.

    #5579
    alexanderpas
    Guest

    Since it hasn’t been mentioned, I would like to point out [PHP: The Right Way](https://phptherightway.com/)

    #5580
    justaphpguy
    Guest

    Have my upvote for going to reddit and face whatever it throws back. That takes some guts!

    #5581
    plump_tomato
    Guest

    Watch this:

    [https://laracasts.com/series/php-for-beginners](https://laracasts.com/series/php-for-beginners)

    Then try again. Then learn a framework if you want to, but there’s almost no reason not to.

    Also, I took one quick glance and saw SQL injection vulnerabilities. You don’t ever want to put $_POST data in your SQL queries directly. Look into PDO and prepared statements.

    #5582
    mds1256
    Guest

    Issues with sql injection – look at PDO and prepared queries. Lots of security issues in this code. You are also storing the password in plain text…..

    #5583
    joppedc
    Guest

    Would definetly recommend using a framework like Symfony. This’ll also help you learn more about the structure of a general PHP project. Frameworks are the way to go imo. there’s not a lot of businesses still using plain php, but its nice to learn the basics.

    #5584
    colshrapnel
    Guest

    To be honest, there are too much rookie mistakes, if not all of them.

    Let me rewrite a couple files for you, just to show you that even without a framework, with only basic options you can have a secure, clean and reusable code.

    ###database.php should be split into 3 files:

    – **bootstrap.php** would contain all configuration options and also would include database.php

    This way you’ll be able to change configuration options without touching program files

    <?php
    session_start();

    /* MySQL host details goes here */
    $db_host = ‘localhost’;
    $db_port = ‘3306’;
    $db_username = ‘root’;
    $db_password = ”;
    $db_name = ‘M711AKJ’;
    $db_charset=’utf8mb4′;

    /* admin account details */
    define(“ADMIN_USERNAME”, “administrator”);
    define(“ADMIN_PASSWORD”, “administrator”);
    define(“ADMIN_EMAIL”, “[email protected]”);
    define(“DASHBOARD_ID”, 0);

    require ‘database.php’;

    – **database.php**

    would only contain the database connection code

    <?php
    mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
    try {
    require __DIR__.’/db_credentials.php’;
    $db = new mysqli($db_host, $db_username, $db_password, $db_name, $db_port);
    $db->set_charset($db_charset);
    } catch (mysqli_sql_exception $e) {
    throw new mysqli_sql_exception($e->getMessage(), $e->getCode());
    } finally {
    unset($db_host, $db_username, $db_password, $db_name, $db_port);
    }

    function prepared_query($mysqli, $sql, $params, $types = “”)
    {
    $types = $types ?: str_repeat(“s”, count($params));
    $stmt = $mysqli->prepare($sql);
    $stmt->bind_param($types, …$params);
    $stmt->execute();
    return $stmt;
    }

    Detailed explanation for all these commands can be found in by article on [how to connect with mysqli](https://phpdelusions.net/mysqli/mysqli_connect)

    – **install.php**

    would contain all the database seeding code. It shouldn’t be run on every request.

    <?php

    exit; // by default this file does nothing

    require ‘bootstrap.php’;

    $query = “CREATE TABLE users (
    id INT(6) UNSIGNED AUTO_INCREMENT PRIMARY KEY,
    username varchar(30) NOT NULL,
    email varchar(255) NOT NULL,
    password varchar(25) NOT NULL,
    registerDate date NOT NULL,
    birthday date NOT NULL,
    biography varchar(500),
    rank INT(11),
    reputation INT(11))”;
    mysqli_query($db, $query);
    // and so on

    Now let’s rewrite your **login.php** to make it’s structure more sensible, secure and maintainable.

    All the database interactions should go first. Start any output only when you finished all data manipulations. Right now you are sending HTTP header and redirecting a user to another page after printing out half the HTML. It just makes no sense and on a properly configured server would give you an infamous Headers already sent error. Other improvements were already mentioned above

    – any variable should never make it to SQL query, it should always go through a parameter.
    – the password must be properly hashed and thus verified using the proper function

    here it goes:

    <?php
    require ‘bootstrap.php’;

    if(isset($_SESSION[‘username’]))
    {
    header(‘Location:index.php’);
    exit();
    }
    if(isset($_POST[‘loginsubmit’])) {
    $sql = “SELECT * FROM users WHERE username = ?”;
    $stmt = prepared_query($db, $sql, $_POST[‘usernameinput’]);
    $result = $stmt->get_result();
    $user = $result->fetch_assoc();

    if ($user && password_verify($_POST[‘passwordinput’], $user[‘pass’]))
    {
    session_start();
    $_SESSION[‘username’] = $user[‘username’];
    header(‘Location: index.php’);
    exit;
    } else {
    print(“<p style=’color=red;’>We could not find an account with that username and password.</p>”);
    }
    }

    require(‘header.php’);
    ?>

    <div class=’box’>
    <h2>M711 – Login</h2>
    <p>In order to play the game you need to have an account, login to your account or <a href=’register.php’>register here</a>.</p>
    <form method=’post’>
    <table>
    <tr>
    <td><p for=usernameinput>Username:</label></td>
    <td>
    <input placeholder=’Enter your username’type=’text’ name=’usernameinput’ required>
    </td>
    </tr>
    <tr>
    <td><p for=passwordinput>Password:</p></td>
    <td><input placeholder=’Enter your password’ type=’password’ name=’passwordinput’ required></td>
    </tr>
    <tr>
    <td colspan=2>
    <input style=’margin:10px; background-color:orange’ type=’submit’ value=’Click to login’ name=loginsubmit>
    </td>
    </tr>
    </table>
    </form>
    </div>
    <?php require(‘footer.php’);

    #5585
    kliin
    Guest

    Good project!
    Like what others said, it also reminds me 15 years ago in early PHP days.

    Today’s PHP is much better than using `require_once` or equivalent in every part of your code. You should try using Laravel / Symfony, learn and follow its best practices, implement PHP Linter to make your code more readable, and learn to apply CI/CD to make your development easier.

    #5586
    SVLNL
    Guest

    Consider adding foreign key constraints to your database scheme.

    #5587
    justlasse
    Guest

    Try to walk over your code a few times and see how you could refactor out functions from procedural code. Then see if you could organize these functions in separate files and lastly see if you can move functions into reusable objects such as classes etc. then once you’ve gotten to that point, look at how you may be able to use design patterns to handle certain situations like db agnostic code or provider agnostic controllers so you use instead interfaces or abstract classes. It is such a good way to learn the language to examine and scrutinize your own code and it helps you think in organization over functions in some ways

    #5588
    HenkPoley
    Guest

    Here you have a code quality robot that can inspect your public Github code: https://insphpect.com/report/5f363f4ed7e1b

    But it only inspects classes. Since you have only one there’s _nothing_ to say. It is in PHP generally recommended to use classes. Though I have my own ideas about this, when you need to use multiple files, PHP’s code without classes tends to become a mess. I would recommend using index.php as a ‘router’ and main entry point, and put all your page types in classes, where index.php will pick the correct one. Otherwise you ‘cannot know’ where users enter into your scripts from the web (well, you do know, but it’s every script on their own, all need to be defended).

    Related tools I tend to use:

    * https://github.com/vimeo/psalm – Finds bugs where you feed things the wrong kind of data. There’s a limited online version, which doesn’t work with `require()`, so only these 2 files:
    * https://psalm.dev/r/446f0704e1 – database.php
    * Are you sure you don’t want to use the database port number?
    * https://psalm.dev/r/489397734c – header.php
    * Since you are using `require()`, an old PHP way of combining software Psalm cannot find every variable you use.
    * Since you didn’t describe the shape of the variables (the ‘type’), Psalm cannot tell you if you are using things correctly.
    * You are calling `document->close_html_form()` with 2 parameters, even though you made it take no parameters.
    * https://github.com/FriendsOfPHP/PHP-CS-Fixer – Finds small old stinky PHP coding patterns and transforms them into modern simpler ways to write them. All the “dangerous” parts are behind a ‘:risky’ marking. The rest is usually pretty safe.
    * https://github.com/rectorphp/rector – Similar idea as above, but is more meant as a toolkit where you need to take care if everything it does is sane. You pick the tools, and operate them one by one. More “dangerous”.

    Personally I would stay away from frameworks such as Laravel. Since everything is dynamically attached at runtime (programmers call this ‘magic’) the above tools cannot really help you.

    #5589
    2012-09-04
    Guest

    How come new PHP programmers are still coding like it’s the early 2000s? Shouldn’t these bad habits like `require` have died years ago?

    I haven’t typed `require` for 10+ years, minus my terrible experiences at US LawShield in 2019…

    #5590
    Rikudou_Sage
    Guest

    u/KaryM711 I’ve rewritten parts of your app (basically just the login and register functionality) into a more modern OOP way and also added extensive comments into the source code: https://github.com/RikudouSage/tutorial-forum.

    You can start reading at [public/index.php](https://github.com/RikudouSage/tutorial-forum/blob/master/public/index.php) and go from there.

    #5591
    drbob4512
    Guest

    Just glanced on mobile but for starters use the correct names for files types, ie a fully html file should be named .html, you can include it and/echo it out after. When you play with the databases, making sure all is indexed on what gets searched (really only checked 4 files, will look at more tomorrow) pull just the fields you want instead of fetching the entire row, ie if you only need the name just pull that instead of getting the row via assoc then pulling the name that way. Also when you get decent with this look into using twig templating, you can do so much more so easily after you get the hang of php. Edit, was off, looks like you were doing smaller db queries, mobiles a bitch to read code on. One thing i would do is stick to single quotes for your lines, you can assign a variable for all your html too and just keep prepending to that with .= then echo out the single variable. Looking like quoting for links may have been off for tour href line on the forum.php file. With single quotes you can do proper html markup too, example $html=‘<div id=“idname” class=“classhere”></div>’;

Viewing 15 posts - 1 through 15 (of 18 total)
  • You must be logged in to reply to this topic.