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.

I really hate messy code, but I seem to be rolling in it. I just want to see if anyone has some helpful advice on how to tidy this up.

<table>
<tbody>
<tr>
<th>Zona</th>
<th>Baptismos Mulheres</th>
<th>Conf. Mulheres</th>
<th>Batismos Homems</th>
<th>Conf. Homems</th>
</tr>
<?php
$currWeek = $CONF->getSetting('current_week');
$zones = $PDO->query("SELECT * FROM zones WHERE active = 1");
foreach($zones as $zone) {
$zoneName = $zone['name'];
$zoneUid = $zone['uid'];
?>
<tr>
<td><?php print $zoneName; ?></td>
</tr>                   
<?php

$areas = $PDO->query("SELECT * FROM areas WHERE zone = '".$zoneUid."'");
foreach($areas as $area) {
$areaName = $area['name'];
$areaUid = $area['uid'];
?>
<tr>
<td><?php print $areaName; ?>

<?php


$missionaries = $PDO->query("SELECT missionary FROM missionary_areas WHERE semana = '".$currWeek."' AND area_uid = '".$areaUid."'");

$compArr = array();

foreach($missionaries as $missionary) {
$companions = $PDO->query("SELECT CONCAT(title, ' ', mission_name) as name from missionarios WHERE mid = '".$missionary['missionary']."'");


foreach($companions as $comp) {
$compArr[] = $comp['name'];
}

}

$compList = $CONF->commaSeparate($compArr);
?>
<?php print $compList; ?></td>
<td></td>
<td></td>
<td></td>
<td></td>
<?php
}
}

?>

I am still a bit inexperienced and would like some helpful tips. My MySQL directory stucture is necessaraly crazy so I have to do a bunch of queries to get some simple data. Using PDO is helping quite a bit though.

Edit:

Database structure

Here is a photo showing the DataBase structure

share|improve this question
I'm not familiar with PDO but can't you divide your PHP code into functions and put them in one file. You can then call them as necessary. It would also be a good idea to use templates or a templating framework. – W.K.S Apr 22 at 5:34
Normalizing a database is not always the best approach, so can you tell me something about the number of entries in the tables. – mnhg Apr 22 at 12:55

2 Answers

WKS is right, first of all you should split your layout from your logic and your logic from your database query. (see Model-View-Controller-Pattern)

Lets start with your model (Model.php): (actually you could split it in a model for each entity/database table)

<?php
class Model()
{
     public function __construct(PDO $connection)
     {
         $this->connection=connection;
     }

     public function getZones($week)  //find a nicer name
     {
         //... see Peters post for a sample join
         // prepare multi dimensional array as needed in your template
         return $zones;
     }
}

It is in general a bad idea to run SQL queries in a loop, so try to create one join for all your queries.

Now the simple Controller (index.php)

<?php
require 'Model.php';
$connection=new PDO(...);
$model=new Model($connection);
$currentWeek = ...;
$zones= $model->getZones($currentWeek);

include "view.php"

That's it! Nothing fancy. Now the view:

<table>
    <tr>
         <th>Zona</th>
         <th>Baptismos Mulheres</th>
         <th>Conf. Mulheres</th>
         <th>Batismos Homems</th>
         <th>Conf. Homems</th>
    </tr>
    <?php foreach($zones as $zone):?>
    <tr>
         <td><?php echo $zone['name']; ?></td>
         <td colspan="4"></td>
    </tr>                   
        <?php foreach($zones['areas'] as $area):?>
        <tr>
            <td>
                 <?php $area['name']; ?> <?php echo $area['companions']; ?>
            </td>
            <td colspan="4"></td>
        </tr>
        <?php endforeach?>
    <?php endforeach?>
</table>

Don't use all-capital-letter variable names. And don't use global variables. Both is considered bad practice.

