Random Page to Critique

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
CoolAsCarlito
Forum Contributor
Posts: 192
Joined: Sat May 31, 2008 3:27 pm
Contact:

Random Page to Critique

Post by CoolAsCarlito »

I was hoping someone could really take a look through this and see if they notice anything that looks wrong or should be written better or if I'm forgetting to do something. I want to give thanks to anyone who takes time to.

Code: Select all

<?php

session_start(); // Access the existing session

// Include the database page
require ('../../inc/dbconfig.php');

$userID = $_SESSION['userID']; // Assigns a variable from the logged in userID

$charactersQuery = "
    SELECT 
        ID, 
        characterName 
    FROM 
        characters 
    WHERE 
        ID NOT IN (SELECT characterID FROM handlerCharacters) 
    ORDER BY 
        characters.characterName";
$charactersByHandlerResult = mysqli_query ( $dbc, $charactersQuery ); // Run The Query

$statusQuery = "
    SELECT 
        * 
    FROM 
        statuses";
$statusResult = mysqli_query ( $dbc, $statusQuery ); // Run The Query 


?>
<script type="text/javascript">
$(document).ready(function() {
    $('div.message-error').hide();
    $('div.message-success').hide();
    $('ul#characterList').css( 'margin-left', '120px' );
    $('li').remove('.characterName');
    $("#handlerForm").validate({ 
        rules: {
            firstName: {
                required: true,
            },
            lastName: {
                required: true,
            },
            userName: {
                required: true,
                minlength: 5
            },
            password: {
                required: true,
                minlength: 5
            },
            password2: {
                required: true,
                minlength: 5,
                equalTo: "#password"
            },
            email: {
                required: true,
                email: true
            }
        },
        messages: {
            firstName: {
                required: "Please enter your first name",
            },
            lastName: {
                required: "Please enter your last name",
            },
            userName: {
                required: "Please enter a username",
                minlength: "Your username must consist of at least 5 characters"
            },
            password: {
                required: "Please provide a password",
                minlength: "Your password must be at least 5 characters long"
            },
            password2: {
                required: "Please provide a password",
                minlength: "Your password must be at least 5 characters long",
                equalTo: "Please enter the same password as above"
            },
            email: "Please enter a valid email address"
        }
    });
    $("input.submit").click(function() {
        var userID = $("input#userID").val();
        var defaultChar = $("select#charactersDrop option:selected").text();
        var userName = $("input#userName").val();
        var firstName = $("input#firstName").val();
        var lastName = $("input#lastName").val(); 
        var password = $("input#password").val(); 
        var email = $("input#email").val(); 
        var statusID = $("select#statusID").val(); 
        var isAdmin = $("select#isAdmin").val(); 
        var liElements = $("ul#characterList li");
        var characterIDList = "";
        for( var i = 0; i < liElements.length; i++ ) {
            var liElement = $( liElements[ i ] );
            
            // only start appending commas in after the first characterID
            if( i > 0 ) {
                characterIDList += ","; 
            }
            
            // append the current li element's characterID to the list
            characterIDList += liElement.data( 'characterID' );
        }
        var dataString = 'userID=' + userID + 'userName=' + userName  + '&password=' + password + '&firstName=' + firstName + '&lastName=' + lastName + '&email=' + email + '&statusID=' + statusID + '&isAdmin=' + isAdmin + '&characterIDList=' + characterIDList + '&submitHandler=True';
        $.ajax({
            type: "POST",
            url: "processes/handler.php",
            data: dataString,
            success: function(myNewVar) {
                if (myNewVar == 'good') {
                    $('div.message-error').hide();
                    $("div.message-success").html("<h6>Operation successful</h6><p>" + userName + " saved successfully.</p>");
                    $("div.message-success").show().delay(10000).hide("slow");
                    $(':input','#handlerForm')
                    .not(':submit, :button, :hidden')
                    .val('')
                    $('ul#characterList li').each(function() {
                    $(this).remove();
                    });
                } else if (myNewVar == 'bad1') {
                    $('div.message-success').hide();
                    $("div.message-error").html("<h6>Operation unsuccessful</h6><p>" + userName + " already exists in the database.</p>");
                    $("div.message-error").show();    
                } else if (myNewVar == 'bad2') {
                    $('div.message-success').hide();
                    $("div.message-error").html("<h6>Operation unsuccessful</h6><p>" + email + " already exists in the database.</p>");
                    $("div.message-error").show();     
                } else if (myNewVar == 'bad3') {
                    $('div.message-success').hide();
                    $("div.message-error").html("<h6>Operation unsuccessful</h6><p>" + userName + " and " + email + " already exists in the database.</p>");
                    $("div.message-error").show();     
                } else if (myNewVar == 'bad1bad2') {
                    $('div.message-success').hide();
                    $("div.message-error").html("<h6>Operation unsuccessful</h6><p>" + userName + " and " + email + " already exists in the database.</p>");
                    $("div.message-error").show();     
                }  
            }
        });
        return false;    
    });
});
</script>

