Assist cleaning up messy HTML/PHP code

Question

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

• W.K.S: 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.
• mheinzerling: Normalizing a database is not always the best approach, so can you tell me something about the number of entries in the tables.

Answer 1

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.

• Peter Kiss: 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.
• mheinzerling: @PeterKiss Please read 'actually you could split it in a model for each entity/database table' again.
• mheinzerling: @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.
• Peter Kiss: 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.
• mheinzerling: 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 .

Answer 2

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

• mheinzerling: If you are using Value Objects please check: PDO Fetch Class otherwise use FETCH_ASSOC to skip the intermediate objects.
• Peter Kiss: 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.
• mheinzerling: 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.