Sometimes you need to modify a system to have it work a certain way for a certain customer. Maybe every time this particular customer buys one widget we actually credit their account with two widgets.

So how do you go about modifying your code to make that happen? You could just write in the code:

if($customer=="greedy_guys") {
	$numWidgets = $numWidgets * 2;
}

Which works fine. But then what if your boss wants an email notification to go out as well when this happens? Then your code becomes:

if($customer=="greedy_guys") {
	$numWidgets = $numWidgets * 2;
	$to = 'myboss@mycompany.com';
	$from = 'thesystem@mycompany.com';
	$subject = 'Greedy Guys got more widgets';
	$body = 'Hello Boss,';
	$body .= '<br><br>Just letting you know that Greedy Guys';
	$body .= 'Got '.$numWidgets.' for the price of '.$numWidgets/2;
	$body .= '<br><br>--<br>The System';
	email_send($to,$from,$subject,$body);
}

So then the boss works out a new deal and now they’re only going to get 2 widgets for the price of 1 if there’s some other situation that occurs, otherwise they get some other complicated formula. So you code becomes:

if($customer=="greedy_guys") {
	$originalNumWidgets = $numWidgets;
	if($thisScenario===true) {
		$numWidgets = $numWidgets * 2;
	} else {
		$numWidgets = $numWidets + round((dateFormat('now','j') % 6),0);
	}
	$to = 'myboss@mycompany.com';
	$from = 'thesystem@mycompany.com';
	$subject = 'Greedy Guys got more widgets';
	$body = 'Hello Boss,';
	$body .= '<br><br>Just letting you know that Greedy Guys';
	$body .= 'Got '.$numWidgets.' for the price of '.$originalNumWidgets;
	$body .= '<br><br>--<br>The System';
	email_send($to,$from,$subject,$body);
}

Now let’s say you’re making all of these changes and then some other changes need to happen on this same page that are critical that have nothing to do with this client but the changes you’ve made for this client haven’t been fully approved yet. What do you do?

Use included files and subcommands.

In our original file, let’s call it “checkout.php”, your code becomes:

if($customer=="greedy_guys") {
	$subcommand = "process_checkout";
	include 'customizations/greedy_guys/greedy.inc.php';
}

In customizations/greedy_guys/greedy.inc.php you have:

if($subcommand=='process_checkout') {
	if($customer=="greedy_guys") {
		$originalNumWidgets = $numWidgets;
		if($thisScenario===true) {
			$numWidgets = $numWidgets * 2;
		} else {
			$numWidgets = $numWidets + round((dateFormat('now','j') % 6),0);
		}
		$to = 'myboss@mycompany.com';
		$from = 'thesystem@mycompany.com';
		$subject = 'Greedy Guys got more widgets';
		$body = 'Hello Boss,';
		$body .= '<br><br>Just letting you know that Greedy Guys';
		$body .= 'Got '.$numWidgets.' for the price of '.$originalNumWidgets;
		$body .= '<br><br>--<br>The System';
		email_send($to,$from,$subject,$body);
	}
}

Now you can go nuts with the customizations for that client and not have it muck up your original checkout page so much.

Important takeaways here: Use included subcommands like this if you need to modify existing page variables on the original system inline and/or have customizations that lend themselves well to this style of coding.

August 20th, 2013

Posted In: Controllers, Syntax

Leave a Comment

When writing code, if you find yourself copy/pasting a chunk of code from one place to another that is something other than “include: ‘header.php'” and “include: ‘footer.php'”, you might want to double-check what you’re doing.

Instead of copy/pasting the code that displays a widget for example, make that code into a function. Call is something like “createWidget()” and then reference that in your code.

In a recent example, I needed to show a notes list on two different view pages. The code was the exact same between the two so I created a function to do it instead. That function looked something like this:

function qt_getNotes($params=array()) {
	$id = $params['id'];
	$query = "SELECT * FROM table WHERE id='".encodeSQL($id)."'";
	$result = getMySQLData($query);
	$out = '';
	while($row = @mysql_fetch_assoc($result)) {
		$out .= 'Author: '.$row['author'].'<br>';
		$out .= $row['body'];
		$out .= '<hr>';
	}
	return $out;
}

if($command=='adminView') {
	echo "Show admin stuff here.";
	echo qt_getNotes(array('id'=>$id));
	echo "Show admin footer here.";
}