share|improve this answer
You Model class is a God object and has a hard dpeendency on a SQL storage which is also a bad practise and this way the OP won't be closer to an MVC approach. – Peter Kiss Apr 22 at 7:37
1  
@PeterKiss Please read 'actually you could split it in a model for each entity/database table' again. – mnhg Apr 22 at 7:48
@PeterKiss "has a hard dpeendency on a SQL storage". I don't get it? Do you would use a Singleton or a kind of Dependency Injection? My solution is the first step away from the global $PDO. – mnhg Apr 22 at 7:52
What happens with your code if you want to unit test it? How can you fake the PDO? You can't. And yes we can split again the code into new classes but none of them should no anything about their storage only an abstraction. In a domain model we should not have anything related to a datastorage like PDOStatement and others. – Peter Kiss Apr 22 at 7:58
If you want to test the model/repository you need the real database anyway, otherwise your test is useless. If you want to test the controller, inject an other model. In general you are right, but you are totally missing your audience here. Sun-tzus script will not evolve from a one-file-solution to a full featured business application in one stackexchange questions. My intermediate step meet his requirement and is not to complex or over engineered . – mnhg Apr 22 at 8:05
show 3 more comments

First things first

Create two model classes:

<?php

class Zone {
    public $Name;
    public $ID;
    public $Areas = array();

    public function __construct($id = NULL, $name = NULL) {
        $this->Name = $name;
        $this->ID = $id;
    }
}

class Area {
    public $Name;
    public $ID;
    public $Companions = array();

    public function __construct($id = NULL, $name = NULL) {
        $this->Name = $name;
        $this->ID = $id;
    }
}

Then you have to clear your SQL statements becouse you are running to much query against the database while you can fatch all data with only 2 query.

SQL

First the companions:

<?php

$companionsResult = $PDO->query("SELECT CONCAT(title, ' ', mission_name) as MissName, area.uid AS AreaID
FROM missionarios
INNER JOIN missionary_areas ON missionary_areas.missionary = missionarios.mid
AND semana = '".$CONF->getSetting('current_week')."' AND area.uid IN (
    SELECT areas.uid AS AreaID
    FROM zones
    INNER JOIN areas ON areas.zone = zones.uid
    WHERE zones.active = 1
)'";

$companions = array();
foreach ($companionsResult->fetchAll(PDO::FETCH_CLASS) as $c) {
    if (!isset($companions[$c->AreaID])) {
        $companions[$c->AreaID] = array();
    }

    $companions[$c->AreaID][] = $c->MissName;
}

Zones and areas with merging areas with companions:

$zonesResult = $PDO->query(
                "SELECT zones.name AS ZoneName, zones.uid AS ZoneID, areas.name AS AreaName, areas.uid AS AreaID
                FROM zones
                LEFT JOIN areas ON areas.zone = zones.uid
                WHERE zones.active = 1");

$zones = array();

foreach($zonesResult->fetchAll(PDO::FETCH_CLASS) as $zoneres) {
    if (!isset($zones[$zoneres->ZoneID])) {
        $zones[$zoneres->ZoneID] = new Zone($zoneres->ZoneID, $zoneres->ZoneName);
    }

    $area = new Area($zoneres->AreaID, $zoneres->AreaName);

    if (isset($companions[$area->ID])) {
        $area->Companions = $companions[$area->ID];
    }

    $zones[$zoneres->ZoneID]->Areas[] = $area;
}

Result

var_dump($zones);

HTML

After viewing the results you can create your own view to display the date without having queries and other not view related stuff between your HTML stuff.

(I'm sure that i have made some typos in the code but it should work after some fix.)

share|improve this answer
If you are using Value Objects please check: PDO Fetch Class otherwise use FETCH_ASSOC to skip the intermediate objects. – mnhg Apr 22 at 9:12
Great suggestion but in the example above i need to use intermediate objects becouse the query result classes (and their "properties") arn't matching any other class. – Peter Kiss Apr 22 at 9:20
You are right, missed to scroll to get the whole join :) But than I would prefer fetch assoc. I read a benchmark somewhere, maybe I will find it again. – mnhg Apr 22 at 9:26

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.