6
\$\begingroup\$

Here is the class I'm using:

<?php
/**
 * Pastr 1.0
 * @author Lloyd
 * Pastr is a quick and easy tool you can use to upload snippets of text and code
 * <pastr.co>
*/

class Pastr
{
    /**
     * Our database information is stored here
     * Array
    */
    public $dbInfo = array();

    /**
     * Our array which stores the allowed highlighting types
     * Array
    */
    public $allowedHighlight = array('Plain Text', 'PHP', 'Python', 'C#', 'Ruby', 'Objective-C', 'CSS', 'JavaScript');

    /**
     * Our key which is used for encryption
     * String
    */
    const ENCRYPTION_PRIVATE_KEY = "MYSECRET";

    /**
     * Our datetime which is used when inserting
     * String
    */
    const DATE_FORMAT = "d/m/y";

    /*
     * PDO database variable
     * Instance
    */
    public $db;

    /**
     * Our location of the 'Internal server error' page
     * String
    */
    public $internalError = "frontend/error_pages/500_error.html";

    /**
     * Our root domain config, along with the title
     * Array
    */
    public $rootConfig = array('pastr.me/', 'Pastr');

    /**
     * Constructor, defines our database variables and a new instance of Pastr
     * Function
    */
    public function __construct()
    {
        $this->db = new PDO("mysql:host=localhost;dbname=Pastr", 'XXXX', 'XXXX') or die ("lmao");
    }

    /**
     * Statistic functions
     * Functions
    */

    /**
     * Fetch the total number of pastes
     * Function
    */
    public function totalPastes()
    {
        try 
        {
            $SQL = $this->db->query("SELECT COUNT(*) FROM `pastes`");
            return $SQL->fetchColumn(0);
        } catch (Exception $e) {
            error_log($e);
        }
    }

    /**
     * Content functions
     * Functions
    */

    /**
     * Fetch the title
     * $pasteID - The ID of a paste
     * Function
    */
    public function fetchTitle($pasteID)
    {
        try 
        {
            $returnTitle = "";
            global $db;
            $SQL = $this->db->prepare("SELECT * FROM `pastes` WHERE `paste_id` = :id ");
            $SQL->execute(array(':id' => $pasteID));
            while($fetch = $SQL->fetch(PDO::FETCH_ASSOC))
            {
                $returnTitle = $fetch['paste_title'];
            }
            return $returnTitle;
        } catch (Exception $e) {
            error_log($e);
            return false;
        }
    }

    /**
     * Fetch the language of the paste
     * Function
    */
    public function fetchLang($pasteID)
    {
        try 
        {
            $returnLang = "";
            global $db;
            $SQL = $this->db->prepare("SELECT * FROM `pastes` WHERE `paste_id` = :id ");
            $SQL->execute(array(':id' => $pasteID));
            while($fetch = $SQL->fetch(PDO::FETCH_ASSOC))
            {
                $returnTitle = $fetch['paste_lang'];
            }
            return $returnTitle;
        } catch (Exception $e) {
            error_log($e);
            return false;
        }
    }

    /**
     * Fetch the application side content for a paste, use nl2br() function
     * $pasteID - The ID of a paste
     * Function
    */
    public function fetchContent($pasteID)
    {
        try 
        {
            $SQL = $this->db->prepare("SELECT * FROM `pastes` WHERE `paste_id` = :id ");
            $SQL->execute(array(':id' => $pasteID));
            while($fetch = $SQL->fetch(PDO::FETCH_ASSOC))
            {
                $returnContent = $fetch['paste_content'];
            }
            return $returnContent;
        } catch (Exception $e) {
            error_log($e);
            return false;
        }
    }