if($command=='workerView') {
	echo "Show worker stuff here.";
	echo qt_getNotes(array('id'=>$id));
	echo "Show worker footer here.";
}

This is obviously a pretty simple example in terms of visual sophistication, I just wanted to show how it could be used.

Why did I preface my function with “gt_”? Because this function was designed for use within our “Quality Ticket” area and would not be used outside of that area. Also, I didn’t want to try to double-define any function that may have been previously called “getNotes”.

And why did I use an entire array for passing the $id? Because you’ll likely need to pass other variables to that function at some point in time (like whether or not to show a delete button) and you can now add those as parameters in the array rather than defining additional in-line inputs to the function. Keeps it clean when you have a variable number of possible inputs.

You’ll also notice I used “$out” to store the view components of this rather than just echo’ing it straight onto the page. This comes in handy if you need to store the returned HTML or have it displayed in an email or something like that.

The benefits of using a function like this is that you can change the code once and it effectively is updated in multiple places. You also have your own variable namespace within the function so you can re-use variables like $id, $data, $result, and $row which are commonly used in the body of a page without concern that you’ll overwrite an existing loop you might be in.

The downsides of a function are that they don’t inherit page variables and must be carefully defined once in your system. Variable inheritance is helpful if you have usernames, permissions, or display variables on your page that you would want your function to utilize. To use those variables, you’d need to pass them in your $params array.

August 20th, 2013

Posted In: Functions, Syntax

Leave a Comment

If you’re writing a complex piece of logic, make it easier for yourself later. I’m going to provide two code examples below and I’d like you to choose which one you’re rather work with.

Hint: There’s generally no need to nest if statements more than 1 or 2 levels deep. Use additional variables to guide your code through how to evaluate itself.

Version 1:

