-2

I have this method and the question is how can i break this very long line not to exceed 85 to 120 characters? For the record i did not made this code myself, it is a old opensource project that i adjusted for my needs.

function doQuery($sql)
{
    $this->counter++;
    $result = $this->unbuffered ? mysqli_query($this->db, $sql, MYSQLI_USE_RESULT) or $this->error = true and $this->errormessage = mysqli_error($this->db) : mysqli_query($this->db, $sql) or $this->error = true and $this->errormessage = mysqli_error($this->db);
    return new ResultSet($result);
    }

Here is the original method, i had to adjust it for php 7 but i may have screwed up. I am not a proffesional, pure hobby programmer.

/**
* Query the database
*
* @param    string      sql query string
* @throws   DatabaseException
* @return   ResultSet
*/
function doQuery($sql) {
    $this->counter++;
    $result = $this->unbuffered ? @mysql_unbuffered_query($sql, $this->db) or $this->error = true and $this->errormessage = mysql_error() : @mysql_query($sql, $this->db) or $this->error = true and $this->errormessage = mysql_error();
    return new ResultSet($result);
}

I adjusted the first method, thanks bill karwin.

/**
 * Query the database
 * 
 * @param string $sql sql query string.
 * 
 * @throws DatabaseException
 * 
 * @return ResultSet.
 */
