Please review this code?

Discussion in 'PHP' started by mokimofiki, Oct 14, 2008.

  1. #1
    I'm not sure what I have done wrong in this snippet of code. If anybody sees any errors in it please let me know. Also any ways that may be better to write it would be nice as well. I'm still a bit Newbie at PHP although i'm getting better thanks to these forums.

    if ($phealth < 1)
    {
      echo "<font color=red>You did $ptotaldmg damage while taking $mtotaldmg damage and were defeated by $enemyuid losing $lostgold Gold and gaining no Skill !!!</font><br>";
      $mskill1 = rand(1,3);
    
        if($mskill=1)
        {
            $mevade = ($mevade+1);
            $magility = ($magility+1);
            mysql_query("INSERT INTO skills WHERE uidskills='$enemyuid' (evade, agility) VALUES ($mevade, $magility)");
        }
        elseif($mskill=2)
        {
            $mevade = ($mevade+1);
            $mdexterity = ($mdexterity+1);
            mysql_query("INSERT INTO skills WHERE uidskills='$enemyuid' (evade, dexterity) VALUES ($mevade, $mdexterity)");
        }
        else
        {
            $mdexterity = ($mdexterity+1);
            $magility = ($magility+1);
            mysql_query("INSERT INTO skills WHERE uidskills='$enemyuid' (agility, dexterity) VALUES ($magility, $mdexterity)");
        }
      
    }
    elseif ($mhealth < 1)
    {
      echo "<font color=green>You did $ptotaldmg damage while taking $mtotaldmg damage and defeated $enemyuid gaining $gainedgold Gold and the skills below !!!</font><br>";
    
      $mskill1 = rand(1,3);
    
        if($mskill=1)
        {
            $pbarehands = ($pbarehands+1);
            $pstrength = ($pstrength+1);
            mysql_query("INSERT INTO skills WHERE uidskills='$_SESSION[uid]' (barehands, strength) VALUES ($pbarehands, $pstrength)");
            echo "<font color=green>Your Barehands Skill has increased by 1 point!</font><br>";
            echo "<font color=green>Your Strength Skill has increased by 1 point!</font>";
        }
        elseif($mskill=2)
        {
            $pbarehands = ($pbarehands+1);
            $pdexterity = ($pdexterity+1);
            mysql_query("INSERT INTO skills WHERE uidskills='$_SESSION[uid]' (barehands, dexterity) VALUES ($pbarehands, $pdexterity)");
            echo "<font color=green>Your Barehands Skill has increased by 1 point!</font><br>";
            echo "<font color=green>Your Dexterity Skill has increased by 1 point!</font>";
        }
        else
        {
            $pagility = ($pagility+1);
            $pstrength = ($pstrength+1);
            mysql_query("INSERT INTO skills WHERE uidskills='$_SESSION[uid]' (agility, strength) VALUES ($pagility, $pstrength)");
            echo "<font color=green>Your Agility Skill has increased by 1 point!</font><br>";
            echo "<font color=green>Your Strength Skill has increased by 1 point!</font>";
        }
      
    }
    Code (markup):
    Thank you in advance.

    Also I realize that nobody knows what the variables come from but its more syntax that i'm looking for.
     
    mokimofiki, Oct 14, 2008 IP
  2. dura_killer

    dura_killer Peon

    Messages:
    684
    Likes Received:
    5
    Best Answers:
    0
    Trophy Points:
    0
    #2
    The error according to me that I spoted is that you are using '=' in 'if' statement rather than '=='
    Only problem I am able to spot, I am newbie too in php, but if it helps then leave a rep to me.Gl
     
    dura_killer, Oct 14, 2008 IP
  3. mokimofiki

    mokimofiki Well-Known Member

    Messages:
    444
    Likes Received:
    3
    Best Answers:
    0
    Trophy Points:
    130
    #3
    Thats not the problem thank you for the response though after further testing the problem seems to lie in this part of the code:

    mysql_query("INSERT INTO skills WHERE uidskills='$enemyuid' (evade, agility) VALUES ($mevade, $magility)");
     
    mokimofiki, Oct 14, 2008 IP
  4. zeroco

    zeroco Peon

    Messages:
    27
    Likes Received:
    1
    Best Answers:
    0
    Trophy Points:
    0
    #4
    Maybe the problem is that you won't get get the value of $enemyuid when it's in single quotes, try this:

    
    mysql_query("INSERT INTO skills WHERE uidskills='" . $enemyuid . "' (evade, agility) VALUES ($mevade, $magility)");
    
    Code (markup):
     
    zeroco, Oct 14, 2008 IP
  5. Logician

    Logician Active Member

    Messages:
    213
    Likes Received:
    5
    Best Answers:
    0
    Trophy Points:
    60
    #5
    1- You shouldn't use WHERE in a INSERT INTO query.

    wrong:
    mysql_query("INSERT INTO skills WHERE uidskills='" . $enemyuid . "' (evade, agility) VALUES ($mevade, $magility)");

    correct:
    mysql_query("INSERT INTO skills (evade, agility) VALUES ($mevade, $magility)");

    If you are trying to update a record with WHERE you should use UPDATE, NOT INSERT:
    mysql_query("
    UPDATE skills
    SET evade = ".addslashes($mevade).",
    agility = ".addslashes($magility)."
    WHERE uidskills='" . $enemyuid . "'
    ")

    2- if condition takes ==, not =
     
    Logician, Oct 14, 2008 IP
    mokimofiki likes this.
  6. mokimofiki

    mokimofiki Well-Known Member

    Messages:
    444
    Likes Received:
    3
    Best Answers:
    0
    Trophy Points:
    130
    #6
    Thank you Logician that works when it comes to the DB. Now I have ran into something else here is the code I will post the problem below it:
    $mskill1 = rand(1,3);
      
        if ($mskill==1)
        {
            $mevade = ($mevade+1);
            $magility = ($magility+1);
            $pstamina = ($pstamina-1);
            mysql_query("UPDATE skills SET stamina = '$pstamina' WHERE uidskills = '$_SESSION[uid]'");
            mysql_query("UPDATE skills SET evade = '$mevade' WHERE uidskills = '$enemyuid'");
            mysql_query("UPDATE skills SET agility = '$magility' WHERE uidskills = '$enemyuid'");
        }
        elseif ($mskill==2)
        {
            $mevade = ($mevade+1);
            $mdexterity = ($mdexterity+1);
            $pstamina = ($pstamina-1);
            mysql_query("UPDATE skills SET stamina = '$pstamina' WHERE uidskills = '$_SESSION[uid]'");
            mysql_query("UPDATE skills SET evade = '$mevade' WHERE uidskills = '$enemyuid'");
            mysql_query("UPDATE skills SET dexterity = '$mdexterity' WHERE uidskills = '$enemyuid'");
        }
        else
        {
            $mdexterity = ($mdexterity+1);
            $magility = ($magility+1);
            $pstamina = ($pstamina-1);
            mysql_query("UPDATE skills SET stamina = '$pstamina' WHERE uidskills = '$_SESSION[uid]'");
            mysql_query("UPDATE skills SET agility = '$magility' WHERE uidskills = '$enemyuid'");
            mysql_query("UPDATE skills SET dexterity = '$mdexterity' WHERE uidskills = '$enemyuid'");
        }
    Code (markup):
    The else is always the one that is chosen even if the random number is 1 or 2?

    NEVERMIND OBVIOUS PROBLEM I JUST NEED TO DO SOMETHING ELSE FOR A FEW MINUTES I THINK I'M BURNED OUT FOR THE MOMENT LOL (WRONG VARIABLE BEING USED)
     
    mokimofiki, Oct 14, 2008 IP
  7. zeroco

    zeroco Peon

    Messages:
    27
    Likes Received:
    1
    Best Answers:
    0
    Trophy Points:
    0
    #7
    Ouch, how embarrassing, my eye just caught that single quoted variable and I didn't even consider what it was part of. :eek:

    good job Logician

    About the new question, I make another attempt to help, only considering the variable and not what's in the if/else blocks :)

    You store the random number in a field called $mskill1 but the test is for a field called $mskill, there's a '1' at the end of the field where the number is stored. That's why you always end up in the "else".
     
    zeroco, Oct 14, 2008 IP
  8. chriseccles2

    chriseccles2 Peon

    Messages:
    28
    Likes Received:
    0
    Best Answers:
    0
    Trophy Points:
    0
    #8
    Coming from a C background, I can also testify that
    the inability to distinguish a lower-case letter 'ell', l ,from the
    number one, 1 ,in the Courier font, has driven more
    programmers into the arms of their therapists than any
    other single source of trouble ............

    :p

    Rule #1 (Drilled into me by my first computing and statistics boss):

    NEVER end a var name in l ! ! ! !


    -
    Chris
     
    chriseccles2, Oct 14, 2008 IP
  9. deathshadow

    deathshadow Acclaimed Member

    Messages:
    9,732
    Likes Received:
    1,999
    Best Answers:
    253
    Trophy Points:
    515
    #9
    Funny, I just always edit using fixedsys... end of problem. Courier sucks ass, if for no other reason than being too pencil thin for legibility minimums.

    I'm not seeing anything too out of the ordinary, though I will say you've got some bad coding practices in there. You are checking a single variable, instead of if-then-else-if-then-else you should be using a switch. You are not using the random variable more than once, so why are you wasting the overhead on it? (oh wait, you are using if then else if then else if then else). You are incrementing values, save yourself some code (and php being an interpreted language this DOES count) and use x+=1 instead of x=x+1. You are performing single operations, as such you don't need parenthases around your operations. You have multiple echo statements in places where only one is needed, multiple font declarations around elements that should only need one (NOT that I would even be using the deprecated font tag in the first place), and double quotes around elements that aren't passing variables (slower).

    It is also no longer considered good practice to not put your array indexes in quotes. Where you are calling '$_SESSION[uid]' I'd revert that to '".$_SESSION['uid']."' just to meet that new 'requirement' as of php5.

    http://us3.php.net/manual/en/language.types.array.php
    see "do's and don'ts".

    I'd probably code that as:

    if ($phealth<1) {
    	echo '
    		<p class="defeated">
    			You did ',$ptotaldmg,' damage 
    			while taking ',$mtotaldmg,' damage
    			and were defeated by ',$enemyuid,' 
    			losing ',$lostgold Gold,'
    			and gaining no Skill!!!
    		</p>
    	';
    	switch (rand(0,2)) {
    		case 0:
    			$mevade+=1;
    			$magility+=1;
    			mysql_query("UPDATE skills
    				SET evade='".$mevade."',
    					agility='".$magility."'
    				WHERE uidskills='".$enemyuid."'
    			");
    		break;
    		case 1:
    			$mevade+=1;
    			$mdexterity+=1;
    			mysql_query("UPDATE skills
    				SET
    					evade='".$mevade."',
    					dexterity='".$mdexterity."'
    				WHERE uidskills='".$enemyuid."'
    			");
    		break;
    		default:
    			$mdexterity+=1;
    			$magility+=1;
    			mysql_query("UPDATE skills
    				SET
    					agility='".$magility."',
    					dexterity='".$mdexterity."'
    				WHERE uidskills='".$enemyuid."'
    			");
    		/* end switch */	
    	}
    	
    } else if ($mhealth<1) {
    
    	echo '
    		<p class="wonFight">
    			You did ',$ptotaldmg,' damage 
    			while taking ',$mtotaldmg,' damage
    			and defeated ',$enemyuid,'
    			gaining ',$gainedgold,' Gold 
    			and the skills below!!!
    		</p>
    	";
    	switch (rand(0,2)) {
    		case 0:
    			$pbarehands+=1;
    			$pstrength+=1;
    			mysql_query("UPDATE skills 
    				WHERE uidskills='".$_SESSION['uid']."' 
    				SET barehands='".$pbarehands."',
    					strength='".$pstrength."'
    			");
    			echo '
    				<div class="skillIncrease">
    					Your Barehands Skill has increased by 1 point!<br>
    					Your Strength Skill has increased by 1 point!
    				</div>
    			';
    		break;
    		case 1:
    			$pbarehands+=1;
    			$pdexterity+=1;
    			mysql_query("UPDATE skills 
    				SET barehands='".$pbarehands."',
    					dexterity='".$pdexterity."'
    				WHERE uidskills='".$_SESSION['uid']."' 
    			");
    			echo '
    				<div class="skillIncrease">
    					Your Barehands Skill has increased by 1 point!<br>
    					Your Dexterity Skill has increased by 1 point!
    				</div>
    			';
    		break;
    		default:
    			$pagility+=1;
    			$pstrength+=1;
    			mysql_query("UPDATE skills 
    				SET agility='".$pagility."',
    					strength='".$pstrength."'
    				WHERE uidskills='".$_SESSION['uid']."' 
    			");
    			echo '
    				<div class="skillIncrease">
    					Your Agility Skill has increased by 1 point!<br>
    					Your Strength Skill has increased by 1 point!
    				</div>
    			';
    		/* end switch */	
    	}
    }
    Code (markup):
     
    deathshadow, Oct 14, 2008 IP
    mokimofiki likes this.
  10. mokimofiki

    mokimofiki Well-Known Member

    Messages:
    444
    Likes Received:
    3
    Best Answers:
    0
    Trophy Points:
    130
    #10
    Deathshadow thank you for the lesson on cleaning up my code I am still a newbie at php and just write it however I know how. Although I will make it a point to use the practices you've shown me above and I really do appriciate you taking the time to explain each thing in detail.

    BTW: I noticed the skill1 right after I posted the code thats why I put the edit at the bottom.

    Thank you everyone for your responses and help :)
     
    mokimofiki, Oct 15, 2008 IP