function isCandidateProfileVisible($params=array()) {
	
	if($this->profileVisibleData['showLinkToApplicant']===NULL) {
			
		
		$isDTstaff = $_SESSION['client']['isDTstaff'];
		$id_company = $_SESSION['client']['id_company'];
		
		$id_user = $_SESSION['client']['id_contact'];
		if($id_user=='') {
			$id_user = $_SESSION['session_id_user'];
		}
		
		if($params['isDTstaff']===true) {
			$isDTstaff = true;
		}
		
		$showLinkToApplicant = false;
		$howAccessGranted = NULL;
		
		if($isDTstaff) {
			$showLinkToApplicant = true;
			$howAccessGranted = "DT Staff";
		} else {
			if(strtolower($this->getOwner())=="client") {
				$showLinkToApplicant = true;
				$howAccessGranted = "owner=client";
			} else {
				if($this->getActivityStage()>=2) {
					$showLinkToApplicant = true;
					$howAccessGranted = "getActivityStage";
				} else {
					$currentEmployee = strtolower($this->getCurrentEmployee());
					if($currentEmployee=='yes' || $currentEmployee=='alumni') {
						$showLinkToApplicant = true;
						$howAccessGranted = "current/alumni employee";
					} else {
						$accessObj = $params['accessObject'];
						if(is_object($accessObj) && $accessObj->id!='') {
							$viewApps = $accessObj->getViewApps();
						} else {
							$query = "SELECT viewApps FROM access 
										WHERE id_company='".$id_company."' 
											AND id_contact='".$_SESSION['client']['id_contact']."'";
							$viewApps = getMySQLValue($query,true);
						}
						if(strtolower($viewApps)=="all") {
							$showLinkToApplicant = true;
							$howAccessGranted = "viewApps=all";
						} else {
							$jobObj = $params['jobObj'];
							$jobviewapps = false;
							
							$id_job = $params['id_job'];
							if(!is_object($jobObj) && $id_job!='') {
								$jobObj = new job($id_job);
							}
							
							if($id_job=='' && is_object($jobObj)) {
								$id_job = $jobObj->id;
							}
							
					
							$jobAccessObj = $params['jobAccessObj'];
							if(is_object($jobAccessObj) && $jobAccessObj->id!='') {
								$jobviewapps = $jobAccessObj->getViewApps();
							} else if($id_job!='') {
								$id_user = $_SESSION['session_id_user'];
								$query = "SELECT viewapps FROM jobaccess 
											WHERE id_job='".$id_job."' 
											AND id_contact='".$id_user."'";
								$jobviewapps = getMySQLValue($query,true);
							}
							
							if($jobviewapps!==false && strtolower($jobviewapps)=="all") {
								$showLinkToApplicant = true;
								$howAccessGranted = "jobviewapps=all";
							} else {
								$id_most_recent_job = $this->getMostRecentIDjob();
								if($id_most_recent_job>0) {
									$query = "SELECT Status_ActiveOrPending FROM jobs 
												WHERE id='".$id_most_recent_job."'";
									$Status_ActiveOrPending = getMySQLValue($query,true);
									if($Status_ActiveOrPending!='Yes') {
										$showLinkToApplicant = true;
										$howAccessGranted = "most recent job (".$id_most_recent_job.") ";
										$howAccessGranted .= "is not active or pending";
									} else {
										$query = "SELECT viewapps FROM jobaccess 
													WHERE id_job='".$id_most_recent_job."' 
														AND id_contact='".$id_user."'";
										$jobviewapps = getMySQLValue($query,true);
										if(strtolower($jobviewapps)=="all") {
											$showLinkToApplicant = true;
											$howAccessGranted = "jobviewapps=all";
										} else {
											if(is_object($jobObj) && $jobObj->id>0) {
				
												if(	strtolower($jobObj->getFlagAppAccessClient())=="view" 
													|| strtolower($jobObj->getFlagAppAccessClient())=="edit") {
													
													$showLinkToApplicant = true;
													$howAccessGranted = "getFlagAppAccessClient=";
													$howAccessGranted .= $jobObj->getFlagAppAccessClient()." on ".$jobObj->id;
												} else {
													$query = "SELECT r.id
																FROM responses AS r 
																LEFT JOIN jobs AS j ON j.id=r.id_job 
																WHERE r.id_applicant='".$this->id."'
																	AND 
																		(
																			j.flag_masterlist='client'
																			OR
																			j.title='Future Opportunities'
																		)
																LIMIT 1";
													$id_self_service_response = getMySQLValue($query,true);
													if($id_self_service_response) {
														$showLinkToApplicant = true;
														$howAccessGranted = "self service job found via response record: ";
														$howAccessGranted .= $id_self_service_response;
													} else {
														if($this->getOwner()=='') {
															$showLinkToApplicant = true;
															$howAccessGranted = "owner=NULL";
														} else {
															$query = "SELECT f.id
																		FROM flags AS f 
																		LEFT JOIN jobs AS j ON j.id=f.id_job 
																		WHERE f.id_applicant='".$this->id."'
																			AND 
																				(
																					j.flag_masterlist='client'
																					OR
																					j.title='Future Opportunities'
																				)
																		LIMIT 1";
															$id_self_service_response = getMySQLValue($query,true);
															if($id_self_service_response) {
																$showLinkToApplicant = true;
																$howAccessGranted = "self service job found via flag record: ";
																$howAccessGranted .= $id_self_service_response;
															} else {
																if($this->getIDreferrer()=='15') {
																	$showLinkToApplicant = true;
																	$howAccessGranted = "Candidate referrer is 'Our Corporate Website'";
																} else {
																	$query = "SELECT r.id
																				FROM responses AS r 
																				WHERE r.id_applicant='".$this->id."'
																					AND id_referrer='15'
																				LIMIT 1";
																	$id_website_application = getMySQLValue($query,true);
																	if($id_website_application) {
																		$showLinkToApplicant = true;
																		$howAccessGranted = "'our corporate website' referrer via response record: ";
																		$howAccessGranted .= $id_website_application;
																	} else {
																		$query = "SELECT id FROM activity 
																					WHERE id_applicant='".$this->id."'
																						AND activity_type='access'";
																		$id_access_activity = getMySQLValue($query,true);
																		if($id_access_activity) {
																			$showLinkToApplicant = true;
																			$howAccessGranted = "access activity record found: ".$id_access_activity;
																		} else {
																			$howAccessGranted = "Not Granted";
																		}
																	}
																}
															}
														}
													}
												}
											}
										}
									}
								}
							}
						}
					}
				}
			}
		}

		$this->profileVisibleData['showLinkToApplicant'] = $showLinkToApplicant;
		$this->profileVisibleData['howAccessGranted'] = $howAccessGranted;
	
	}
	
	
	if($params['returnKind']=="" || $params['returnKind']=="boolean") {
		return $this->profileVisibleData['showLinkToApplicant'];
	} else if($params['returnKind']=="howAccessGranted" || $params['returnKind']=="debug") {
		return $this->profileVisibleData['howAccessGranted'];
	}
}