    /**
     * Add a paste to the database
     * $pasteContent - The content of the paste, without nl2br.
     * $pasteLang - The language that the paste is in e.g plain text
     * $pasteID - The unique ID for the paste, for example the URL: pastr.me/erIRiuX
     * $pasteTitle - The title of the paste
     * $pastePassword - The password of the paste
     * Function
    */
    public function insertPaste($pasteID, $pasteTitle, $pasteContent, $pasteLang, $pastePassword)
    {
        try
        {
            $insertPaste = $this->db->prepare("INSERT INTO `pastes` VALUES(NULL, :paste_id, :paste_title, :paste_content, :paste_lang, :paste_date, :paste_views, :paste_password)");
            $insertPaste->execute(array(':paste_id' => $pasteID, ':paste_title' => $pasteTitle, ':paste_content' => $pasteContent, ':paste_lang' => $pasteLang, ':paste_date' => date(self::DATE_FORMAT), ':paste_views' => 0, ':paste_password' => $pastePassword));
        } catch(Exception $e) {
            error_log($e);
            echo "fail";
        }
    }

    /**
     * Check if a paste has a password attatched to it
     * $pasteID - The ID of a paste
     * Function
    */
    public function hasPassword($pasteID)
    {
        try 
        {
            $SQL = $this->db->prepare("SELECT * FROM `pastes` WHERE `paste_id` = :id ");
            $SQL->execute(array(':id' => $pasteID));
            while($fetch = $SQL->fetch(PDO::FETCH_ASSOC))
            {
                if($fetch['paste_password'] == "0")
                {
                    return false;
                } else {
                    return true;
                }
            }
        } catch (Exception $e) {
            error_log($e);
            return false;
        }
    }

    /**
     * Get the password from a protected paste
     * $pasteID - The ID of a paste
     * Function
    */
    public function fetchPassword($pasteID)
    {
        try 
        {
            $returnContent = "";
            $SQL = $this->db->prepare("SELECT * FROM `pastes` WHERE `paste_id` = :id ");
            $SQL->execute(array(':id' => $pasteID));
            while($fetch = $SQL->fetch(PDO::FETCH_ASSOC))
            {
                $returnContent = $fetch['paste_password'];
            }
            return $returnContent;
        } catch (Exception $e) {
            error_log($e);
            return false;
        }
    }

    /**
     * PasteID Generation Functions
     * Functions
    */

    /**
     * Generate random string
     * Length - 5
     * Function
    */
    public function generateRandom()
    {
        $returnString = "";
        $allowedChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
        for ($i=0; $i < 6; $i++) 
        { 
            $returnString .= $allowedChars[rand(0, (strlen($allowedChars)-1))];
        }
        return $returnString;
    }

    /**
     * Generate a random pasteid
     * Function
    */
    public function generatePasteID()
    {
        $finalString = "";
        $genRand = $this->generateRandom();
        do
        {
            $finalString = $this->generateRandom();
        } while($this->pasteExists($genRand));
        return $finalString;
    }

    /**
     * Paste exists
     * $pasteID - The ID of a paste
     * Funciton
    */
    public function pasteExists($pasteID)
    {
        $SQL = $this->db->prepare("SELECT * FROM `pastes` WHERE `paste_id` = :paste_id");
        $SQL->execute(array(':paste_id' => $pasteID));
        $result = $SQL->fetchColumn(0);
        if ($result == 0)
        {
            return false;
        } else {
            return true;
        }
    }

    /**
     * Generate a paste link
     * $pasteID - The ID of a paste
     * Function
    */
    public function generatePasteLink($pasteID)
    {
        return "https://".$this->rootConfig[0]."!/".$pasteID;
    }

    /**
     * Encryption functions for all pastes, all pastes are decrypted on load
     * Function
    */

    /**
     * Seed our random function, making it more random when using MCRYPT_RAND
     * Double
    */
    public function generateSeed()
    {
        return ((double) microtime() * 1000000);
    }

