0
\$\begingroup\$

I am working on a website, and due to some reasons, they want my code to be as short as possible. I have, from my side, tried everything to shorten this code.

So if possible, can you all also help me in making this code shorter? I know that there are vulnerabilities, and I will solve them later.

For now, just help me make it shorter. There are two files that need to be shortened.

first.php

<?php
$LocationId = (isset($_GET['l'])) ? $_GET['l'] : null;
if( $LocationId )
{ //Make connection to DB
    $DBConn = mysqli_connect("localhost","root","","url") or die("Connection Error");
//Selects the URL which mathces the ID or say shortcode
    $Resut  = mysqli_query($DBConn, sprintf("SELECT url FROM url WHERE id LIKE '%s' LIMIT 0,1", mysqli_real_escape_string($DBConn, $LocationId))) or die( mysqli_error() );
    if( $Resut )
    {   $Row = mysqli_fetch_array($Resut);
        if( isset($Row['url']) && !empty($Row['url']) ) //checks whether URL exist or not
        {        header('HTTP/1.1 301 Moved Permanently');
            header(sprintf('Location: %s', $Row['url'])); //Redirects the user to the long URL
            exit;   }
        else
        {   echo "Invalid URL"; }}} //If wrong ID given by user then returns this error
?> 

second.php

<?php
$con=mysqli_connect("localhost","root","","url") or die("Connection Error");

$url = $_POST['url'];
$sql=mysqli_query($con,"INSERT INTO url (url)VALUES('$url')");

$result = mysqli_query($con,"SELECT * FROM url WHERE url='$url'");
while($id = mysqli_fetch_array($result))
  {
  echo " URL Shortened Successfully! ".'<br>'."Your shortened URL is ";
  echo 'http://localhost/redirect/' . $id['id'];
  }

mysqli_close($con);
?> 