Version 2:

function isCandidateProfileVisible($params=array()) {
	
	if($this->profileVisibleData['showLinkToApplicant']===NULL) {
			
		
		$isDTstaff = $_SESSION['client']['isDTstaff'];
		$id_company = $_SESSION['client']['id_company'];
		
		$id_user = $_SESSION['client']['id_contact'];
		if($id_user=='') {
			$id_user = $_SESSION['session_id_user'];
		}
		
		if($params['isDTstaff']===true) {
			$isDTstaff = true;
		}
		
		$showLinkToApplicant = false;
		$howAccessGranted = NULL;
		
		$useInternalDTrules = false;
		if($isDTstaff && $this->getIDclient()=='9786') {
			$useInternalDTrules = true;
		}
		

		if(!$showLinkToApplicant && $isDTstaff) {
			$showLinkToApplicant = true;
			$howAccessGranted = "DT Staff";
		}

		if(!$showLinkToApplicant && strtolower($this->getOwner())=="client") {
			$showLinkToApplicant = true;
			$howAccessGranted = "owner=client";
		}
		
		if(!$showLinkToApplicant && $this->getActivityStage()>=2) {
			$showLinkToApplicant = true;
			$howAccessGranted = "getActivityStage";
		}
		
		if(!$showLinkToApplicant) {
			$currentEmployee = strtolower($this->getCurrentEmployee());
			if($currentEmployee=='yes' || $currentEmployee=='alumni') {
				$showLinkToApplicant = true;
				$howAccessGranted = "current/alumni employee";
			}
		}

		if(!$showLinkToApplicant) {
			$accessObj = $params['accessObject'];
			if(is_object($accessObj) && $accessObj->id!='') {
				$viewApps = $accessObj->getViewApps();
			} else {
				$query = "SELECT viewApps FROM access 
							WHERE id_company='".$id_company."' 
								AND id_contact='".$_SESSION['client']['id_contact']."'";
				$viewApps = getMySQLValue($query,true);
			}

		}

		if(!$showLinkToApplicant && strtolower($viewApps)=="all") {
			$showLinkToApplicant = true;
			$howAccessGranted = "viewApps=all";
		}
		
		// special rules for DT persons viewing DT candidates (internal jobs, etc.)
		if($useInternalDTrules) {
			// the above rules don't apply to this situation so we should override any previous data
			$showLinkToApplicant = false;
			$howAccessGranted = NULL;
		}
		

		if(!$showLinkToApplicant) {
			$jobObj = $params['jobObj'];
			$jobviewapps = false;
			
			$id_job = $params['id_job'];
			if(!is_object($jobObj) && $id_job!='') {
				$jobObj = new job($id_job);
			}
			
			if($id_job=='' && is_object($jobObj)) {
				$id_job = $jobObj->id;
			}
			
	
			$jobAccessObj = $params['jobAccessObj'];
			if(is_object($jobAccessObj) && $jobAccessObj->id!='') {
				$jobviewapps = $jobAccessObj->getViewApps();
			} else if($id_job!='') {
				$id_user = $_SESSION['session_id_user'];
				$query = "SELECT viewapps FROM jobaccess 
							WHERE id_job='".$id_job."' 
							AND id_contact='".$id_user."'";
				$jobviewapps = getMySQLValue($query,true);
			}
				
	
			if($jobviewapps!==false && !$showLinkToApplicant && strtolower($jobviewapps)=="all") {
				$showLinkToApplicant = true;
				$howAccessGranted = "jobviewapps=all";
			}
			
			
			
			if(!$showLinkToApplicant) {
				$id_most_recent_job = $this->getMostRecentIDjob();
				if($id_most_recent_job>0) {
					$query = "SELECT Status_ActiveOrPending FROM jobs 
								WHERE id='".$id_most_recent_job."'";
					$Status_ActiveOrPending = getMySQLValue($query,true);
					if($Status_ActiveOrPending!='Yes') {
						$showLinkToApplicant = true;
						$howAccessGranted = "most recent job (".$id_most_recent_job.") ";
						$howAccessGranted .= "is not active or pending";
					} else {
						$query = "SELECT viewapps FROM jobaccess 
									WHERE id_job='".$id_most_recent_job."' 
										AND id_contact='".$id_user."'";
						$jobviewapps = getMySQLValue($query,true);
						if(strtolower($jobviewapps)=="all") {
							$showLinkToApplicant = true;
							$howAccessGranted = "jobviewapps=all";
						}
					}
				}
			}
			
			
			if(!$showLinkToApplicant && is_object($jobObj) && $jobObj->id>0) {
				
				if(	strtolower($jobObj->getFlagAppAccessClient())=="view" 
					|| strtolower($jobObj->getFlagAppAccessClient())=="edit") {
					
					$showLinkToApplicant = true;
					$howAccessGranted = "getFlagAppAccessClient=";
					$howAccessGranted .= $jobObj->getFlagAppAccessClient()." on ".$jobObj->id;
				}
			}
		}
		
		
		if(!$showLinkToApplicant) {
			// check to see if the app has applied to any self-service jobs
			$query = "SELECT r.id
						FROM responses AS r 
						LEFT JOIN jobs AS j ON j.id=r.id_job 
						WHERE r.id_applicant='".$this->id."'
							AND 
								(
									j.flag_masterlist='client'
									OR
									j.title='Future Opportunities'
								)
						LIMIT 1";
			$id_self_service_response = getMySQLValue($query,true);
			if($id_self_service_response) {
				$showLinkToApplicant = true;
				$howAccessGranted = "self service job found via response record: ";
				$howAccessGranted .= $id_self_service_response;
				if($this->getIDclient()=='104093') {
					$subcommand="correct_hm_access";
					require config_include_path_constant.'client/reports/104093/dr.inc.php';
				}
			}
		}
		
		if(!$showLinkToApplicant && $this->getOwner()=='') {
			$showLinkToApplicant = true;
			$howAccessGranted = "owner=NULL";
		}
		
		if(!$showLinkToApplicant) {
			// check to see if the app has been attached to any self-service jobs
			$query = "SELECT f.id
						FROM flags AS f 
						LEFT JOIN jobs AS j ON j.id=f.id_job 
						WHERE f.id_applicant='".$this->id."'
							AND 
								(
									j.flag_masterlist='client'
									OR
									j.title='Future Opportunities'
								)
						LIMIT 1";
			$id_self_service_response = getMySQLValue($query,true);
			if($id_self_service_response) {
				$showLinkToApplicant = true;
				$howAccessGranted = "self service job found via flag record: ";
				$howAccessGranted .= $id_self_service_response;
				if($this->getIDclient()=='104093') {
					$subcommand="correct_hm_access";
					require config_include_path_constant.'client/reports/104093/dr.inc.php';
				}
			}
		}
		
		/*
			2013-06-03: Jay says that all candidates that come from the "Our Corporate Website" 
			should be visible by the client.
		*/
		if(!$showLinkToApplicant) {
			if($this->getIDreferrer()=='15') {
				$showLinkToApplicant = true;
				$howAccessGranted = "Candidate referrer is 'Our Corporate Website'";
			}
		}
		
		if(!$showLinkToApplicant) {
			// check to see if the app has applied to any self-service jobs
			$query = "SELECT r.id
						FROM responses AS r 
						WHERE r.id_applicant='".$this->id."'
							AND id_referrer='15'
						LIMIT 1";
			$id_website_application = getMySQLValue($query,true);
			if($id_website_application) {
				$showLinkToApplicant = true;
				$howAccessGranted = "'our corporate website' referrer via response record: ";
				$howAccessGranted .= $id_website_application;
			}
		}
		
		if(!$showLinkToApplicant) {
			// check to see if any activity "access" records have been created
			// this may need to be modified later to check for job-specific
			// restrictions and/or contact-specific restrictions
			$query = "SELECT id FROM activity 
						WHERE id_applicant='".$this->id."'
							AND activity_type='access'";
			$id_access_activity = getMySQLValue($query,true);
			if($id_access_activity) {
				$showLinkToApplicant = true;
				$howAccessGranted = "access activity record found: ".$id_access_activity;
			}
			
		}
		
			
		
		if($howAccessGranted=='') {
			$howAccessGranted = "Not Granted";
		}
		
		$this->profileVisibleData['showLinkToApplicant'] = $showLinkToApplicant;
		$this->profileVisibleData['howAccessGranted'] = $howAccessGranted;
	
	}
	
	
	if($params['returnKind']=="" || $params['returnKind']=="boolean") {
		return $this->profileVisibleData['showLinkToApplicant'];
	} else if($params['returnKind']=="howAccessGranted" || $params['returnKind']=="debug") {
		return $this->profileVisibleData['howAccessGranted'];
	}
}

