Managing "Jobs" data in a database

Question

I've been working in a small page with some easy CRUD. The data I get from the database is getting shown on the page. But the way how I show the data feels a bit off. I guess it can be done in a much cleaner way, and really want to know how.

I do it as follows:

First: I require the necessary class, and call the function the the HTML file:

<?php
require 'classes/vacatureData.php';
$db = new database();
$db->listAllVacatures();
require 'templates/head.php';
require 'templates/vacatureOverzicht.php';
require 'templates/footer.php';

In my class I got a public variable public $listAllVacatures;, which I use to store all the values which I retrieve from the database.

The method to retrieve the data from the database looks as following:

public function listAllVacatures() {
 $sql = 'SELECT * FROM vacatures';
 $stmt = $this->db->prepare($sql);
 if($stmt->execute()) {
 $vacatures = $stmt->fetchAll();
 foreach($vacatures as $vacature){
 $this->listAllVacatures .= "<p>";
 $this->listAllVacatures .= "<a href='vacature.php?id=$vacature[0]'>$vacature[1]</a>";
 $this->listAllVacatures .= " - <a href='delete.php?id=$vacature[0]'>delete</a>";
 $this->listAllVacatures .= " - <a href=''>Edit</a>";
 $this->listAllVacatures .= "</p>";
 }
 } else {
 $e = $stmt->errorInfo();
 echo $e[2];
 }
 $this->db = null;
}

As you can see I use a foreach to fill $listAllVacatures with data, but also some HTML (Which i feel shouldn't be there, but right now I'm not quite sure how to organise this properly).

Then I got the template file which uses $listAllVacatures:

<div class="container">
 <p><h3>Vacatures:</h3></p>
 <?php echo $db->listAllVacatures ?>
</div>

This all works fine, but like I said, it doesn't feel right to do it this way. But for now it's the best way I know.

Answer 1

I see you want to apply Separation of Concerns by separating business logic from the View's.

The database::listAllVacatures() method should return the $vacatures array. This way you can move the foreach() (which prepares the HTML output) from inside the method into the template like so:

<div class="container">
 <p><h3>Vacatures:</h3></p>
<?php foreach ($db->listAllVacatures() as $vacature): ?>
 <p>
 <a href="vacature.php?id=<?= $vacature[0] ?>"><?= $vacature[1] ?></a>
 - <a href="delete.php?id<?= $vacature[0] ?>">delete</a>
 - <a href="#">Edit</a>
 </p>
<?php endforeach; ?>
</div>

---

Additional Information

• It would be best to follow a naming convention that uses one language. It only makes everything harder to understand when you have to read code that is written in multiple languages (like English and Dutch). I'd say stick to the same language of the programming language you're using. So listAllVacatures() method would now actually be renamed something like findAllJobs().

• Your database class isn't actually what the name implies it to be. It's actually a class which responsibility is (or should be) to retrieve all job data from the database. You might wanna call it JobFetcher or something like that (note the CamelCase naming convention for classes in PHP).

• I see that you're instantiating your database class like $db = new database();. No dependencies are injected. In the database::listAllVacatures() you use $this->db to communicate with the database. Which means you have your database class tightly coupled with PDO.

  What if you need to update the credentials to connect to the database? You will have to change your class, which is a bad thing. I recommend you to utilize Dependency Injection to get rid of this problem, so you can inject the PDO instance the class needs to operate. It introduces many benefits.

---

Since you showed interest in wanting to separate your concerns I would advise you to take a look at the MVC pattern that does just what you want: It separates business logic from the View's.

Also try to adhere to the widely known coding standards defined by the PHP Framework Interop Group. It improves the code's readability, especially to other programmers who follow these standards as well. Of course, you do not have to be perfect with it.

• Kevinvhengst: Thanks a lot! This was very helpful. I constructed the Database class with the PDO connection. I thought, every time i need a method from this class, the method requires a connection to the database.
• Kid Diamond: @SupSonツ You're welcome. :)