    /**
     * Encrypt the string with AES 256 & encode with BASE64
     * Function
    **/
    public function encryptString($string)
    {
        srand((double) microtime() * 1000000);
        try
        {
            return rtrim(base64_encode(mcrypt_encrypt(MCRYPT_RIJNDAEL_256, self::ENCRYPTION_PRIVATE_KEY, $string, MCRYPT_MODE_ECB, 
                    mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB),  MCRYPT_RAND))), "\0\3");
        } catch(Exception $e) {
            error_log($e);
        }
    }

    /**
     * Decrypt the string with AES 256 & encode with BASE64
     * Function
    **/
    public function decryptString($string)
    {
        srand((double) microtime() * 1000000);
        try
        {
            return rtrim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, self::ENCRYPTION_PRIVATE_KEY, base64_decode($string), MCRYPT_MODE_ECB,
                    mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256,MCRYPT_MODE_ECB), MCRYPT_RAND)), "\0\3");
        } catch(Exception $e) {
            error_log($e);
        }
    }

    /**
     * Misc functions which don't have an exact category
     * Functions
    */

    /*
     * Redirect our user using header(), and get the paste URL from the generate function
     * $pasteID - The ID of a paste
     * Function
    */
    public function pasteRedirect($pasteID)
    {
        header("Location: ".$this->generatePasteLink($pasteID));
    }

    /*
     * Enable the user to download a paste
     * $pasteID - The ID of a paste
     * Function
    */
    public function pasteDownload($pasteID)
    {
        header('Content-type: text/plain');
        header('Content-Disposition: attachment; filename="'.$this->fetchTitle($pasteID).'.txt"');
        exit($this->fetchContent($pasteID));
    }

    /*
     * Generates the content for print_paste.php
     * $pasteContent - The content of a paste (decrypted)
     * Function
    */
    public function pushContent($pasteContent)
    {
        return htmlspecialchars(stripcslashes($pasteContent));
    }

    /**
     * The functions dedicated to our highlighting feature
     * Functions
    **/

    /**
     * Translates the given user input into a CSS element we can use
     * $highInput - The input given from the Syntax bar
    */
    public function translateHigh($highInput)
    {
        switch ($highInput) 
        {
            case 'PHP':
                return "php";
                break;
            case 'Python':
                return "python";
                break;
            case 'C#':
                return "cs";
                break;
            case 'Objective-C':
                return "objectivec";
                break;
            case 'JavaScript':
                return "javascript";
                break;
            case 'CSS':
                return "css";
                break;
            case 'Ruby':
                return "ruby";
                break;
            case 'Plain Text':
                return "plain";
                break;
        }
    }

    /**
     * Checks if a language is valid or not
     * Boolean/Function
    **/
    public function highValid($type)
    {
        if(in_array($type, $this->allowedHighlight))
        {
            return true;
        } else {
            return false;
        }
    }
}
?>

Is there anything that can be changed or altered to make it more efficient? I'm no professional programmer and am under 16 years of age. I've coded this all without no help at all, so please tell me if there are any efficiency problems!

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! That's a pretty massive script, impressive to do on your own. I hope you get some good reviews! \$\endgroup\$ – Phrancis Dec 24 '14 at 16:00
  • 1
    \$\begingroup\$ I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mathieu Guindon Feb 22 '17 at 1:54
  • \$\begingroup\$ I'm locking this for now since you defaced the post after being warned. If you really think that this post needs to be removed, or you're okay with removing your name from this post, use the Contact Us form at the bottom of the page. We're otherwise reluctant about removing posts with upvoted answers. \$\endgroup\$ – Jamal Feb 22 '17 at 2:14
1
\$\begingroup\$

This isn't something huge but I would recommend using more spacing in your application to make it more readable. Also as of  you can define arrays using [] instead of array(). That really doesn't matter at all but some prefer it for a slightly cleaner syntax.

Another problem I see is that your methods have a lot of names that don't really make sense for what they do.

For example:

public function pushContent($pasteContent)
{
    return htmlspecialchars(stripcslashes($pasteContent));
}

"pushContent" doesn't really make any sense here. This method is escaping HTML entities and removing backward slashes, not pushing any content. A name like escape() might be better.

Another example is the highValid() method. A name like highValid doesn't make too much sense from just looking at it. Try something like isHighlightValid();