August 12th, 2013

Posted In: Controllers, Syntax

Leave a Comment

I’m a fanatic about using tabs properly in code. It can make the difference between easily-readable code and a nightmare.

Of the two code segments below, which would you rather work with?

$out .= selectList(array('name'=>'type',
'id'=>'typeSelection',
'elements'=>array('Unspecified'=>'- please select -',
'questions'=>'General Questions',
'quote'=>'Request Info/Demo/Quote',
'order'=>'Place Order',
'comment'=>'Feedback/Comment'),
));
$out .= selectList(
			array(
				'name'=>'type',
				'id'=>'typeSelection',
				'elements'=>array(
							'Unspecified'	=> '- please select -',
							'questions'		=> 'General Questions',
							'quote'			=> 'Request Info/Demo/Quote',
							'order'			=> 'Place Order',
							'comment'		=> 'Feedback/Comment'),
				));

August 7th, 2013

Posted In: Syntax

Leave a Comment

One of my favorite tricks lately is storing an array of data in a serialized format and writing it to a database.  This is really handy when you have an arbitrary set of information that needs to be stored that doesn’t really fit anywhere else and does not need to be searchable or sortable.

An example I had recently required me to store whether or not 4 checkboxes were checked or not on a record.  I could have created 4 additional fields or a related table where it would create 4 additional records per original object, or I could just store this as an array and serialize it and be done with it.  Since I already had a “generic_data” table, I just added this to that table and in a couple minutes I was done.  Awesome.