function doQuery($sql)
{
    $this->counter++;

    if ($this->unbuffered) {
        $resultmode = MYSQLI_USE_RESULT;
    } else {
        $resultmode = MYSQLI_STORE_RESULT;
    }

    $result = mysqli_query($this->db, $sql, $resultmode);

    // If anything goes wrong.
    if ($result === false) {
        $this->error = true;
        $this->errormessage = mysqli_error($this->db);
    }
    return new ResultSet($result);
}
dugo666
  • 11
  • 2
  • Adopt whichever coding standard you wish. You could break it at any space or special character. – El_Vanja Dec 12 '20 at 16:41
  • i'm using pear coding standard. So if i understand right i can break it up at a space to split the long line in 4 lines for example? – dugo666 Dec 12 '20 at 16:43
  • I'm not familiar with the pear coding standard guidelines. In general, it's best to break at an operation (in your case I'd suggest at logical operators). – El_Vanja Dec 12 '20 at 17:00
  • I would get rid of the whole method. This function is rubbish and you should never use it. Please enable proper error reporting instead. [How to get the error message in MySQLi?](https://stackoverflow.com/a/22662582/1839439) – Dharman Dec 12 '20 at 17:17
  • I am not being mean, but I honestly do not understand what this code should do and I doubt that it even works. IMHO it should throw an exception or produce some really incorrect results. Maybe if you can explain the reasoning behind this method we can suggest some better alternatives. – Dharman Dec 12 '20 at 17:22
  • Well to be honest i don't understand it either, i did not made this code. it is a opensource project that i modified to my needs. it is relatively old, i upgraded the code a few times. The good news is it works for my personal use, the bad news is i don't understand parts of it like $this->unbuffered or the ? or : operators. So i understand that from a coding perspectif this doesn't make any sence, like the error reports, but that is for now not the question. – dugo666 Dec 12 '20 at 18:01

1 Answers1

0

Whoever wrote that code was being too clever for their own good. It's cruel and irresponsible to write code like that and hand it off. It works, but it's needlessly unclear.

There's a quote by Brian Kernighan that's important to keep in mind when you're a programmer:

"Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it?"

Here's a try at code that works the same but is more clear:

function doQuery($sql)
{
    $this->counter++;

    if ($this->unbuffered) {
        $resultmode = MYSQLI_USE_RESULT;
    } else {
        $resultmode = MYSQLI_STORE_RESULT;
    }

    $result = mysqli_query($this->db, $sql, $resultmode);

    if ($result === false) {
        $this->error = true;
        $this->errormessage = mysqli_error($this->db);
    }

    return new ResultSet($result);
}

EDIT: fixed the error = true line.

Of course I have not tested this code in your project. So if there's something I didn't notice about the way other code in the project works, I wouldn't know that. If you try this out, I recommend you keep a copy of your current code first, so if this doesn't work, you can restore the previous code easily.


Explaining some of the annoying cleverness the original code.

The ? : operator is like an if/then/else structure, except you can use it like an expression. In other words, you can assign the value of the expression to another variable, unlike if/then/else.

$var = (1 == 2) ? "yes" : "no";

The way this works is that the first express (1 == 2) is evaluated. If it's true, then the result is the expression after ?. If the result of the first expression is false, then the result is the expression after :.

The variable is assigned the value "no" in this example.

Frankly, although this format seems clever, the more experience I get as a programmer, the less often I like to use it. Stuffing more logic into a single expression can reduce the lines of code, but it makes the code hard to debug, hard to change, and doesn't actually make it any more speedy than if you had spelled it all out with if/then/else structures.

This part uses another trick:

mysqli_query($this->db, $sql, MYSQLI_USE_RESULT) or $this->error = true and $this->errormessage = mysqli_error($this->db)

It relies on boolean short-circuiting which means if you write an expression like A OR B then first A is evaluated, and if it's true (or something like true), then B doesn't need to be evaluated, because true OR anything would end up being true anyway.

In this case, mysqli_query() will return a result resource which is kind of "true" or at least it's not literally false. But if there's an error in the SQL query, then mysqli_query() definitely returns false (it says so in the documentation). So the stuff to the right of or in this expression will be evaluated only if mysqli_query() has an error.

The last trick is that even though = is an assignment (not == which is equality comparison), it's legitimate to use it in an expression. What this means is that the expression returns the value assigned, but the expression also has a side-effect that it has assigned values to variables.

So the code you inherited is using the ternary operator, boolean short-circuiting, and assignment side-effects inside expressions, all for the dubious goal of writing as much logic as possible in one line of code. Programmers who do this are just being show-offs, and they are revealing that they don't care about code readability or maintainability.


You also asked about this in comments below:

isset($_GET["sort"]) ? $sort = $_GET["sort"] : $sort = "name";

That also uses the ternary operator. It's the same as the following:

if (isset($_GET["sort"])) {
    $sort = $_GET["sort"];
} else {
    $sort = "name";
}

This example of the ternary operator could also be written this way:

$sort = isset($_GET["sort"]) ? $_GET["sort"] : "name";

Because the whole value of the ternary expression is either the second or third expression, you can assign the result once to a variable.

Finally, PHP introduced another shorthand notation because this kind of task is so common.

$sort = $_GET["sort"] ?: "name";

This means if there is a "sort" key in the $_GET array, then read that, but if the key is not present in the array, fall back to the value "name".

Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
  • 1
    This is something i actually can understand, thanks! this makes more sense than the above method – dugo666 Dec 12 '20 at 18:39
  • I'm glad you understood how this works, thanks again! – dugo666 Dec 12 '20 at 19:27
  • Are you interested in any explanation of the original code, or are you just as happy to forget about it? :-) – Bill Karwin Dec 12 '20 at 19:55
  • Still learning every day so yes i'm interested! is the ? and : a kind of short style for if else ? – dugo666 Dec 12 '20 at 21:09
  • Another question is this: isset($_GET["sort"]) ? $sort = $_GET["sort"] : $sort = "name"; i found this and that explains it a bit: https://stackoverflow.com/questions/1506527/how-do-i-use-the-ternary-operator-in-php-as-a-shorthand-for-if-else – dugo666 Dec 12 '20 at 21:20
  • Thank you very much for explaining this, it was even worse than i thought. I head to read it a few times to actually understand what it says. I agree i don't like short writings at all, hard to understand, hard to maintain. if else is more code but much simpler to read. – dugo666 Dec 13 '20 at 10:27
  • Yes, if they're related to the first question, go ahead. If they're new questions, please make a new post. – Bill Karwin Dec 13 '20 at 17:21
  • Another thing about all-code-on-one-line that is annoying when one is a professional programmer working in a team is that if _anything_ in the code changes, it obviously changes the one line of code that everything is implemented on. This makes it difficult to report changes using standard code-difference tools. – Bill Karwin Dec 13 '20 at 17:23