5
\$\begingroup\$

This URL shortener is just for my own use and it runs on my local machine. I created this for fun and I doubt if I would ever use it in production. Is my script any good?

shortener.php

<?php
require_once 'lib/db.php';

function random_string($length, $charset = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789') {
    $chars_length = strlen($charset) - 1;
    $str = '';

    for ($i = 0; $i < $length; ++$i) {
        $str .= $charset[random_int(0, $chars_length)];
    }

    return $str;
}


$url = $error = '';

if (isset($_GET['id'])) {
    $id = trim($_GET['id']);

    $stmt = $pdo->prepare('SELECT url FROM url WHERE id = ?');
    $stmt->execute([$id]);
    $url = $stmt->fetchColumn();

    header('Location: ' . ($url ?: "http://{$_SERVER['HTTP_HOST']}{$_SERVER['SCRIPT_NAME']}"));
    exit;
}
else {
    if ($_SERVER['REQUEST_METHOD'] === 'POST') {
        $url = $_POST['url'];

        if (filter_var($url, FILTER_VALIDATE_URL) !== FALSE) {
            do {
                $id = random_string(5);

                $stmt = $pdo->prepare('SELECT COUNT(*) FROM url WHERE id = ?');
                $stmt->execute([$id]);
                $count = $stmt->fetchColumn();
            }
            while ($count > 0);

            $stmt = $pdo->prepare('INSERT INTO url (id, url) VALUES (?, ?)');
            $stmt->execute([$id, $url]);

            header("Location: http://{$_SERVER['HTTP_HOST']}{$_SERVER['SCRIPT_NAME']}");
            exit;
        }
        else {
            $error = 'Not a valid URL';
        }
    }

    $last_id = $pdo->query('SELECT id FROM url ORDER BY created DESC LIMIT 1')->fetchColumn();
}
?><!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8">
    <title>Extremely Simple Short URL Generator</title>
</head>
<body>
    <form method="post">
        <input type="text" name="url" placeholder="URL here" value="<?= htmlspecialchars($url) ?>">
        <button type="submit">Submit</button>
    </form>
    <?php if ($last_id): ?><p><a href="<?= "{$_SERVER['SCRIPT_NAME']}?", http_build_query(['id' => $last_id]) ?>">Latest Link</a></p><?php endif ?>
    <?php if ($error): ?><p style="color: red"><?= $error ?></p><?php endif ?>
</body>
</html>

I'm aware that separating the view from the script is better, but for a simple app like this, I don't think it's necessary.


db.php

<?php
$pdo = new PDO(
    'mysql:host=host;dbname=dbname;charset=utf8mb4',
    'username',
    'password',
    [
        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
        PDO::ATTR_EMULATE_PREPARES => FALSE,
    ]
);

Any improvement is welcome.

\$\endgroup\$
2
  • \$\begingroup\$ What is the implementation of random_int()? I could guess that it is similar to random_string() or might just utilize rand()... \$\endgroup\$ Mar 19, 2018 at 18:30
  • \$\begingroup\$ It's random_int() that was added since PHP 7. \$\endgroup\$
    – Zylvan
    Mar 20, 2018 at 23:44

1 Answer 1

1
\$\begingroup\$

Feedback

The script looks pretty good. For a small script that runs on your local machine it appears to suffice for your needs.

Is the goal to only show the latest link? Or would there be a use for showing previous links?

Suggestions

Variable naming

This variable naming might be misleading:

$chars_length = strlen($charset) - 1;

Because the value is one less than the length of the string. If I was working with that code, I would ask you to rename it to something more appropriate, like:

$max_index = strlen($charset) - 1;

since that value is ultimately used to determine the maximum index used to get a character out of the string.

Use uuid() or uuid_short()

Bearing in mind that you might likely need to alter the length of the id column, instead of generating your own random string, you could consider using MySQL's uuid() or uuid_short() function. That might eliminate the need to repeat the loop of calling random_string() and running a query just to see if the return value isn't used by any records.

One other approach to a really short URL would be to just have an auto-increment integer field starting at 1... up until there are 9,999 records, the id would have 1-4 digits...

Unspecified parameter $charset

$charset doesn't appear to be passed by the one place that the function is called. Unless you plan to utilize that, the default value could be made a constant:

define('CHARSET', 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789');

function random_string($length) {
    $chars_length = strlen(CHARSET) - 1;

for vs foreach

I thought about suggesting you use foreach(range(0, $length) but after reading posts like this it might not be wise to get into a habit of that, in case you work on code that deals with large amounts of data.

If you were more used to using foreach, range() can allow you to use foreach like a for statement:

foreach(range(1, $length) as $i) {
    $str .= $charset[random_int(0, $chars_length)];
}
\$\endgroup\$
4
  • \$\begingroup\$ Thanks a lot for the review. I'll probably do more than just showing the last link. For $chars_length, I'll change it as the suggested name seems to fit the purpose more. The current implementation can generate around 916,132,832 links, so I don't think I'll need to alter the length in any time soon, especially from a script running on my own machine. :P I'll definitely read about the UUID functions, because running the script without looping sounds good. I don't feel like enabling people enumerate through the integer ID though... there are also less possible links generated. I declared ... \$\endgroup\$
    – Zylvan
    Mar 23, 2018 at 18:01
  • \$\begingroup\$ ... $charset in the function arg just because somehow I don't really like seeing it be put inside the function... I don't see myself utilize it either. I knew I should've declared it inside the function... I know about the range() tip though, and I don't use it as much as for (++$i), but it does read nicer. Again, thank you for the review. \$\endgroup\$
    – Zylvan
    Mar 23, 2018 at 18:01
  • \$\begingroup\$ Anyways, about define(), an article I read (I forget the source) says that it isn't collected by the garbage collector (is this the correct term?), but won't PHP automatically do that to everything after a script finished executing anyway? Would it affect performance even at the slightest? Is it even something worth thinking about? Also, although very unlikely, if there are only, say, 100 remaining unused links, the loop will be executed a lot of times, correct? \$\endgroup\$
    – Zylvan
    Mar 23, 2018 at 18:02
  • \$\begingroup\$ Yeah the memory should be cleaned up at script termination; for a script this small the performance differences should be negligible... Perhaps it isn't really worth thinking about it. If there are ~100 existing links then I doubt the while loop would execute more than 1-2 times... \$\endgroup\$ Apr 12, 2018 at 17:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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