macg-gh / madlibs

A little game that runs on a LAMP stack
0 stars 0 forks source link

feedback #47

Closed alawton closed 4 years ago

alawton commented 4 years ago

initial feedback, might have more to go as I dig deeper.

  1. Organization. I don't know where to start looking here, no index.php and files aren't describing enough to know what they do.
    • Add some folder structure
      • The db stuff put in like utils or dbutils or something to keep that separate.
    • add an index.php to whichever file is supposed to be the main entry (madlibs.php?)
  2. dbcreate.sql
    • set defaults for rank & note, make not null null since it never can be
      • update checkphrase.php entry insert to just insert phrase
        • i.e. INSERT INTO entry(phrase) values($phrase)
    • extra points if you make a schema migration tool like Phinx, or use a framework like Doctrine or Laravel. But not really important to do that
  3. Separation of concerns
    • Separate out your controller code from view code. A few ways to do that,
      • most points for using a templating framework like https://github.com/bobthecow/mustache.php
        • This will also make some of the HTML generation a lot less klunky.
      • But at the least create some classes and put logic in there, and reference that in the html renderers.
  4. Prepared queries: https://www.php.net/manual/en/mysqli.quickstart.prepared-statements.php. Use that instead of escaping strings, the prepped statement will do that for you so it's a safer way to execute sql.
  5. Classes. So a lot of logic is spread out throughout the files here, so it's tough to understand what everything is doing.

I'll make an example of createtemplate.

./lib/madlib.php:

<?
public class Madlib()
{

    public __construct()
    {

    }

    public GenerateWords($number_of_words)
    {
        $output = [];
        if($number_of_words == 0 || $number_of_words > 20)
        {
            throw new Exception("hey too many words");
        }

        for($i = 0; $i < $number_of_words; $i++)
        {
            $output[] = chr(rand(65,90));
        }

        return $output;
    }
}
?>

./create_template.php

<?
    require('./lib/madlib.php');

    $madlib = new Madlib();

    try
    {
        $words = $madlib->GenerateWords($_POST['howmanywords']);
    }
    catch(Exception $e)
    {
        // do something about errors.
    }
?>
<!DOCTYPE html>
<html>
<style>
    body
    {
        background-color:#6699ff;
    }
</style>
<body>
    You have selected this many words: <?= $_POST['howmanywords'] ?>
    <br/>
    <form method="post" action="checkphrase.php">
        <?
            foreach($words AS $i => $word)
            {
                echo "\n    <br>";
                echo $word;
                echo " :&nbsp;&nbsp;<textarea name=\"$word"."_"."$i\"></textarea>";
                echo "\n    <br>";
                echo "\n    <br>";
            }
        ?>
        Fill in the phrase!
        <br/><br/>
        <input type="submit" value="Submit Phrase."/> 
    </form>
</form>
    <br>
    <br>
    <a href="dumpdb.php">Dump db</a>
</body>
</html>
  1. Style and shit

    • I'm not doing to go too much into this, but using an existing framework for styling makes it easier to generate something quick and something that looks good. https://getbootstrap.com/ as example, basically look up CSS frameworks for examples.
  2. PHP framework: lookup and try to use something like Laravel, CodeIgniter, or Symfony to build your php app on, you can get some of that templating/MVC separation I was talking about in #3.

  3. Consider using PDO instead of mysqli. mysqli is fine but PDO is compatible with other DBs, and you're not doing anything mysql specific here so it's a good win to have the ability to not care about which DB you're using.

  4. not sure if this is applicable, but i see dumpdb.php and want to make sure this isn't the only way you're able to interrogate your db. you can use a client to make administration a bit easier, like mysql workbench or https://dbeaver.io/, not sure what else is around but a decent client that allows you to view/query db directly is easier than creating php code to do that shit

  5. no need for $i = ''; in createtemplate.php, at least when you're putting work in a class/function. Even as-is you don't need it.

macg-gh commented 4 years ago

Created 10 separate issues for this so I can track them more more finely, so closing this one. Thank you for the feedback!!