Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

After mseancole's answer

I just realized that I was using json_id for 3 of the 4 queries after reading mseancole's hint about putting them into an array. Neat!

Are there more improvements I could make here?

function query(PDO $conn, $query, $params) {
    $result = $conn->prepare($query);

    foreach ($params as $key => $value) {
        $result->bindValue(":$key", $value);
    }

    $result->execute();
    // does this seem 'correct'? This is my solution to queries
    // with more than 1 result and then using foreach() instead of while($row = ...)
    if($result->rowCount() > 1) {
        return $result->fetchAll(PDO::FETCH_ASSOC);         
    } else {
        return $result->fetch(PDO::FETCH_ASSOC);
    }
}

$conn = new PDO("mysql:host=$host;dbname=$db", $user, $pass, array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));

$queries['general'] = "SELECT people.id, people.surname, people.firstname, people.email,
                     leases.room_id,
                     DATE_FORMAT(leases.in_date, '%e %M %Y') as in_date,
                     DATE_FORMAT(leases.out_date, '%e %M %Y') as out_date,
                     leases.rent_type FROM people, leases
                     WHERE people.id = :json_id AND leases.person_id = :json_id";

// find all bills charged to tenant
$queries['owed'] = "SELECT SUM(amount) as amount FROM money_due WHERE person_id = :json_id";

// find all money paid into the system for the bills charged to tenant
$queries['paid'] = "SELECT sum(money_paid.amount) as amount FROM money_paid, money_due WHERE
                    money_paid.money_for = money_due.id AND money_due.person_id = :json_id";

foreach ($queries as $label => $query) {
    ${$label} = query($conn, $query, array("json_id" => $json_id));
}

$query = "SELECT num as room_num, wg, rent FROM rooms WHERE id = :room_id";
$room_info = query($conn, $query, array("room_id" => $general['room_id']));

// merge lease info with general info
$all_info = array_merge($general, $room_info);
$all_info['rent_type'] = $rent_types[$all_info['rent_type']];

// calculate money owed to the house
$all_info['balance'] = $paid['amount'] - $owed['amount'];

Original post:

I'm trying to improve my coding practices and I was just wondering if (I'm definitely sure there are) there are better ways of doing the following task.

I'm rewriting the rent system of the building I live in, which currently (still) uses an Access '97 database. This is the schema that I've constructed and the following code fetches the data to be used in the section just below the nav.

I just have a few questions.

  1. Am I doing too many queries? I'm pretty sure I cannot get all the information needed in just a single query (I've tried).

  2. Naming conventions, should I be using $query1, $result1 etc or $person_query, $result_query. My logic is that, I don't need the resource variable again, why not reuse it? It's not like I create a new $conn each time.

  3. Is there a simpler way of getting data, without using any framework. I'm trying to solidify my own practices first before learning CakePHP. But perhaps CakePHP will force me to do that anyway.

Thanks for reading!

Nay

<?php header('content-type: application/json; charset=utf-8');

//$_POST = json_decode(file_get_contents('php://input'));

$host = "127.0.0.1:3306";
$db = "bettenhaus";
$user = "root";
$pass = "";

$rent_types[0] = "Mieter";
$rent_types[1] = "Untermieter";
$rent_types[2] = "Zwischenmieter";

$json_id = $_GET['id'];

$conn = new PDO("mysql:host=$host;dbname=$db", $user, $pass, array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));

// get basic info
$query = "SELECT people.id, people.surname, people.firstname, people.email, leases.room_id, DATE_FORMAT(leases.in_date, '%e %M %Y') as in_date, DATE_FORMAT(leases.out_date, '%e %M %Y') as out_date, leases.rent_type FROM people, leases WHERE people.id = :json_id AND leases.person_id = :json_id";
$result = $conn->prepare($query);
$result->bindValue(":json_id", $json_id);
$result->execute();

// prepare to merge
$person_info = $result->fetch(PDO::FETCH_ASSOC);

// get lease info 
$query = "SELECT num as room_num, wg, rent FROM rooms WHERE id = :room_id";
$result = $conn->prepare($query);
$result->bindValue(":room_id", $person_info['room_id']);
$result->execute();

$room_info = $result->fetch(PDO::FETCH_ASSOC);

// merge two results
$all_info = array_merge($person_info, $room_info);
$all_info['rent_type'] = $rent_types[$all_info['rent_type']];

// get all bills charged to this tenant
$query = "SELECT SUM(amount) as debt FROM money_due WHERE person_id = :json_id";
$result = $conn->prepare($query);
$result->bindValue(":json_id", $json_id);
$result->execute();

