Critique for a sports website
Posted: Tue Feb 12, 2013 8:29 am
I am developing my first PHP app - a sports league website. I am not using any framework because I want to understand the basics since I don't have any programming background.
I am looking for feedback to make my code better, eg. can it be shortened anywhere, can I have lesser files, or is it better to separate out further, etc.
My main confusion is around using mysqli prepared statements in functions and returning the data, especially when I want multiple record sets. Should I return an array or should I loop through the results on the view page?
Thanks for your feedback.
Here is my setup
a config file holding my db connection info
a db class to connect to the database using values from the config file
One of my classes is a League class as follows
I am not listing all the functions since I need critique on how I am writing my methods and if I am returning the data correctly. It works, but want to see how it can be improved.
I have following files to perform CRUD
leaguenew.php - Contains the form to create the league, when submitted to self it validates the data, checks if a league already exists, adds the new league, and redirects to the league dashboard page
leagueedit.php - Similar to leaguenew.php but prepopulated with the league info that's being edited. Also, performs the delete function which is executed from the dashboard page
Below is the code for the 3 files
dashboard.php
leaguenew.php
leagueedit.php
I am looking for feedback to make my code better, eg. can it be shortened anywhere, can I have lesser files, or is it better to separate out further, etc.
My main confusion is around using mysqli prepared statements in functions and returning the data, especially when I want multiple record sets. Should I return an array or should I loop through the results on the view page?
Thanks for your feedback.
Here is my setup
a config file holding my db connection info
a db class to connect to the database using values from the config file
One of my classes is a League class as follows
Code: Select all
<?php
class League{
private $dbconn;
public function __construct($dbconn) {
$this->dbconn = $dbconn;
}
public function verifyLeagueName($leagueName){
$stmt = $this->dbconn->prepare('SELECT id FROM leagues WHERE LeagueName=?');
$stmt->bind_param('s',$leagueName);
$stmt->execute();
$stmt->store_result();
$rowCount = $stmt->num_rows;
$stmt->free_result();
$stmt->close();
if($rowCount > 0){
return TRUE;
} else {
return FALSE;
}
}
public function getAllLeagues(){
$stmt = $this->dbconn->prepare("SELECT id, LeagueName,LeagueAlias FROM leagues ORDER BY LeagueName asc");
$stmt->execute();
$result = $stmt->get_result();
$stmt->free_result();
$stmt->close();
if($result){
return $result;
}else{
return FALSE;
}
}
public function addLeague($leagueName,$leagueAlias){
$this->dbconn->autocommit(FALSE);
$stmt = $this->dbconn->prepare("INSERT INTO leagues (LeagueName, LeagueAlias)
VALUES (?,?)");
$stmt->bind_param('ss',$leagueName,$leagueAlias);
$stmt->execute();
$leagueInserted = $stmt->affected_rows;
$stmt->close();
if($leagueInserted == 1){
$this->dbconn->commit();
$this->dbconn->autocommit(TRUE);
return TRUE;
}else {
$this->dbconn->rollback(); // remove temporary data stored in the tables
$this->dbconn->autocommit(TRUE); // turn autocommit back on
return FALSE; // tell controller it failed
}
}
public function updateLeague($leagueId,$leagueName,$leagueAlias){
$this->dbconn->autocommit(FALSE);
$stmt = $this->dbconn->prepare('UPDATE leagues SET LeagueName=?,LeagueAlias=?
WHERE id=?');
$stmt->bind_param('ssi',$leagueName,$leagueAlias,$leagueId);
$stmt->execute();
$rowsUpdated = $stmt->affected_rows;
if($rowsUpdated === 1){
$this->dbconn->commit();
$this->dbconn->autocommit(TRUE);
return TRUE;
} else {
$this->dbconn->rollback(); // remove temporary data stored in the tables
$this->dbconn->autocommit(TRUE); // turn autocommit back on
return FALSE; // tell controller it failed
}
}
public function deleteLeague($leagueId){
$stmt = $this->dbconn->prepare('DELETE FROM leagues WHERE id=?');
$stmt->bind_param('i',$leagueId);
$stmt->execute();
$rowCount = $stmt->affected_rows;
if($rowCount == 1){
return TRUE;
}else{ return FALSE; }
}
}
I have following files to perform CRUD
leaguenew.php - Contains the form to create the league, when submitted to self it validates the data, checks if a league already exists, adds the new league, and redirects to the league dashboard page
leagueedit.php - Similar to leaguenew.php but prepopulated with the league info that's being edited. Also, performs the delete function which is executed from the dashboard page
Below is the code for the 3 files
dashboard.php
Code: Select all
<?php
require_once __DIR__.'/lib/classes/db.php';
require_once __DIR__.'/lib/classes/User.php';
require_once __DIR__.'/lib/classes/League.php';
require_once __DIR__.'/lib/validateforms.php';
session_start();
if (!$_SESSION['UserRole']){
header('Location: index.php');
exit();
}
include __DIR__.'/lib/header.php';
?>
<div id="content">
<div id="leftbox">
</br>
<!-- Leagues Section Start-->
<?php
}
if($_SESSION['UserRole'] === 'WebAdmin' ||
$_SESSION['UserRole'] === 'LeagueAdmin'){
?>
<h3 class="subHeading">Leagues - Current Season</h3>
<?php if($_SESSION['UserRole'] === 'WebAdmin'){?>
<a href="leaguenew.php">[Add New League]</a>
<?php } ?>
<table class="table">
<thead>
<tr>
<th>No.</th>
<th>Name</th>
<th>Alias</th>
<th></th>
<th></th>
<th></th>
<th></th>
</tr>
</thead>
<tbody>
<?php
$currentSeason = date('Y');
$db = generaldb::getInstance();
$league = new League($db);
$leagueResult = $league->getAllLeagues();
if($leagueResult === FALSE){
echo '<tr><td>No leagues found</td><td></td><td></td><td></td><td></td></tr>';
}else{
$serial = 1;
while($rows = $leagueResult->fetch_assoc()):
echo '<tr><td>' , $serial++ , '</td>';
echo'<td>' , htmlentities($rows['LeagueName']) , '</td>';
echo'<td>' , htmlentities($rows['LeagueAlias']) , '</td>';
$leagueId = $rows['id'];
?>
<td>
<form method="POST" action="leagueedit.php">
<input type="hidden" name="leagueidbox" value="<?php echo $leagueId; ?>" />
<button type="Submit" name ="leagueEditBttn" class="editBttn">Edit</button>
</form>
</td>
<td>
<?php if($_SESSION['UserRole'] === 'WebAdmin'){?>
<form method="POST" action="leagueedit.php">
<input type="hidden" name="leagueidbox" value=" <?php echo $leagueId; ?>"/>
<button type="Submit" name ="leagueDeleteBttn" class="deleteBttn">Delete</button>
</form>
<?php } ?>
</td>
<td>
<form method="POST" action="manageleagueusers.php">
<input type="hidden" name="leagueidbox" value="<?php echo $leagueId; ?>"/>
<button type="submit" name ="manageLeagueUsersBttn">Admins</button>
</form>
</td>
<td>
<form method="POST" action="dashboardleague.php">
<input type="hidden" name="leagueidbox" value="<?php echo $leagueId; ?>"/>
<button type="submit" name ="leagueDashboardBttn">Dashboard</button>
</form>
</td>
<?php
echo '</tr>';
endwhile;
}
?>
</tbody>
</table></br></br>
<!-- Leagues Section End-->
<?php } ?>
</div><!-- End of leftbox div -->
<div id="rightbox"><!-- End of rightbox div -->
<?php include __DIR__.'/lib/rightbar.php'; ?>
</div>
</div><!-- End of content div -->
<?php include __DIR__.'/lib/footer.php'; ?>
Code: Select all
<?php
session_start();
require_once __DIR__.'/lib/classes/db.php';
require_once __DIR__.'/lib/classes/League.php';
require_once __DIR__.'/lib/classes/Season.php';
require_once __DIR__.'/lib/validateforms.php';
if($_SESSION['UserRole'] === 'WebAdmin'){
$leagueNameIsUnique = TRUE;
$leagueAliasIsUnique = TRUE;
$leagueNameLengthValid = TRUE;
$leagueAliasLengthValid = TRUE;
$successMsg = '';
$failMsg = '';
if(isset($_POST['newLeagueBttn'])){
$db = generaldb::getInstance();
$league = new League($db);
$LName = $_POST['leaguenamebox'];
$LAlias = $_POST['leaguealiasbox'];
$leagueNameLengthValid = checklength($LName,5,50);
$leagueAliasLengthValid = checklength($LAlias,2,10);
$leagueNameExists = $league->verifyLeagueName($LName);
if($leagueNameExists){
$leagueNameIsUnique = FALSE;}
$leagueAliasExists = $league->verifyLeagueAlias($LAlias);
if($leagueAliasExists === TRUE){
$leagueAliasIsUnique = FALSE;}
if($leagueNameIsUnique
&& $leagueNameLengthValid
&& $leagueAliasIsUnique
&& $leagueAliasLengthValid){
$addLeague = $league->addLeague($LName, $LAlias);
if($addLeague === TRUE){
$successMsg = 'New League added successfully.<br/>';
}else{
$failMsg = 'League was not added.<br/>';
}
}
}else{
$LName = '';
$LAlias = '';
}
}else{
header('Location: dashboard.php');
exit();
}
include __DIR__.'/lib/header.php';
?>
<div id="content">
<div id="leftbox">
<br/>
<?php echo '<h3>',$_SESSION['FirstName'],'!! You are adding a new league.</h3>'; ?>
<form class="form entryForm" method ="POST" action="leaguenew.php">
<h2>Add New League</h2><br/>
<div class="errorMsg">
<?php if(!$leagueNameIsUnique){echo "Oops!! League Name already exists. Try another one.<br/>";} ?>
<?php if(!$leagueNameLengthValid){echo 'League Name should be between 5 and 50 characters.<br/>';} ?>
<?php if(!$leagueAliasIsUnique){echo "Oops!! League Alias already exists. Try another one.<br/>";} ?>
<?php if(!$leagueAliasLengthValid){echo 'League Alias should be between 2 and 10 characters.<br/>';} ?>
<?php echo $failMsg; ?>
</div>
<div class="successMsg">
<?php echo $successMsg; ?>
</div><br/>
<label>Name</label>
<input type="text" name="leaguenamebox" value="<?php echo $LName; ?>" placeholder="League Full Name" /><br/><br/>
<label>Alias</label>
<input type="text" name="leaguealiasbox" value="<?php echo $LAlias; ?>" placeholder="League Alias" /><br/><br/>
<button type="Submit" name="newLeagueBttn" class="submitBttn">Save League</button>
</form>
</div><!-- End of leftbox div -->
<div id="rightbox"><!-- End of rightbox div -->
<h1>This is my RIGHTBOX</h1>
</div>
</div><!-- End of content div -->
<?php include __DIR__.'/lib/footer.php'; ?>
Code: Select all
<?php
session_start();
require_once __DIR__.'/lib/classes/db.php';
require_once __DIR__.'/lib/classes/League.php';
require_once __DIR__.'/lib/classes/Season.php';
require_once __DIR__.'/lib/validateforms.php';
$db = generaldb::getInstance();// Define new instance of the database class
$league = new League($db);// Define new instance of League class
//Check for Edit button from previous page
if(isset($_POST['leagueEditBttn']) || isset($_POST['leagueDeleteBttn'])){
//Read values from Edit table
$leagueId = $_POST['leagueidbox'];
//Get League information based on id
$leagueResult = $league->getLeagueById($leagueId);
$leagueRows = $leagueResult->fetch_assoc();
$leagueName = $leagueRows['LeagueName'];
$leagueAlias = $leagueRows['LeagueAlias'];
}
//Verify user role
if($_SESSION['UserRole'] === 'WebAdmin'
|| $_SESSION['UserRole'] === 'LeagueAdmin'){
// Set the variables for validation
$leagueNameIsUnique = TRUE;
$leagueAliasIsUnique = TRUE;
$leagueNameLengthValid = TRUE;
$leagueAliasLengthValid = TRUE;
$successMsg = '';
$failMsg = '';
//Check for Save button from current page
if(isset($_POST['saveLeagueBttn'])){
//Capture all the submitted values from current page
$leagueName = $db->escape($_POST['leaguenamebox']);
$leagueAlias = $db->escape($_POST['leaguealiasbox']);
$leagueId = $_POST['leagueidbox'];
//Validate the fields
$leagueNameLengthValid = checklength($leagueName,5,50);
$leagueAliasLengthValid = checklength($leagueAlias,2,10);
//Verify if records exist
$leagueNameExists = $league->verifyLeagueName($leagueName);
$leagueAliasExists = $league->verifyLeagueAlias($leagueAlias);
//Check if fields are filled correctly
if($leagueNameLengthValid && $leagueAliasLengthValid){
//Check if league name exists
if($leagueNameExists){
$leagueNameIsUnique = FALSE;
}
//Check if league alias exists
if($leagueAliasExists){
$leagueAliasIsUnique = FALSE;
}
//If League name & alias are unique than update
if($leagueNameIsUnique || $leagueAliasIsUnique){
$updateLeague = $league->updateLeague($leagueId, $leagueName, $leagueAlias);
if($updateLeague){
header('Location: dashboard.php');
exit();
} else {
$failMsg = "Ooops!! Something went wrong. Try again.";
}
}
}
}
}else{
//if user role is not authorized than redirect to logout
header('Location: logout.php');
exit();
}
if(isset($_POST['leagueDeleteBttn'])){
if($_SESSION['UserRole'] === 'WebAdmin'){
$dbconn = generaldb::getInstance();
$league = new League($dbconn);
$leagueId = $_POST['leagueidbox'];
$deleteLeague = $league->deleteLeague($leagueId);
if($deleteLeague){
header('Location: dashboard.php');
exit();
} else {
$failMsg = 'League was not deleted. First delete all records under this league to delete the league.';
}
} else {
$failMsg = 'You do not have permissions to perform this action';
}
}
include __DIR__.'/lib/header.php';
?>
<div id="content">
<div id="leftbox">
<br/>
<?php
echo '<h3>', $_SESSION['FirstName'],'!! You are editing <strong>',$leagueName,'</strong> league.</h3><br/><br/>';
?>
<form class="form entryForm" method ="POST" action="leagueedit.php">
<h2>Edit League</h2><br/>
<div class="errorMsg">
<?php if(!$leagueNameIsUnique){echo "Oops!! League Name already exists. Try another one.<br/>";} ?>
<?php if(!$leagueNameLengthValid){echo 'League Name should be between 5 and 50 characters.<br/>';} ?>
<?php if(!$leagueAliasIsUnique){echo "Oops!! League Alias already exists. Try another one.<br/>";} ?>
<?php if(!$leagueAliasLengthValid){echo 'League Alias should bebetween 2 and 10 characters.<br/>';} ?>
<?php echo $failMsg; ?>
</div>
<div class="successMsg">
<?php echo $successMsg; ?>
</div><br/>
<input type="hidden" name="leagueidbox" value="<?php echo $leagueId ?>" />
<label>Name</label>
<input type="text" name="leaguenamebox" value="<?php echo $leagueName; ?>" placeholder="League Full Name" /><br/><br/>
<label>Alias</label>
<input type="text" name="leaguealiasbox" value="<?php echo $leagueAlias; ?>" placeholder="League Alias" /><br/><br/>
<button type="Submit" name="saveLeagueBttn" class="submitBttn">Save League</button>
</form>
</div><!-- End of leftbox div -->
<div id="rightbox">
<?php include __DIR__.'/lib/rightbar.php'; ?>
</div><!-- End of rightbox div -->
</div><!-- End of content div -->
<?php include __DIR__.'/lib/footer.php'; ?>