4
\$\begingroup\$

I am working on a Laravel 5.1 website where I am counting the average rent of apartments in a specific area. Besides of that I want to get the average rent by amount of rooms in the apartment. The functionality itself isn't anything that hard, but then I want to be able to get the average rent for apartments with e.g. two rooms by a variable.

How would you be able to do this in a cleaner way? I am going to have attributes for up to about 10 rooms, that would mean 20 new methods. Have to be a better way to do this?

public function average_rent($rooms, $timespan = '6 months', $fromDate = null)
{
    if ($fromDate === null) $fromDate = date('Y-m-d');

    $query = DB::select(
        "SELECT COUNT(id) as amt, SUM(rent) AS total_rent
          FROM " . env('DB_PREFIX', '') . "apartments
          WHERE
           rooms = " . $rooms . " AND rent > 0 AND
           formatted_address LIKE '% " . $this->query_params . ", Sverige%' AND
           created_at BETWEEN '" . date('Y-m-d', strtotime('-' . $timespan)) . "' AND '" . $fromDate . "'");

    $avgRent = null;
    $avgAmount = $query[0]->amt;
    if ($avgAmount >= 10) $avgRent = round($query[0]->total_rent / $avgAmount, 0) . ' kr';

    return [$avgRent, $avgAmount];
}

public function getOneRoomAvgRentAttribute()
{
    return $this->average_rent(1)[0];
}

public function getTwoRoomAvgRentAttribute()
{
    return $this->average_rent(2)[0];
}

public function getOneRoomAmtAttribute()
{
    return $this->average_rent(1)[1];
}

public function getTwoRoomAmtAttribute()
{
    return $this->average_rent(2)[1];
}
\$\endgroup\$
1
\$\begingroup\$

I am going to have attributes for up to about 10 rooms, that would mean 20 new methods. Have to be a better way to do this?

The only reason you have all these methods in the first place is because it's unclear how to use the average_rent method. If you would just document it properly, you can remove all the other methods that don't actually contain any logic themselves.

I'm unsure what the difference between amount and rent is, but it may make sense to have two methods: getAmount($roomCount) and getAverageRent($roomCount). If both are needed, you could also just query once and save the result in private fields. What you definitely do not want is what you have now, where you add a method for each amount of rooms you expect. That's what method arguments are for.

Security

You are vulnerable to SQL injection. Never put variables into a query directly, always use prepared statements.

Naming

  • don't shorten variable names, it makes code harder to read. amt should be amount, etc.
  • getOneRoomAmtAttribute is really hard to read. Someone not familiar with your code will have no idea what it will do. It also draws too much focus to irrelevant words (eg attribute), and less focus to the important parts (amt). Something like getAmount($roomCount) would be better.
  • it's hard to understand the difference between avgAmount and avgRent from their names. Average amount of what? Money? What's the difference to rent then?
  • either always use snake_case or always camelCase, but don't mix them.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for a great answer! I am definatly going to move the query variables to the second argument to make it safe. Regarding the amount, that is because I want to show on how many apartments this average rent is counted on. Makes sense to split the method into two separate methods. This is methods inside an eloquent method where you expose values through a method like getSomeAttribute and then are able to access it by $town->some. In this case I want to be able to display both $town->one_room_avg_rent and $town->two_room_avg_rent, that's the main question how to do so without 20 methods. \$\endgroup\$ – Jimmie Johansson Feb 13 '16 at 10:07
  • \$\begingroup\$ @JimmieJohansson It's still not clear to me why you would want $town->one_room_avg_rent, $town->two_room_avg_rent, ... instead of $town->avg_rent($roomCount). Maybe I'm missing some context, but the second approach is definitely the correct one, because it avoids having 20 methods. \$\endgroup\$ – tim Feb 13 '16 at 10:27
  • \$\begingroup\$ Haha holy crap, how could I not see that?! Thank you, that has to be the best solution, thanks again! \$\endgroup\$ – Jimmie Johansson Feb 13 '16 at 10:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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