$debt_info =  $result->fetch(PDO::FETCH_ASSOC);

// get total amount of money paid into the system by tenant
$query = "SELECT sum(money_paid.amount) as sum FROM money_paid, money_due WHERE money_paid.money_for = money_due.id AND money_due.person_id = :json_id";
$result = $conn->prepare($query);
$result->bindValue(":json_id", $json_id);
$result->execute();

$paid_info = $result->fetch(PDO::FETCH_ASSOC);

// calculate money owed to the house
$all_info['balance'] = $paid_info['sum'] - $debt_info['debt'];

$conn = null;

echo json_encode($all_info);

?>

share|improve this question

1 Answer

Functions!

Functions are what this code needs. You've already started separating your code logically, which is the first step in creating functions. Just look at your comments. You can start by making each commented section its own function. Of course, this isn't the end of it. We still have to make our PDO connection and query parameters available to these functions, and ideally we will want to make them reusable as well, but it will help us get started. I'll do the first one for you.

function getPeople( $conn, $json_id ) {
    $query = "SELECT people.id, people.surname, people.firstname, people.email, leases.room_id, DATE_FORMAT(leases.in_date, '%e %M %Y') as in_date, DATE_FORMAT(leases.out_date, '%e %M %Y') as out_date, leases.rent_type FROM people, leases WHERE people.id = :json_id AND leases.person_id = :json_id";
    $result = $conn->prepare($query);
    $result->bindValue(":json_id", $json_id);
    $result->execute();
    return $result->fetch( PDO::FETCH_ASSOC );
}

A function is a reusable script. Each function must have a unique name that should be indicative of its purpose. For instance, the above function sets up a query to get a list of people from a database and then returns the results. Thus its name tells us exactly what it is doing and it should do no more and no less. This is a principle called Single Responsibility. As the name implies, the function has one responsibility. Any additional tasks must be delegated to other functions. The two parameters, $conn and $json_id, are components that it needs to do its task. Instead of fetching these components each time, which is beyond the scope of this function's responsibilities, we inject them into our function via the parameters.

Just like PHP, MySQL and PDO do not care about whitespace. I'm still unfamiliar with the SQL syntax to properly space that out for you, but know that you can add as many newlines and whitespace as you need to make that more legible, in fact I would encourage you to.

Reusing our Functions

So, I created the first function for you. If you went ahead and made the other functions, like I suggested, then you might hate me for this, but they will be unnecessary. It was good practice though. You only need one function here. There is a general programming principle called "Don't Repeat Yourself" (DRY). As the name implies, your code should not repeat itself. There are many different ways you can keep your code DRY (variables, loops, and functions to name a few), but we are specifically going to talk about not repeating functionality right now. However, these concepts should be easily transferable to the other aspects of DRY.

How do we know if our code is DRY? If you look at all of those queries, you should begin to notice a pattern. They are all almost identical. This is a clear sign that we can apply the DRY principle. In order to do it with our functions we are going to have to abstract anything that is not exactly the same and inject those properties into our script through some other means. With functions we should do this with the parameters.

function query( $conn, $query, $params ) {
    $result = $conn->prepare( $query );

    foreach( $params AS $key => $value ) {
        $result->bindValue( ":$key", $value );
    }

    $result->execute();
    return $result->fetch(PDO::FETCH_ASSOC);
}

//usage
$params = array(
    ':json_id' => $json_id
);
query( $conn, $query, $params );

As you should be able to see from this, you will no longer need to manually fetch each query, instead you can use this function and pass in the required components. But, as I mentioned above, DRY doesn't stop here. If we were to leave it at this then already our code would be 10 times better, but we are still violating DRY by manually calling our new query function each time we need it. Remember that I mentioned DRY didn't have to be all about functions? Well, how would you handle this? I'll give you a hint, but I'm not going to write this next bit for you.

Mouseover for hint:

Try using an array and foreach loop.

Bonus

Another thing we can do to help ensure we get the right kind of parameters is to implement something called type hinting. This is a little more advanced and not something you really need to know right now, but simply put, if you add the type of variable you are expecting to get as a parameter, PHP will throw errors and stop execution if any other type is used. This is a good way to ensure our function can do what we are asking of it. For example, if we passed in a non PDO object we should not expect it to be able to use PDO methods, so we could prevent this by explicitly requiring the PDO type.

