current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

Please keep in mind I am new and still learning when reading the following.

What I am doing

I have the following code which pulls a sport, tournament and round NR, from a DB table called event where the event is still active.

The Problem

The code works and does what I want it to do, but my problem is looking at the code makes me sick, I know there are more efficient ways to achieve what I am trying to do but I am unsure of where to start to improve the code below, I would like to move to a more object orientated or at least a more efficient way of coding.

I would appreciate it if one of the more experienced members of the community could give the code a look and provide some pointers.

enter image description here

$date = date('Y-m-d');
//get sport & tournament
$sql = "Select distinct sport, tournament, round FROM event WHERE date > $date AND active = 'y'";
$result = mysqli_query($conn,$sql) or die(mysqli_error($conn)); 
while($row = mysqli_fetch_array($result)){
    $sport[] = $row['sport'];
    $tournament[] = $row['tournament'];
    $round[] = $row['round'];
}
//make form with tournament & sport
?>
<form name="select" name="sport" method="post">
<!--GET SPORT ON SELECT  -->
<select name="sport">
<?php
//get sport
foreach($sport as $index => $sportCode){
    echo '<option value="'. $sport[$index].'">'.$sport[$index].'</option>'; 
}
?>
</select>
<!--GET TOURNAMENT ON SELECT  -->
<select name="tournament">
<?php
//get tournament
foreach($tournament as $index => $tournamentCode){
    echo '<option value="'. $tournament[$index].'">'.$tournament[$index].'</option>'; 
}
//get round
?>
</select>
<select name="round">
<?php
foreach($round as $index => $roundNr){
    echo '<option value="'. $round[$index].'">'.$round[$index].'</option>'; 
}
//get round
?>
</select>
</form>
share|improve this question
    
Correct me if I'm wrong, but what you have here is actually PHP embedded in HTML, right? – pacmaninbw Jul 15 at 14:22
    
That is correct it is php embedded in html, not very pretty.,, – Timothy Coetzee Jul 15 at 14:46
    
You will probably generate more interest and get more answers if you indicate what the webpage shows in the title, something like Sports Stats. – pacmaninbw Jul 15 at 15:00
    
You suggesting I should change the post title? – Timothy Coetzee Jul 15 at 15:02
    
Yes, make it clear what the page is supposed to show. – pacmaninbw Jul 15 at 15:02
up vote 3 down vote accepted

TL;DR: Read about MVC and understand the basics and then learn laravel

This is not about object oriented approach but more like a separation of concerns thing. You have two concerns for the moment:

  1. You're collecting some data
  2. You're displaying it

so what you have to do is to separate them, start by using a template system like twig or smarty. Collect the data in some php and pass it to the template file and render it so the html code and php code will be separated.

The template will be responsible of "how" you display the data and your php file will be responsible of "preparing the data" for the template.

Second step is to separate the code that is "dealing with the data" from the code that is "setting up the template". You can move the data related code to some class so that you'll have smt. like this in the end:

<?php 

// Require classes, setup things etc.

$obj = new Events;
$events = $obj->getAllEvents();

echo $tpl->render('events/index.tpl', array('events' => $events));

Third step may be about the // Require classes, setup things etc. part in the example above. You can use a front-controller to deal with that...

... and the list goes on...

After some struggling you'll start to realize that you need some helper classes to deal with the http requests, responses, sessions etc.

...and you'll need more and more "concerns" to "separate" in the future and in the end you'll understand why people are using mature frameworks these days.

I was writing like this in 1997-1998 (no kidding) because I had to, but you don't :)

share|improve this answer
1  
thank you so much for the GREAT response, I will definitely read up on all your suggestions - I have one concern though information overload!! but I guess you have to take it one step at a time. – Timothy Coetzee Jul 15 at 16:02
    
@TimothyCoetzee it think it would certainly be worth understanding at least the basics of Objected Oriented programming before jumping into Laravel or another MVC framework - you'll not only pick up the framework more quickly, but you'll understand WHY/HOW the techniques are employed and how to implement some of the benefits of their approach in your own code. There's plenty of info in the php manual - php.net/manual/en/language.oop5.php – C Ivemy Jul 27 at 16:22

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.