<!-- Form -->
<form action="#" id="handlerForm">
	<fieldset>
		<legend>Add New Handler</legend>
        <div class="field required">
			<label for="userName">User Name</label>
			<input type="text" class="text" name="userName" id="userName" title="User Name"/>
			<span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="password">Password</label>
			<input type="password" class="text" name="password" id="password" title="Password" />
            <span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
        </div>
        <div class="field required">
			<label for="password2">Confirm Password</label>
			<input type="password" class="text" name="password2" id="password2" title="Confirm Password" />
            <span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
        </div>
        <div class="field required">
			<label for="firstName">First Name</label>
			<input type="text" class="text" name="firstName" id="firstName" title="First Name"/>
            <span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="lastName">Last Name</label>
			<input type="text" class="text" name="lastName" id="lastName" title="Last Name"/>
            <span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="email">Email</label>
			<input type="text" class="text" name="email" id="email" title="Email"/>
            <span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="statusID">Status</label>
            <select class="dropdown" name="statusID" id="statusID" title="Status">
                <option value="">- Select -</option>
                <?php
                    while ( $row = mysqli_fetch_array ( $statusResult, MYSQL_ASSOC ) ) { 
                        print "<option value=\"".$row['ID']."\">".$row['statusName']."</option>\r";
                    }
                ?>
            </select>
			<span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="isAdmin">Administrator</label>
            <select class="dropdown" name="isAdmin" id="isAdmin" title="Administrator">
                <option value="">- Select -</option>
                <?php
                    $administrator = array('Yes', 'No');
                    foreach($administrator as $admin):
                ?>        
                    <option value="<?php echo $admin; ?>"><?php echo $admin; ?></option>
                <?php endforeach; ?>
            </select>
			<span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
	</fieldset>
    <fieldset>
		<legend>Handler's Characters</legend>
        <div class="field">
			<label for="charactersDrop">Characters</label>
            <select class="dropdown" name="charactersDrop" id="charactersDrop" title="Characters Dropdown">
                <option value="">- Select -</option>
                <?php
                    while ( $row = mysqli_fetch_array (  $charactersByHandlerResult, MYSQL_ASSOC ) ) { 
                        print "<option value=\"".$row['ID']."\">".$row['characterName']."</option>\r";
                    }
                ?>
            </select>
            <input type="button" value="Add Character" class="" onclick="HandlerCharacters()"/>
            <ul id="characterList"></ul>
        </div>
        <input type="hidden" name="userID" id="userID" value="<?php echo $userID; ?>" />      
        <input type="submit" class="submit" name="submitHandler" id="submitHandler" title="Submit Handler" value="Submit Handler" />
	</fieldset>
</form>
<!-- /Form -->
<!-- Messages -->
<div class="message message-error">
    <h6>Required field missing</h6>
    <p>Please fill in all required fields. </p>
</div>
<div class="message message-success">
    <h6>Operation succesful</h6>
    <p>Handler was added to the database.</p>
</div>
<!-- /Messages -->  

Code: Select all

<?php

session_start(); // Access the existing session

// Include the database page
require ('../../inc/dbconfig.php');

$handlerID = $_GET['id'];

$handlerQuery = "
    SELECT 
        characters.ID,
        characters.characterName,
        handlers.userName,
        handlers.password,
        handlers.firstName,
        handlers.lastName,
        handlers.email,
        handlers.isAdmin,
        handlers.statusID
    FROM 
        handlers
        LEFT JOIN handlerCharacters
            ON handlerCharacters.handlerID =  handlers.ID
            LEFT JOIN characters
                ON characters.ID = handlerCharacters.characterID
    WHERE 
        handlers.ID = '" . $handlerID . "'";

$handlerResult = mysqli_query ( $dbc, $handlerQuery ); // Run The Query 
$row = mysqli_fetch_array ( $handlerResult, MYSQL_ASSOC );

$charactersQuery = "
    SELECT 
        ID, 
        characterName 
    FROM 
        characters 
    WHERE 
        ID NOT IN (SELECT characterID FROM handlerCharacters) 
    ORDER BY 
        characters.characterName";
$charactersByHandlerResult = mysqli_query ( $dbc, $charactersQuery ); // Run The Query

$statusQuery = "
    SELECT 
        * 
    FROM 
        statuses";
$statusResult = mysqli_query ( $dbc, $statusQuery ); // Run The Query 


