PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Tue Oct 24, 2017 4:46 am

All times are UTC - 5 hours




Post new topic Reply to topic  [ 3 posts ] 
Author Message
PostPosted: Tue Feb 12, 2013 9:29 am 
Offline
Forum Commoner

Joined: Wed Dec 14, 2011 1:17 pm
Posts: 36
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
Syntax: [ Download ] [ Hide ]
<?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 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
Syntax: [ Download ] [ Hide ]
<?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'; ?>
 


leaguenew.php
Syntax: [ Download ] [ Hide ]
<?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'; ?>
 


leagueedit.php
Syntax: [ Download ] [ Hide ]
<?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'; ?>
 


Top
 Profile  
 
PostPosted: Thu Feb 14, 2013 7:41 pm 
Offline
Forum Contributor
User avatar

Joined: Wed Apr 14, 2010 4:45 pm
Posts: 375
Location: UK
Hi,

While I'm looking at the code, here are a few suggestions:

1. There is a reasonable amount of redundancy in your database query code - maybe you could have a parent class that does the basic query handling and then extend it with child classes that give context to the queries.

2. You define quite a few variables to contain the results of test conditions (is the league name unique, etc.) when you could probably just include the functions they call directly in your conditional tests. Compare

Syntax: [ Download ] [ Hide ]
$is_unique = NULL;
$is_unique = check_if_unique($some_value);
if ($is_unique == TRUE) {
// do something
}


with

Syntax: [ Download ] [ Hide ]
if (check_if_unique($some_value) == TRUE) {
// do something
}


That said, an argument *against* that approach would be that you might end up hard-coding that function into lots of places in your script.

3. Define all your error messages in one place and use constants or globals instead as placeholders - this means that you can easily tailor the application for different language variations.

4. Consider using or writing a basic templating engine so you don't have to have HTML in your PHP. I know some developers think that having HTML sprawled all over their PHP is the way to go but, from a maintenance point of view, it's not that great - at least in my opinion :) I find it some much easier to see what's going on in both the script *and* the page template when the two are separate, and my template engine handles all the basic things like context switching and repeating elements. Arguments I've heard against this approach tend to suggest that a templating engine is (somehow) completely redundant and goes against the very nature of PHP itself, but if that were the case then templating engines like Smarty would presumably be very unpopular (which they aren't). In case you're still not convinced, check out this page:

http://smarty.incutio.com/?page=SmartyFrequentlyAskedQuestions#basics-1

Anyway, just a few points for what they're worth (remembering that my advice is worth what you paid for it).

HTH,

Mecha Godzilla


Top
 Profile  
 
PostPosted: Fri Mar 01, 2013 6:05 pm 
Offline
Moderator
User avatar

Joined: Tue Nov 09, 2010 3:39 pm
Posts: 6391
Location: Montreal, Canada
Without having spent a ton of time on it, here are some quick observations:

You don't need a third-party templating engine -- PHP is perfectly suited to that task -- but you should consider making use of templates. Separate your concerns.

Syntax: [ Download ] [ Hide ]
$db = generaldb::getInstance();

Try to avoid using static methods. They're inflexible and difficult to test.

What does your generaldb class actually do? Wrap the mysqli constructor?

Syntax: [ Download ] [ Hide ]
public function __construct($dbconn) {
    $this->dbconn = $dbconn;
}

Consider adding type hinting to the constructor.

Syntax: [ Download ] [ Hide ]
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;
        }
}

This looks like a scoping problem to me. You have a database wrapper class; why is your League class making database calls directly? Why are you writing raw SQL? This is also too tightly coupled to the mysqli object. Also, and this is very minor, $rowCount > 0 returns a boolean; you can just return that.

Syntax: [ Download ] [ Hide ]
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';

Look into auto loading.

Syntax: [ Download ] [ Hide ]
$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;}

Validator class?

Syntax: [ Download ] [ Hide ]
if(isset($_POST['leagueDeleteBttn'])){
    if($_SESSION['UserRole'] === 'WebAdmin'){
        $dbconn = generaldb::getInstance();

$dbconn was already defined.

_________________
Supported PHP versions No longer supported versions


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 3 posts ] 

All times are UTC - 5 hours


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
Powered by phpBB® Forum Software © phpBB Group