function getPeople( PDO $conn, $json_id ) {

You'll notice that only $conn has been type hinted. This is because $json_id is a scalar type and is unnecessary to type hint due to PHP's loose typing. If that variable happened to be an int and you needed a string you could just treat it as a string and it would work. And vice-versa. Sometimes this is undesirable and you will have to explicitly check for these types, but you can not do this with typehinting. Instead you would have to use one of the is_*() functions. Replace the wildcard with the type of variable you are looking for (int, string, etc...). You don't really need to understand this right now, but it is a good thing to keep in mind.

Another, much more advanced topic, is OOP. Though I tell this to everyone, get proficient with functions and the concepts I mentioned above before trying to tackle OOP. Once you are fluent with functions, then you should take a look at classes and different OOP strategies.

UPDATE

Sorry for the delay in my answer. I'm typically unavailable over the weekend. This is much better, you still have a minor violation of DRY with that second query(), but overall it is much much better. Let's look at a couple things I missed last time and something new you are doing this time.

Connection

This is something I forgot to mention last time, but you should also abstract all of your connection information so that you can set this up to come from anywhere. Remove the following from your PDO instantiation and replace them with variables.

$dsn = "mysql:host=$host;dbname=$db";
$params = array( PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8" );

The benefit of doing it like this is that you can now abstract all of your connection information to a config file or to any other source to allow for easier extension. The config file would be my suggestion for now as it is fairly simple to implement while still being a pretty powerful tool. I wont go into the specifics on how to implement one here, but a quick Google search should yield immediate results. You can also set up a PHP file with constants to serve as a config file.

Arrays

Another thing I missed was your array. First the $rent_types and now the $queries. Always define your array initially before pushing anything onto it.

$queries = array();
$queries[ 'general' ] = //etc...

Of course, then the better way to do what you are trying to do is to just do it all at once. For instance.

$queries = array(
    'general' => /*general SQL*/,
    'owed'    => /etc...
);

The reason we do this is to avoid accidentally manipulating a preexisting array or string. With an array it probably wont be as noticeable, unless you end up needing that array again elsewhere, but the string should become immediately apparent. Strings can be accessed with array syntax to easier allow for specific character accessing. However, I believe trying to do so with an associative key would cause errors. Likewise, trying to later access a string's contents like an array would yield unexpected results, such as a single letter or NULL. Even if you are 100% sure that you aren't going to have any conflicts, just in case you ever need to extend this later, it is always better to explicitly define it. Besides, you are probably getting a warning initially about how that array is undefined. Make sure your error reporting is turned up so that you can catch these kinds of things.

Variable-Variables

${$label}
//or
$$label

What you are doing here is called variable-variables. For the most part variable-variables, or variable-functions, should never be used. Assume they are, 99% of the time, a bad idea. There are exceptions (MVC Controllers rendering variables for Views and Factories dynamically accessing unknown functions are pretty common), but this is not one of them. The reason variable-variables are so disliked is because of how easy they are to miss, and thus debug. There is also a security concern, because the information you are typically converting to variables is from userland and thus untrustworthy.

For instance, imagine this scenario: The very first thing you did was load your config file for your database connection, but, for some reason, you had to load variable-variables into scope before connecting. It could have been an accident, or a malicious user, but now your connection variables have been overwritten and break the system, or point to another database altogether. The problems just become bleaker and can get down right ugly.

So, instead of using variable-variables, or variable-functions, what we are going to want to do is either do something with the information immediately, thus creating a temporary generic variable, or do the same thing you are currently doing, only with an array.

${$label} = //etc...
//becomes
$query_results[ $label ] = //etc...

As you can see, this is almost identical, except we are using a more verbose array instead. There is less security concern and ambiguity here. For instance, now if we have a key that conflicts with a variable in the current scope it wont interact with it in any way. It has been abstracted, or perhaps namespaced, and thus is safer. It also becomes easier to debug. Instead of having variable-variables that could easily hide among the current scope variables, all we have to do is var_dump() the array to peek at its contents. Everything is self-contained.

share|improve this answer
Hi, thanks very much for your help. It helped me a great deal. I'm not sure how to reply with code, it says it's too long. I guess I have to 'answer' my own question? *Actually, it told me to re-edit my original question. – sanlikestabbies Oct 19 '12 at 21:19
@sanlikestabbies: Yes, typically you will want to edit your question with any updates. Always append or prepend your new content, as you have done, and never delete the original to avoid confusing the context. Sometimes small bits of code can be included in comments by wrapping it in back ticks(tilde key/that squiggly line next to the 1). I have updated my answer to address your other questions, sorry for the delay. – mseancole Oct 22 '12 at 14:47

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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