You should also try to follow better naming conventions in methods. For example pasteExists() should be more properly named doesPasteExists() or totalPastes() to getTotalPastes(). And instead of fetch in a lot of cases you should use get.

Long descriptive names help in the long run too. Don't be lazy and shorten strings for sake of saving a keystrokes. For example, instead of generateRandom() try generateRandomString() or instead of generatePasteID() try generateUniquePasteId() to show that the id returned is unique.

Condensing variable names in functions can also lead to confusing names. In every function involving a "highlight" you shorten it to "high." That makes no sense just looking at it as high and highlight are two different things. I would recommend changing everywhere it says "high" to "highlight."

I see you use comments which is great. A lot of developers under comment their code which is a problem. However in this case it appears you over comment your code. Some stuff is obvious and doesn't really need a comment like ENCRYPTION_PRIVATE_KEY or generatePasteLink(). Some of your comments also don't really tell you much. An example is pasteExists(). It is verbatim of the method. For defining what each variable type is seems a little overkill too. I've personally never seen it done and while I can understand why it might be useful, it's a little too much.

Some small refractoring you could is in the method highValid();. You can condense this into one line.

public function highValid($type)
{
    return in_array($type, $this->allowedHighlight);
}

I see you use no private modifiers at all. This could be a problem especially for variables like ENCRYPTION_PRIVATE_KEY that should not be allowed to be changed externally. And since I'm not sure of this class' implementation outside of the code you provided, I'm not sure if you are accessing the class variables elsewhere. If you are consider using getters and setters instead of public.

I just overviewed your style of programming. I didn't look too much at the actual functionality of your code so hopefully someone else does that for you. I hope this helped.

\$\endgroup\$
  • \$\begingroup\$ Push content was made simply for displaying the content on the page, if the user was pasting PHP code it simply wouldn't display on the page. Is there any work around for this? \$\endgroup\$ – JustLloyd Dec 31 '14 at 20:22
  • \$\begingroup\$ @JustLloyd Push content isn't displaying any content though. \$\endgroup\$ – Ryan Printup Jan 1 '15 at 20:44
  • \$\begingroup\$ It's used to show content on another page which I've not included in my post \$\endgroup\$ – JustLloyd Jan 2 '15 at 23:17
1
\$\begingroup\$
public $allowedHighlight = array('Plain Text', 'PHP', 'Python', 'C#', 'Ruby', 'Objective-C', 'CSS', 'JavaScript');

I'd store this information in the database. Particularly considering how you use it:

    switch ($highInput) 
    {
        case 'PHP':
            return "php";
            break;
        case 'Python':
            return "python";
            break;
        case 'C#':
            return "cs";
            break;
        case 'Objective-C':
            return "objectivec";
            break;
        case 'JavaScript':
            return "javascript";
            break;
        case 'CSS':
            return "css";
            break;
        case 'Ruby':
            return "ruby";
            break;
        case 'Plain Text':
            return "plain";
            break;
    }
}

You don't need those break; statements. The return is sufficient.

You use both kinds of strings for the same purpose. Since you are not using variable interpolation, I'd suggest only using single-quoted strings.

        case 'Plain Text':
            return 'plain';
        default:
            return null;

I'd add a default case. This makes it easier

A better solution might be to store both entries in a database table that maps between them. Then you can just add new rows in the database when you add a new language with highlighting.

public function __construct()
{
    $this->db = new PDO("mysql:host=localhost;dbname=Pastr", 'XXXX', 'XXXX') or die ("lmao");
}

As a rule, don't do work in the constructor. You could either pass the connection as a parameter to the constructor or do this in a later call.

    try 
    {
        $SQL = $this->db->query("SELECT COUNT(*) FROM `pastes`");
        return $SQL->fetchColumn(0);
    } catch (Exception $e) {
        error_log($e);
    }

