Proper use of if else if else

SFDonovan

I need to process grades. I want to follow the proper constuct of

if {
}
else if {
}else{
}

Do I have this right?

If they are first year grades and are using peer evaluations as part of the grade then check if they are complete.

// Find out which course year grades are for then process them accordingly.
if (($CourseInfo['Course_Year'] == 1) && ($CourseInfo['Peer_Eval'] == 'Yes')) {

//Do check for complete peer eval

if ($eval_total != $student_total) {
OpenTable();
echo"<tr><td>Not all Peer Evaluation are complete for this course.</td></tr>";
echo"<tr><td><a href='javascript:history.back()'>[ Back ]</a></td></tr>"; 
CloseTable();
die();
} else {
processM1Grades($Course_Number); 

//Now check if they are not using peer evaluations if so then go straight to processM1Grades

} else if (($CourseInfo['Course_Year'] == 1) && ($CourseInfo['Peer_Eval'] == 'No')) 
processM1Grades($Course_Number);
{

//if course year equals 2 they don't do peer evaluation at a course level but after each term so go process the grade.  

} else if ($CourseInfo['Course_Year'] == 2) {
processM2Grades($Course_Number);
}

ixalmida

Oops...I don't think you want to put an "elseif" after an "else". The "else" would cover every condition besides the "if", which would negate the need for another condition check. You might consider using the "switch()" statement instead if you can. The limitation in this case is that your conditions are all different, but you could use "switch(TRUE)" and put your conditions in the case section (not sure if that's as efficient).

I would discourage you from using that bracket placement. There's nothing more frustrating than trying to find a missing or extra bracket, and not having them line up in the code makes it all the more difficult. You should also use liberal indentation in your code to make it more readable.

In your example, the generally-accepted convention is:

if ($eval_total != $student_total)
{ 
  OpenTable(); 
  echo"<tr><td>Not all Peer Evaluation are complete for this course.</td></tr>"; 
  echo"<tr><td><a href='javascript:history.back()'>[ Back ]</a></td></tr>"; 
  CloseTable(); 
  die(); 
}
else if (($CourseInfo['Course_Year'] == 1) && ($CourseInfo['Peer_Eval'] == 'No')) processM1Grades($Course_Number); 
{ 
  //if course year equals 2 they don't do peer evaluation at a course level but after each term so go process the grade.   

}
else if ($CourseInfo['Course_Year'] == 2)
{ 
  processM2Grades($Course_Number); 
}
else
{
  processM1Grades($Course_Number); 
  //Now check if they are not using peer evaluations if so then go straight to processM1Grades 
}

I do it a little different, maybe because I'm a rebel but mostly because I like it better:

if ($eval_total != $student_total)
  { 
  OpenTable(); 
  echo"<tr><td>Not all Peer Evaluation are complete for this course.</td></tr>"; 
  echo"<tr><td><a href='javascript:history.back()'>[ Back ]</a></td></tr>"; 
  CloseTable(); 
  die(); 
  }
else if (($CourseInfo['Course_Year'] == 1) && ($CourseInfo['Peer_Eval'] == 'No')) processM1Grades($Course_Number); 
  { 
  //if course year equals 2 they don't do peer evaluation at a course level but after each term so go process the grade.   

  }
else if ($CourseInfo['Course_Year'] == 2)
  { 
  processM2Grades($Course_Number); 
  }
else
  {
  processM1Grades($Course_Number); 
  //Now check if they are not using peer evaluations if so then go straight to processM1Grades 
  }

This way each conditional statement stands out, but either way is preferrable.

laserlight

I would say that the main point is that you need to consistently indent your code. The placement of braces for your chosen indent style is fine. Once you do indent your code, it would be easier to spot such mistakes as the else if chained after the else, and even things like the misplacement of closing parenthesis.

ixalmida wrote:
You might consider using the "switch()" statement instead if you can. The limitation in this case is that your conditions are all different, but you could use "switch(TRUE)" and put your conditions in the case section (not sure if that's as efficient).

Personally I dislike that style. I would rather switch on some variable or more complex expression and then have the cases deal with constants.

ixalmida

I agree with both statements, laserlight. I can't say I haven't cheated and used the "switch(TRUE)" statement when it was convenient though. Sometimes it just works.

ixalmida

Looks like Weedpacket might have deleted his post, but he pointed out that switch(TRUE) doesn't save any processing over if...elseif...else because the condition has to be re-evaluated for each case. He is right, of course. The fact that I've used it a few times is more about form than function and there is probably a better way in most cases.

Or maybe I'm just having Visual Basic "SELECT...CASE" flashbacks. It's a sickness, I tell ya!

SFDonovan

I think I'm going to go with this. I think it is correct.

// Find out which course year grades are for then process them accordingly.
if ($CourseInfo['Course_Year'] == 1) && ($CourseInfo['Peer_Eval'] == 'Yes')) {
$check = checkM1PeerEval($Course_Number);
		if($check == 1){
		OpenTable();
		echo"<tr><td>Not all Peer Evaluation are complete for this course.</td></tr>";
		echo"<tr><td><a href='javascript:history.back()'>[ Back ]</a></td></tr>"; 
		CloseTable();
		}
	}else if (($CourseInfo['Course_Year'] == 1) && ($CourseInfo['Peer_Eval'] == 'No')) {
	processM1Grades($Course_Number);		
	 }else{
		processM2Grades($Course_Number);
		}

So my checkM1PeerEval would do the work from outside.

function checkM1PeerEval($Course_Number);
global $prefix, $db, $admin_file, $Course_Number;

//Get course info
$getcourseinfo = $db->sql_query("SELECT * FROM ".$prefix."_tl_session s
JOIN ".$prefix."_tl_courses c ON ( s.Course_Number = c.Course_Number ) 
WHERE c.Course_Number = '$Course_Number'");
$CourseInfo = $db->sql_fetchrow($getcourseinfo);
//Count number of students for that year. 
$countNumStudents = $db->sql_query("SELECT * FROM ".$prefix."_tl_students WHERE Class_Year = ".$CourseInfo['Course_Year']."");
$student_total = $db->sql_numrows($countNumStudents);
//Next count number of students who have submited evaluations.
$countNumEvals = $db->sql_query("SELECT DISTINCT rater FROM ".$prefix."_tl_peereval
WHERE Academic_Year = '$Academic_Year'
AND Course_Number = '$Course_Number'
GROUP BY rater");
$eval_total = $db->sql_numrows($countNumEvals);   

//Compare the two and see if they match.  If not then not all studetns have submitted evaluations.
if ($eval_total != $student_total) {
$PeerCheck = 1;
}else{
$PeerCheck = 0;
return $PeerCheck;
}

Thanks

laserlight

Instead of declaring those variables as global, turn them into parameters. If they really are some kind of configuration variables that should be globally available, group them together into an array, and maybe turn that array into a singleton.