How does it work?  Well, something like this (showing ideas here, not full code chunks):

if($command=='view') {
	$raw = $obj->getSerializedData(); // read data from the database
	if($raw!='') {
		$myArray = unserialize($raw);
	}
	if(!is_array($myArray)) {
		$myArray = array();
	}
	// do whatever display is needed with $myArray
}
if($command=='save') {
	$myArray = $_REQUEST['x']; // assuming $_REQUEST['x'] is an array
	$obj->setSerializedData(serialize($myArray)); // write to the database
	$obj->Save();
}

June 7th, 2013

Posted In: Shortcuts, Syntax

Leave a Comment

Make your code easy to read. Trust me, it’ll help you and everyone else that has to work with your code later.  Try to avoid excess layers in your code, and make your variables, functions and classes all have reasonable names.

Point in case: don’t have every single page in your site be based on the index.php page.  Just because you can abstract out the views and then include them later doesn’t mean you should.  Go ahead, have an about.php page, a contact.php page, a download.php page.  The next programmer that comes along will appreciate it.

Programmers and DBAs (or just those that like to work with databases) have a tendency to go a little overboard on normalization.  It’s OK if you copy/paste a function here or there or have the same variable declared in a couple different spots – if it makes your code easier to read, easier to debug, and simpler for anyone else to handle, then it’s good code.

Avoid hidden functions and features.  Don’t hide a function inside of a header file if it’s being used in your model code.  I don’t want to have to do a grep to try to figure out where something lives.  If you have a bunch of functions that live by themselves, make a functions.inc.php file.  It’ll be easy to find and easy to work with.

Name your files, variables, functions, methods, classes with worthwhile and meaningful names.  Don’t go nuts spending hours trying to come up with the perfect name for things, but simple things like resetting a password could be called $userObj->sendPasswordReset(); instead of $u->pr(); or $user->sendpasswordresetnowviaemail().

Try to include some kind of a “common” file in your system.  It will help you and other programmers figure out where key things live, like functions files, class files, and any database connections or custom settings.  Then make sure that common file is included from all other pages that are executed.  Makes live easy.

Just remember when writing code: if I had to tell another programmer what I did and why, would I look foolish?

March 29th, 2013

Posted In: Syntax

Leave a Comment