This is an odd formatting. Most people would either

    try {

or

    }
    catch (Exception $e)
    {

I prefer the former, but both are popular. There's a third, but it's even more different.

        $SQL = $this->db->prepare("SELECT * FROM `pastes` WHERE `paste_id` = :id ");
        $SQL->execute(array(':id' => $pasteID));
        while($fetch = $SQL->fetch(PDO::FETCH_ASSOC))
        {
            $returnTitle = $fetch['paste_title'];
        }
        return $returnTitle;

As a general rule, you shouldn't SELECT * unless you know that you will always want every column in the row. In this case, you only want paste_title, so SELECT paste_title.

You currently return the last row with that ID even though you fetch all of them. I'm guessing that id is unique -- only one entry with a particular value in the table. If so or if you're just happy with returning the first value found rather than the last, it's more direct to say

public function fetchTitle($pasteID)
{
    try {
        $statement = $this->db->prepare("SELECT paste_title FROM `pastes` WHERE `paste_id` = :id ");
        $statement->execute(array(':id' => $pasteID));
        if ( $row = $statement->fetch(PDO::FETCH_ASSOC) ) {
            return $row['paste_title'];
        }
    } catch (Exception $e) {
        error_log($e);
    }

    return false;
}

I changed $SQL to $statement, as that seems to better describe it. SQL is what I'd call the input to prepare, not the output. The prepare function returns a resource handle representing the statement that SQL will create.

I also changed $fetch to $row, as that better reflects what it holds.

I removed the $returnTitle variable and took the return false; out of the catch block. This way it returns a consistent value when it does not fetch the title successfully.

        global $db;

You would only do this if you wanted to access a global variable named $db. Since you do not do that, this line is unnecessary.

An alternative would be to replace most of your fetch functions with a single one:

public function fetchPaste($pasteID)
{
    try {
        $statement = $this->db->prepare(<<<'EOSQL'
SELECT paste_title, paste_lang, paste_content, paste_password
FROM `pastes` 
WHERE `paste_id` = :id 
EOSQL
);
        $statement->execute(array(':id' => $pasteID));
        return $statement->fetch(PDO::FETCH_ASSOC);
    } catch (Exception $e) {
        error_log($e);
    }

    return false;
}

You would then have all the values at once, rather than making multiple database calls.

Note that I used a nowdoc to hold the multiline SQL string.

        $insertPaste = $this->db->prepare("INSERT INTO `pastes` VALUES(NULL, :paste_id, :paste_title, :paste_content, :paste_lang, :paste_date, :paste_views, :paste_password)");

Note that if you ever add a column to the table, this call will fail until you adjust it to include the new column. If you include the column names, then this call can still succeed so long as the column has a default value (possibly null).

        $insertPaste = $this->db->prepare("INSERT INTO `pastes` (?, id, paste_title, paste_content, paste_lang, paste_date, paste_views, paste_password) VALUES(NULL, :paste_id, :paste_title, :paste_content, :paste_lang, :paste_date, :paste_views, :paste_password)");

Replace the ? with the name of that column.

The comment

 * Length - 5

doesn't match the code

    for ($i=0; $i < 6; $i++) 

There are six characters, not five. Indexed: 0, 1, 2, 3, 4, 5.

        $returnString .= $allowedChars[rand(0, (strlen($allowedChars)-1))];

As a general rule, try to avoid calling functions multiple times with the same parameters in a loop.

    $allowedCount = strlen($allowedChars) - 1;
        $returnString .= $allowedChars[rand(0, $allowedCount)];

Initialize a variable with the value before the loop and then just use the variable in the loop.

        $finalString = $this->generateRandom();
    } while($this->pasteExists($genRand));

A more common way to do this is to have an auto-increment ID column and use a cryptographic function to obfuscate it. Then you are guaranteed uniqueness by the database and don't have to do anything as heavyweight as this.

?>

You don't need this in a file that is all PHP code, and it can lead to problems if you include it. In particular, extra whitespace after closing the PHP block can lead to being unable to redirect to a new URL or otherwise creating headers. You'll get the error, "Headers already sent." The standard is to only use this in files where PHP is embedded in HTML.

There may be more things that I missed.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.