HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Rewriting the rent system of a building

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thesystembuildingrentrewriting

Problem

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.

-
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).

-
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.

-
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.

```
"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 =

Solution

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

Code Snippets

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 );
}
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 );
function getPeople( PDO $conn, $json_id ) {
$dsn = "mysql:host=$host;dbname=$db";
$params = array( PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8" );
$queries = array();
$queries[ 'general' ] = //etc...

Context

StackExchange Code Review Q#17717, answer score: 2

Revisions (0)

No revisions yet.