current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

How could I make this code more efficient? Instead of using many if-statements, should I make a function, array, or switch? How would I do that?

<form method="post" action="trivia2.php">
<ol>
<li>How many days are in a week? <input type="text" name="daysweek"></li>
<li>How many days are in a year? <input type="text" name="daysyear"></li>
<li>How many weeks are in a year? <input type="text" name="weeksyear"></li>
<li>What color is the sky? <input type="text" name="colorsky"></li>
<li>what is us president name? <input type="text" name="presidentname"></li>
<li>How many sides does a triangle have? <input type="text" name="sidestriange"></li>
<li>What is the first element on the periodic table of elements? <input type="text" name="firstelement"></li>
<li>Bees create what sweet substance? <input type="text" name="beesubstance"></li>
<li>How many stripes are displayed on the American flag?<input type="text" name="stripes"></li>
</ol>
<input type="submit" value="Done!"/>
</form>
<?php
$daysweek=$_POST['daysweek'];
$daysyear=$_POST['daysyear'];
$weeksyear=$_POST['weeksyear'];
$colorsky=$_POST['colorsky'];
$presidentname=$_POST['presidentname'];
$sidestriangle=$_POST['sidestriange'];
$firstelement=$_POST['firstelement'];
$beesubstance=$_POST['beesubstance'];
$stripes=$_POST['stripes'];
$wrong=0;
if($daysweek==7){
echo "1. you got the right answer: $daysweek<br/>";
}else {
    $wrong++;
    echo "number 1 is wrong<br/>";}

if($daysyear==365){
echo "2. you got the right answer: $daysyear<br/>";
}else {
    $wrong++;
    echo "number 2 is wrong<br/>";}

if($weeksyear==52){
echo "3. you got the right answer: $weeksyear<br/>";
}else {
    $wrong++;
    echo "number 3 is wrong<br/>";}

if($colorsky=='blue'){
echo "4. you got the right answer: $colorsky<br/>";
}else {
    $wrong++;
    echo "number 4 is wrong<br/>";}

if($presidentname=='obama'){
echo "5. you got the right answer: $presidentname</br>";
}else {
    $wrong++;
    echo "number 5 is wrong<br/>";}

if($sidestriangle==3){
echo "6. you got the right answer: $sidestriangle</br>";
}else {
    $wrong++;
    echo "number 6 is wrong<br/>";}

if($firstelement=='hydrogen'){
echo "7. you got the right answer: $firstelement<br/>";
}else {
    $wrong++;
    echo "number 7 is wrong<br/>";}

if($beesubstance=='honey'){
echo "8. you got the right answer: $beesubstance<br/>";
}else {
    $wrong++;
    echo "number 8 is wrong<br/>";}

if($stripes==13){
echo "9. you got the right answer: $stripes<br/>";
}else {
    $wrong++;
    echo "number 9 is wrong<br/>";}
echo "<h3>You have $wrong wrong answers</h3>";
?>
share|improve this question

migrated from stackoverflow.com Aug 20 '13 at 18:59

This question came from our site for professional and enthusiast programmers.
    
Security should be a far greater concern here. –  Rob Apodaca Aug 21 '13 at 2:19
    
The answer to #2 is incorrect on leap years, but I digress. –  icktoofay Aug 21 '13 at 4:11

2 Answers 2

up vote 1 down vote

Theres nothing particularly unefficient about what you have, but there are a couple of things. instead of storing the variables like

$daysweek=$_POST['daysweek'];

just leave them in $_POST like 

if($_POST['daysweek']==7){//..

and then instead of 

echo "1. you got the right answer: $daysweek<br/>";

you already know the answer is 7 so just

echo "1. you got the right answer: 7<br/>";

but based on the content of your application you are not yet in a place where you need to be worrying about optimization

share|improve this answer
1  
I think what Seth gave is nearly as far as it gets to save some resources, on the other hand it is a bad habit to use a variable if you are not 100% sure if it exists. –  Daniel P Aug 20 '13 at 3:48
    
if $_POST['daysweek'] does not exist, neither will $daysweek=$_POST['daysweek'] –  Seth Cohen Aug 20 '13 at 3:51
up vote 0 down vote

How about this.. To add or remove answers, all you would have to do is change the $answers array making the code easy to maintain.

$answers = [
            "daysweek" => 365,
            "daysyear" => 365
           ];

$answerNumber = 1;
$wrong = 0;

foreach($answers as $key => $value){

  if(checkAnswer($key)){
    echo "$answerNumber. you got the right answer: $value<br/>";

  } else {
    $wrong++;

  }

  $answerNumber++;

}

function checkAnswer($key){
  return $_POST[$key] == $answers[$key] ? TRUE : FALSE;
}
share|improve this answer
    
can u please explain me this line: return $_POST[$key] == $answers[$key] ? TRUE : FALSE; what does ? True: FALSE mean? –  Ham Dlink Aug 20 '13 at 4:15
    
@HamDlink ? : is a conditional operator more or like if else. This is same as writing if($_POST[$key] == $answers[$key]) { echo "TRUE"; } else { echo "FALSE"; } , the former one saves a lot of code writing but both serve the same purpose. –  Shankar Damodaran Aug 20 '13 at 8:25
2  
Why not remove the ? TRUE : FALSE completely? == returns a boolean anyway. –  icktoofay Aug 21 '13 at 4:12

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.