?>

<script type="text/javascript">
$(document).ready(function() {
    $('div.message-error').hide();
    $('div.message-success').hide();
    $('ul#characterList').css( 'margin-left', '120px' );
    $('li').remove('.characterName');
    
    $("#handlerForm").validate({ 
        rules: {
            firstName: "required",
            lastName: "required",
            userName: {
                required: true,
                minlength: 5
            },
            password: {
                required: true,
                minlength: 5
            },
            password2: {
                required: true,
                minlength: 5,
                equalTo: "#password"
            },
            email: {
                required: true,
                email: true
            },
        },
        messages: {
            firstName: "Please enter your firstname",
            lastName: "Please enter your lastname",
            userName: {
                required: "Please enter a username",
                minlength: "Your username must consist of at least 2 characters"
            },
            password: {
                required: "Please provide a password",
                minlength: "Your password must be at least 5 characters long"
            },
            password2: {
                required: "Please provide a password",
                minlength: "Your password must be at least 5 characters long",
                equalTo: "Please enter the same password as above"
            },
            email: "Please enter a valid email address"
        }
    });
    
    $("input.submit").click(function() {
        var userID = $("input#userID").val();
        var defaultChar = $("select#charactersDrop option:selected").text();
        var userName = $("input#userName").val();
        var firstName = $("input#firstName").val();
        var lastName = $("input#lastName").val(); 
        var password = $("input#password").val(); 
        var email = $("input#email").val(); 
        var statusID = $("select#statusID").val(); 
        var isAdmin = $("select#isAdmin").val();   
        var liElements = $("ul#characterList li");
        var characterIDList = "";
        for( var i = 0; i < liElements.length; i++ ) {
            var liElement = $( liElements[ i ] );
            
            // only start appending commas in after the first characterID
            if( i > 0 ) {
                characterIDList += ","; 
            }
            
            // append the current li element's characterID to the list
            characterIDList += liElement.data( 'characterID' );
        }
        var dataString = 'userName=' + userName + '&firstName=' + firstName + '&lastName=' + lastName + '&email=' + email + '&statusID=' + statusID + '&isAdmin=' + isAdmin + '&characterIDList=' + characterIDList + '&handlerID=' + handlerID + '&editHandler=True';
        $.ajax({
            type: "POST",
            url: "processes/handler.php",
            data: dataString,
            success: function() {
                $('div.message-error').hide();
                $("div.message-success").html("<h6>Operation successful</h6><p>" + userName + " saved successfully.</p>");
                $("div.message-success").show().delay(10000).hide("slow");
                $(':input','#handlerForm')
                .not(':submit, :button, :hidden')
                .val('')
                return true;
            }
        });
        return false;    
    });
});
</script>
    
<!-- Form -->
<form action="#" id="handlerForm">
	<fieldset>
		<legend>Edit Handler</legend>
        <div class="field required">
			<label for="userName">User Name</label>
			<input type="text" class="text" name="userName" id="userName" title="User Name" value="<?php echo $row['userName']; ?>"/>
			<span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="password">Password</label>
			<input type="password" class="text" name="password" id="password" title="Password" value="<?php echo $row['password']; ?>" disabled="disabled"/>
            <span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="password2">Confirm Password</label>
			<input type="password" class="text" name="password2" id="password2" title="Confirm Password" value="<?php echo $row['password']; ?>" disabled="disabled"/>
            <span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
        </div>
        <div class="field required">
			<label for="firstName">First Name</label>
			<input type="text" class="text" name="firstName" id="firstName" title="First Name" value="<?php echo $row['firstName']; ?>"/>
            <span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="lastName">Last Name</label>
			<input type="text" class="text" name="lastName" id="lastName" title="Last Name" value="<?php echo $row['lastName']; ?>"/>
            <span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="email">Email</label>
			<input type="text" class="text" name="email" id="email" title="Email" value="<?php echo $row['email']; ?>"/>
            <span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="statusID">Status</label>
            <select class="dropdown" name="statusID" id="statusID" title="Status">
                <option value="">- Select -</option>
                <?php
                while ( $status_row = mysqli_fetch_array ( $statusResult, MYSQL_ASSOC ) ) {
                        print "<option value=\"".$status_row['ID']."\" ";
						if($status_row['ID'] == $row['statusID']) {
							     print " SELECTED";
						}
                        print ">".$status_row['statusName']."</option>\r";
                }
                ?>
            </select>
			<span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
        <div class="field required">
			<label for="isAdmin">Administrator</label>
            <select class="dropdown" name="isAdmin" id="isAdmin" title="Administrator">
                <option value="">- Select -</option>
                <?php
                    $administrator = array('Yes', 'No');
                    foreach($administrator as $admin):
                ?>        
                    <option value="<?php echo $admin; ?>"<?php if($admin == $row['isAdmin']): echo ' SELECTED'; endif; ?>><?php echo $admin; ?></option>
                <?php endforeach; ?>
            </select>
			<span class="required-icon tooltip" title="Required field - This field is required, it cannot be blank, and must contain something that is different from emptyness in order to be filled in. ">Required</span>
		</div>
	</fieldset>
    <fieldset>
		<legend>Handler's Characters</legend>
        <div class="field">
			<label for="charactersDrop">Characters</label>
            <select class="dropdown" name="charactersDrop" id="charactersDrop" title="Characters Dropdown">
                <option value="">- Select -</option>
                <?php
                while ( $row = mysqli_fetch_array (  $charactersByHandlerResult, MYSQL_ASSOC ) ) { 
                    print "<option value=\"".$row['ID']."\">".$row['characterName']."</option>\r";
                }
                ?>
            </select>
            <input type="button" value="Add Character" class="" onclick="HandlerCharacters()"/>
            <ul id="characterList">
              <?php mysqli_data_seek( $handlerResult, 0 ) ?>
              <?php while($row = mysqli_fetch_array ( $handlerResult, MYSQL_ASSOC )): ?>
                <li><?php echo htmlentities($row['characterName']); ?> <a href="#" onclick="removeCharacter(<?php echo $row['ID'] . ','. "'" . $row['characterName'] . "'"; ?>);">Remove</a></li>
              <?php endwhile; ?>
            </ul>  
        </div>
        <input type="hidden" name="handlerID" id="handlerID" value="<?php echo $handlerID; ?>" /> 
        <input type="submit" class="submit" name="editHandler" id="editHandler" title="Submit Handler" value="Submit Handler" />
	</fieldset>