And one more thing: there are 6-7 lines of whose meaning I can't understand, so please explain this to me this as well. Those 6-7 lines are as follows:

 $Resut  = mysqli_query($DBConn, sprintf("SELECT url FROM url WHERE id LIKE '%s' LIMIT 0,1", mysqli_real_escape_string($DBConn, $LocationId))) or die( mysqli_error() );
    if( $Resut )
    {   $Row = mysqli_fetch_array($Resut);
        if( isset($Row['url']) && !empty($Row['url']) ) //checks whether URL exist or not
        {        header('HTTP/1.1 301 Moved Permanently');
            header(sprintf('Location: %s', $Row['url'])); //Redirects the user to the long URL
            exit;   }
\$\endgroup\$
  • \$\begingroup\$ If its any help, I open sourced my url shortener smfu.in (doesn't work as I'm in the middle of a server move). You can see the code at github.com/jsanc623/smfu \$\endgroup\$ – jsanc623 Jan 6 '14 at 14:48
  • 1
    \$\begingroup\$ Asking for explanations of other people's code is off topic here. \$\endgroup\$ – RubberDuck Dec 19 '15 at 12:27
  • \$\begingroup\$ Solve the vulnerabilities before shortening your code. It may impact how you can shorten it, may prevent certain things from being used, may add new avenues -- but most importantly, it's very, very difficult to modify extremely golfed code. \$\endgroup\$ – Fund Monica's Lawsuit Dec 25 '15 at 18:04
7
\$\begingroup\$

Brevity of code seems an odd point of focus, so I plan on ignoring that. I do, however, have quite a few suggestions, and, convieniently for you, those will probably shorten the code.


Don't assume user provided variables are actually set. For example, don't access $_POST['url'] directly. Either cover it in a isset guard like you've done other places, or use filter_input. Also, don't trust that user provided variables will be strings. They can be arrays. Might want to add a is_string check.


Your code is wide open to SQL injection. Never put an unescaped string into a query. Always either use prepared statements or properly escape strings. Prepared statements tend to be a bit harder to accidentally forget to do, so they are what I usually go with.


Use either $a=val or $a = val consistently. Inconsistent coding style is hard on the brain. I strongly suggest the $a = val style.


Your indention and line breaks are rather sporadic and odd. first.php is particularly bad. Don't include things on the same line as a brace like:

if ()
{ something here

Also, make sure you develop a habit for indention at each code block level and stick with it.


For such a simple script, or die is fine. In general though, it can be a bit of a problem since it doesn't allow for pretty error handling very easily. It's often better to throw an exception so that higher up code can catch it and gracefully render an error instead of just a plain text string. Like I said though, it doens't really apply here since nothing is being rendered anyway.


Queries should never fail in production code. This means that if( $Resut ) shouldn't be necessary. If the query is failing, something is majorly wrong as it means either an entity is missing (like a DB or table) or a syntax error happened. An entity missing is problematic since it means the DB is out of sync with the code, and a syntax error is worrying since it means that code either wasn't tested and legitimately has a syntax error or that SQL injection is occuring.


Basically all of the comments are meaningless. All they do is describe exactly what the code is doing. As long as someone can read PHP, they can already see what the code is doing. Typically comments should describe why something is being done not how. (In algorithmical code, it's fairly common to comment what is going on, but that's because it's much harder to follow than straight forward code like this.)


There's no reason to put the final closing tag in a PHP file. It can end up causing complications with trailing whitespace or non-printed characters, and it isn't syntactically required. For this reason, it's become customary lately to omit the closing tag.


This comes down to personal preference and OCD really, but I tend to avoid closing resources that last until the end of script execution. The mysql connection is about to be cleaned up anyway when PHP tears itself down, so the mysqli_close is really just superfluous. Some people like the explicitness of it though and it gives them peace of mind knowing that they closed something they opened. That's a very valid approach. There's just not technically a need for it, and I like to avoid clutter.

Note that this does not apply to resources that are not tied to the life time of the script. In general, resources should be held as briefly as reasonably possible.


empty($x) is equivalent to isset($x) && $x. This means that isset($x) && !empty($x) is the same as isset($x) && isset($x) && $x. In other words, just use !empty($x) if you want the behavior specified. The isset is redundant.


For code this simple and short, it's not really necessary, but if this grows, I would extract common setup in a bootstrapping mechanism of some sort. This would allow you to avoid having to duplicate your mysqli conneciton across every file. That means you won't have to change the DB credentials in n files if you need. Instead, you could just change them in one.


If the code grows more complex, I would extract functionality into functions. Two obvious functions that could exist now are getUrlFromId and createUrlShortening.

Functions allow your program flow to focus on program flow and not implementation.

Functions also allow for a central point of change. For example, if this grows into a complex website that has tons of pages and whatnot, you're likely going to need to be able to retrieve URLs from many pieces of code. Now imagine that you've found a bug in your URL retrieving code or you want to change a table name or something. You can either Control+F and fix it in a ton of places and hope you haven't missed anything, or you can have one centralized place to change it.


And one more thing: there are 6-7 lines of whose meaning I can't understand, so please explain this to me this as well.

That block of code queries the database for a record in the table url that has an ID equivalent to the location ID the end user provided (via the l GET parameter -- like blah.php?l=33).


WHERE id LIKE `%s` LIMIT 0, 1

This is wrong for three reasons.

  • Simple equality should be used like id = blah since there's no pattern matching needed
  • The limit is unnecessary since id is (or should be) a primary key and primary keys are unique
  • id is (or should be) an integer, not a string and thus should be treated as such. Don't escape it with mysqli_real_escape_string, and don't interpolate it in as a string. Instead, make sure it's an integer format and put it in as an integer. The easiest way to do this is simply casting the variable like " ... WHERE id = " . (int) $locationID"

If you want to be able to actually detect an invalid integer, you can use something like ctype_digit or filter_input with the proper validation flags:

$blah = (isset($_GET['blah']) && is_string($_GET['blah'])) ? $_GET['blah'] : null;
if ($blah === null) {
    echo "You must provide blah!";
} else if (!ctype_digit($blah)) {
    echo "Blah is invalid!";
} else {
    $query = "SELECT blah FROM bleh WHERE id = " . (int) $blah;
    //(The int cast is unnecessary since you already know it's an int. It can protect
    //against a change in the code flow later allowing an invalid value through though)
}

Not only do prepared statements simplify this situation a bit, they can also allow it to be a bit briefer since you don't have to worry as much about validation or careful interpolation/concatenation.


In second.php: inserting a value only to immediately retrieve it is a bit odd. Just use mysqli_insert_id($conn) instead of fetching back the id.

Also, while($id = mysqli_fetch_array($result)) doesn't make sense. Only one row should be returned since the user just created one shortening. Oddly enough, this loop can actually return more than 1 item though since I'm assuming the url column isn't unique. If this is the functionality you desire, ignore the point about not fetching back the row. That is required to bring back all of the duplicates.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ while($id = mysqli_fetch_array($result)) Yes @corbin you are right it actually sometimes return more rows, 1) Can you please tell me how can I make it in a way so that only 1 result should come out OR 2) we can either do this: When the user gives the URL then second.php should also check that whether this URL exist or not, if it exist then a error but if the URL do not exist then no error. So please edit my code as per 1st or 2nd way, cause I am actually a noob and I think i won't be able to do it without any problems! \$\endgroup\$ – user3161945 Jan 5 '14 at 10:08
  • \$\begingroup\$ I tried to make some changes myself and now it returns only one ID; I just added LIMIT 0,1 in this line $result = mysqli_query($con,"SELECT * FROM url WHERE url='$url' LIMIT 0,1"); But now the problem is this that the URL is being added in the database! Can you tell me that how can I do some changes in my code so that it should not accept the URLs that already exist in my database. Thanks. \$\endgroup\$ – user3161945 Jan 5 '14 at 10:14
  • \$\begingroup\$ @user3161945 just perform a select to retrieve the ID of an existing one. If it doesn't exist, create a new one instead. For tracking purposes in the future though, you might to actually keep duplicates. As for the technical specifics, perhaps StackOverflow could help you with that. \$\endgroup\$ – Corbin Jan 5 '14 at 11:39

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