</form>
<!-- /Form -->
<!-- Messages -->
<div class="message message-error">
    <h6>Required field missing</h6>
    <p>Please fill in all required fields. </p>
</div>
<div class="message message-success">
    <h6>Operation succesful</h6>
    <p>Handler was added to the database.</p>
</div>
<!-- /Messages -->
Last edited by CoolAsCarlito on Sat Mar 12, 2011 10:09 am, edited 2 times in total.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Random Page to Critique

Post by Jonah Bron »

It would be better to keep the individual pages in separate files and include() them.
CoolAsCarlito
Forum Contributor
Posts: 192
Joined: Sat May 31, 2008 3:27 pm
Contact:

Re: Random Page to Critique

Post by CoolAsCarlito »

Thank you. I was able to take a moment and do that. Any other suggestions?
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Random Page to Critique

Post by Jonah Bron »

Could you edit your post to put in the separate pages?
CoolAsCarlito
Forum Contributor
Posts: 192
Joined: Sat May 31, 2008 3:27 pm
Contact:

Re: Random Page to Critique

Post by CoolAsCarlito »

I separated them for you.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Random Page to Critique

Post by Jonah Bron »

Don't see any real problems right off. I do notice that it's not consistent in how it outputs data in tags. For example, sometimes it's like this:

Code: Select all

<option value="<?php echo $admin; ?>"<?php if($admin == $row['isAdmin']): echo ' SELECTED'; endif; ?>><?php echo $admin; ?></option>
And sometimes it's like this:

Code: Select all

print "<option value=\"".$row['ID']."\">".$row['characterName']."</option>\r";
Any particular reason for that?
CoolAsCarlito
Forum Contributor
Posts: 192
Joined: Sat May 31, 2008 3:27 pm
Contact:

Re: Random Page to Critique

Post by CoolAsCarlito »

There's not. Which is the better way to go? I do have new updated code that is a little better structured.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Random Page to Critique

Post by Jonah Bron »

I usually use single quotes and concatenate variables with the dot. Rarely do I use double quotes at all. But it's really a matter of preference. Still, it should be kept uniform. Look up the Zend coding standards.
CoolAsCarlito
Forum Contributor
Posts: 192
Joined: Sat May 31, 2008 3:27 pm
Contact:

Re: Random Page to Critique

Post by CoolAsCarlito »

I'm not a Zend framework user.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Random Page to Critique

Post by Jonah Bron »

That doesn't matter, they're syntax standards. They're good, and pretty much accepted as the standard.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Re: Random Page to Critique

Post by John Cartwright »

Jonah Bron wrote:That doesn't matter, they're syntax standards. They're good, and pretty much accepted as the standard.
I wouldn't go so far as to say it doesn't matter. As a professional however, I can agree it is probably the most adopted standard, and you'd do well to use it.
Post Reply