I think my classes have to much responsibility

fastsol · July 1, 2015

So I'm fairly confident that I understand the idea of having a class with minimal responsibility but I struggle with concept of having tons of little classes instead of a few larger ones that encompass more responsibility. Just seems a bit silly to break it down into several classes when it could be just one.

I had 2 function pages that held a few dozen regular functions that all relate to a specific aspect of my site, hence why they were grouped among these 2 pages. I am trying to organize them into classes and make it easier to expand in the future and eliminate duplicate code from the functions, like always setting

$db = DB::getInstance();

to be able to use my db class. So now I have 13 new classes with functions that relate to the class. I think that is a good start, but I feel like it's not enough, or not enough in the proper direction of how a class should act.

Here is a sample of a new class. Can you take a look at it and give advice on how you might change or extract its responsibility to an additional class/es, or if it's just fie the way it is. There are a few functions in these classes that I'm really not sure how to split apart any more, simply cause they are form processing functions that validate, set errors, return messages, email and do db stuff. But I have classes in those functions that are already doing things separately and the flow of logic would be hard to break apart and maintain the functions goal of form processing quickly and efficiently in one spot.

QuoteResponse.class.php

<?php

class QuoteResponse extends QuoteSystem
{
	public function __construct()
	{
		parent::__construct();
	}
	
	public function verifyReferralCode($ref_code)
	{
		return $this->db->query("SELECT COUNT(`id`) FROM `".QUOTE_RESPONSES."` WHERE `id` = {$ref_code}");
	}
	
	public function checkQuoteExists($q_id)
	{
		return $this->db->query("SELECT `id` FROM `".QUOTE_RESPONSES."` WHERE `request_id` = ".$q_id);
	}
	
	// Gets info for the given quote-id and c code for a client view
	public function getQuote()
	{
		$id = (int)Input::val('quote-id|g');
		$code = sanitize(Input::val('c|g'));
		$get = $this->db->prepare("SELECT `".QUOTE_RESPONSES."`.*,
								   `".QUOTE_ITEMS."`.*,
								   `".PRODUCTS."`.*,
								   `".QUOTE_DEPOSITS."`.`id` AS `deposit_id`,
								   `".QUOTE_DEPOSITS."`.`product_id` AS `deposit_prod`,
								   `".QUOTE_DEP_RESP."`.`id` as `dep_resp_id`,
								   `".QUOTE_REQUESTS."`.`cookie_code`,
								   `".QUOTE_REQUESTS."`.`id` as `orig_quote_id`,
								   ".CONCAT_VEHICLE." AS `vehicle`,
								   `".QUOTE_REQUESTS."`.`vehicle_sufix`,
								   `".QUOTE_REQUESTS."`.`name`,
								   `".SCH."`.`schedule_id`,
								   `".SCH."`.`install_time`,
								   `".SCH."`.`cancelled` 
							  FROM `".QUOTE_RESPONSES."`
							  INNER JOIN `".QUOTE_ITEMS."` ON `".QUOTE_ITEMS."`.`response_id` = `".QUOTE_RESPONSES."`.`id`
							  INNER JOIN `".PRODUCTS."` ON `".PRODUCTS."`.`id` = `".QUOTE_ITEMS."`.`product_id`
							  INNER JOIN `".QUOTE_REQUESTS."` ON `".QUOTE_REQUESTS."`.`id` = `".QUOTE_RESPONSES."`.`request_id`
							  ".VEHICLE_JOIN."
							  LEFT JOIN `".QUOTE_DEPOSITS."` ON `".QUOTE_DEPOSITS."`.`q_id` = `".QUOTE_RESPONSES."`.`id`
							  LEFT JOIN `".QUOTE_DEP_RESP."` ON `".QUOTE_DEP_RESP."`.`deposit_id` = `".QUOTE_DEPOSITS."`.`id`
							  LEFT JOIN `".SCH."` ON `".SCH."`.`deposit_id` = `".QUOTE_DEPOSITS."`.`id`
							  WHERE `".QUOTE_RESPONSES."`.`id`=$id AND `code`= ?");
							  
		$get->execute(array($code));
		
		return $get->fetchAll();
	}

	// Validates the quote
	public function processQuote()
	{
		//diagDiv($_POST);
		$qi = new QuoteItems;
		$i = new Input;
		
		$resp_id = $i->get('response-id', '', 'int');
		$q_id = $i->post('quote_id', '', 'int');
		
		$name = $i->post('name', TRUE);
		$car_name = $i->post('vehicle_make_name', TRUE);
		$car_model = $i->post('vehicle_model_name', TRUE);
		
		$make_id = $i->post('make_id', '', 'int');
		$model_id = $i->post('model_id', '', 'int');
		$year = $i->post('year', '', 'int');
		
		$car_sufix = $i->post('vehicle_sufix', TRUE);
		$ebody = $i->post('email_body', TRUE);
		$sig = $i->post('signature');
		$unit = $i->post('units');
		$notes = $i->post('notes', TRUE);
		$email = $i->post('email', TRUE);
		$code = $i->post('code', TRUE);
		$sms = ($i->post('send_sms', '', 'int') == 1) ? TRUE : FALSE;
		$phone = $i->post('phone', TRUE);
		//$bypass = (isset($_POST['bypass'])) ? sanitize($_POST['bypass']) : '';
		
		$fields = array('name' => $name, 'vehicle_make_name' => $car_name, 'vehicle_model_name' => $car_model, 'vehicle_sufix' => $car_sufix, 'email_body' => $ebody, 'signature' => $sig, 'units' => $unit, 'notes' => $notes, 'email' => $email, "code" => $code, "quote_id" => $q_id, 'send_sms' => $sms, 'phone' => $phone, 'vehicle_make_id' => $make_id, 'vehicle_model_id' => $model_id, 'vehicle_year_id' => $year);
		$notes = escape($notes);
		
		$required = array("name", "email_body", "email", "make_id", "model_id", "year");
		requiredFields($i->all('post'), $required);
		
		if(empty($resp_id))
		{
			$check = $this->checkQuoteExists($q_id);
			if($check->rowCount() > 0)
			{ Errors::set('A Quote has already been made for this request.'); }
		}
		
		if(Errors::has()){}
		else
		{		
			nameLength($name, 2, 45);
			//maxStringLength($car, 100, 'The vehicle cannot exceed 100 characters.');
			minStringLength($ebody, 2, 'The email body must be at least 2 characters in length.');
			//minStringLength($sig, 2, 'The signature must be at least 2 characters in length.');
			validEmail($email);
		
			$email_units='';
			foreach($unit as $key => $val)
			{
				if(!empty($val['price']))
				{
					$diff = $val['original_price'] - $val['sale_price'];
					$email_units .= $key.' - $'.number_format($val['price'], 2);
					$email_units .= ($val['on_sale'] == TRUE) ? ' (while on sale, add $'.$diff.' after sale ends)' : '';
					$email_units .= ' +tax';
					$email_units .= (!empty($val['note'])) ? " (".$val['note'].")\r\n" : "\r\n"; 
				}
			}
			
			if(empty($email_units))
			{ Errors::set('Please select products for the quote.'); }
		}
		
		if(Errors::has())
		{ echo Errors::output(); }
		else
		{
			$now = time();		
			$units_text = "Prices include all parts and labor:\r\n".$email_units;
			$where = '';
			$date_sent = '';
			$date_modified = NULL;
			
			// INSERT/UPDATE QUOTE_RESPONSES
			$params = [
				'request_id' => [$q_id, 'INT'],
				/*'name' => $name,*/
				/*'vehicle' => $car,*/
				/*'vehicle_sufix' => $car_sufix,*/
				'date_modified' => NULL,
				'body' => $ebody,
				//'signature' => $sig,
				'code' => $code,
				'notes' => $notes,
				'unit_pricing' => $units_text
			];
			
			if($i->get('action') == "add")
			{			
				$params['code'] = randomCode(20);
				$code = $params['code'];
				
				$set = $this->db->insert(QUOTE_RESPONSES, $params);
			}
			elseif($i->get('action') == "edit")
			{
				$params['date_modified'] = date("Y-m-d H:i:s");
				$set = $this->db->update(QUOTE_RESPONSES, $params, '`id` = '.$resp_id, 1);
			}
			
			$last_id = ($i->get('action') == "add") ? $set->lastInsertId() : $resp_id;
			///////////// END ///////////////////////////////////
			
			// UPDATE REFERRALS
			if($i->get('action') == "add")
			{ (new QuoteReferral)->setQuoteId($last_id, $q_id); }
			//////////// END ////////////////////// 
			
			// Clear QUOTE_ITEMS		
			if($i->get('action') == "edit")
			{ $qi->deleteQuoteItems($resp_id); }
			/////////// END //////////////////////
			
			// UPDATE QUOTE_REQUESTS with posted vehicle info.
			$params = [
				'vehicle_make_id' => [$make_id, 'INT'],
				'vehicle_model_id' => [$model_id, 'INT'],
				'vehicle_year_id' => [$year, 'INT'],
				'vehicle_sufix' => $car_sufix
			];
			
			(new QuoteRequest)->updateById($params, $q_id);
			///////// END //////////////////////////////////////////
			
			// SET new QUOTE_ITEMS
			foreach($unit as $key => $val)
			{
				$params = [];
				
				if(!empty($val['price']))
				{
					$note = (!empty($val['note'])) ? sanitize($val['note']) : '';
					$unit_price = addSalesTax($val['price'], Config::get('tax_rate'));
					$add_amt = ($val['on_sale'] == TRUE) ? (int)$val['price'] - (int)$val['sale_price'] : (int)$val['price'] - (int)$val['original_price'];
					
					$params = [
						'item' => $key,
						'quote_price' => [$val['price'], 'INT'],
						'response_id' => [$last_id, 'INT'],
						'product_id' => [$val['product_id'], 'INT'],
						'item_note' => $note,
						'add_amt' => [$add_amt, 'INT']
					];
					
					$qi->insertQuoteItems($params);
				}
			}
			////////// END ///////////////////////////////
			
			if($i->get('action') == "add")
			{
				// Build Email to customer and admin
				// OLD EMAIL FORMAT //////////////////////////////////////////////////////////////////
				/*$ebody .= $units_text;
				$link  = "If this link is not clickable please copy and paste it into your browser address bar to confirm you have received your quote and to make your deposit.\r\n";
				$link_url = 'http://remotelystartedmn.com/quotes.php?quote-id='.$last_id.'&c='.$code;
				$sig .= "\r\n\r\n".$link.$link_url.Config::get('quote_signature_part2');
	*/			//////////////////////////////////////////////////////////////////////////////////////
				
				// OLD Signature storage, now signature is built in the composeQuote()
				/*$link  = "Please visit the link below to see your pricing options. If the link is not clickable please copy and paste it into your browser address bar.\r\n";
				$link_url = 'http://remotelystartedmn.com/quotes.php?quote-id='.$last_id.'&c='.$code;
				$email_link_url = $link_url.'&e=1';
				$signature = $link.$email_link_url."\r\n\r\n".$sig.Config::get('quote_signature_part2');*/
				
				$link_url = 'http://remotelystartedmn.com/quotes.php?quote-id='.$last_id.'&c='.$code.'&direct';
				$email_link_url = $link_url.'&e=1';
				
				//$body  = 'Hello '.$name.',';
				//$body .= "\r\n".$ebody."\r\n".$sig; // <--- OLD EMAIL FORMAT //
				//$body .= "\r\n".$ebody."\r\n".$signature.thanksSignature();
		
				$set = $this->db->update(QUOTE_RESPONSES, [/*'signature' => $signature, */'quote_link' => $email_link_url], '`id`= '.$last_id);
				//$set->execute(array($sig)); // <--- OLD EMAIL FORMAT //
				//$set->execute(array($signature));
				
				$qid = $i->get('quote-id', '', 'int');
				(new QuoteRequest)->updateResponseType($qid);
				
				// Attempt to send the quote email	
				$send = $this->composeQuote($last_id);
				if($send === TRUE)
				{ 
					$message = 'Email Sent';
					$sms_response = TRUE;
					
					if($sms === TRUE && !empty($phone))
					{
						$args = array('username' => Config::get('ez_sms_username'),
									  'password' => Config::get('ez_sms_password'),
									  'subject' => '',
									  'message' => "Remote start quote is ready! ".$link_url."&t=1 Reply texts won't be received",
									  'phone' => $phone);
						$sms_response = sendSMS($args);
						$message .= '<br>Text Message Sent';
					}
					if($sms_response === TRUE){ echo formComplete($message, TRUE); }
					else{ echo 'Text Message Failed:<br>'.$sms_response; } 
				}
				elseif($send === FALSE)
				{ formErrors(array('Something went wrong.')); }
				///////////////////////////////////////////////
			}
			elseif($i->get('action') == "edit")
			{ echo formComplete('Quote Updated', TRUE); }
	
			unset($_SESSION['quote_original_info']);
			$fields='';
		}
		
		return $fields;
	}

	// Updates the purchase status for each posted
	public function changePurchase($tbl)
	{
		$key="";
		foreach(Input::val('bought|p') as $c[$key] => $value)
		{ 
			$id = (int)$c[$key];
			$this->db->query("UPDATE `".$tbl."` SET `purchased` = 1 WHERE `id` = '".$id."'"); 
			
			if(isset($_POST['miles'][$id]) && $_POST['miles'][$id] > 0)
			{
				$miles = (float)$_POST['miles'][$id];
				(new MilesDriven)->setMiles($id, $miles);
			}
			
			// Send completed email
			if(Config::get('send_email_when_completed') === TRUE)
			{
				$get = $this->db->query("
					SELECT
						`".QUOTE_REQUESTS."`.`name`,
						`".QUOTE_REQUESTS."`.`email`,
						`".QUOTE_REQUESTS."`.`gift_date`,
					  	`".QUOTE_REQUESTS."`.`gift_name`,
					  	`".QUOTE_REQUESTS."`.`gift_email`,
						".CONCAT_VEHICLE." AS `vehicle`,
						`".QUOTE_REQUESTS."`.`vehicle_sufix`,
						
						qr1.video,
						qr1.screen_shot,
						qr1.description,
						
						qr2.video as `specific_video`,
						qr2.screen_shot as `specific_screen_shot`,
						qr2.description as `specific_description`,
						
						qr3.video as `non_specific_video`,
						qr3.screen_shot as `non_specific_screen_shot`,
						qr3.description as `non_specific_description`
						
					FROM `".QUOTE_RESPONSES."`
					INNER JOIN `".QUOTE_REQUESTS."` ON `".QUOTE_REQUESTS."`.`id` = `".QUOTE_RESPONSES."`.`request_id`
					INNER JOIN `".QUOTE_DEPOSITS."` ON `".QUOTE_DEPOSITS."`.`q_id` = `".QUOTE_RESPONSES."`.`id`
					INNER JOIN `".PRODUCTS."` ON `".PRODUCTS."`.`id` = `".QUOTE_DEPOSITS."`.`product_id`
					".VEHICLE_JOIN."
					LEFT JOIN `".VIDEOS."` qr1 ON qr1.`id` =  `".PRODUCTS."`.`video_id`
					LEFT JOIN `".VIDEOS."` qr2 ON qr2.`specific_to` = CONCAT(`".QUOTE_REQUESTS."`.`trans`, ' ', `".QUOTE_REQUESTS."`.`key_type`)
					LEFT JOIN `".VIDEOS."` qr3 ON qr3.`specific_to` = 'All'						
					WHERE `".QUOTE_RESPONSES."`.`id` = $id
				")->fetch();
				
				$videos = [];
				$body2 = '';
				
				if(!empty($get['video']) || !empty($get['specific_video']) || !empty($get['non_specific_video']))
				{
					$videos[] = [
						'video' => $get['video'],
						'desc' => $get['description'],
						'shot' => $get['screen_shot']
					];
					
					$videos[] = [
						'video' => $get['specific_video'],
						'desc' => $get['specific_description'],
						'shot' => $get['specific_screen_shot']
					];
					
					$videos[] = [
						'video' => $get['non_specific_video'],
						'desc' => $get['non_specific_description'],
						'shot' => $get['non_specific_screen_shot']
					];
					
					$count = 0;
					
					foreach($videos as $v)
					{ 
						$count = (!empty($v['video'])) ? $count + 1 : $count + 0;
					}
					
					$video_text = ($count == 1) ? 'a video that has' : $count.' videos that have';
					$video_text2 = ($count == 1) ? 'this video is' : 'these videos are';
			
					$body2 = "If you are a first time remote start owner, we understand that it can be confusing learning how to use your remote start. We have provided ".$video_text." valuable information on how to use your remote start. Please understand that ".$video_text2." somewhat generalized to your product and even possibly your car, so it's possible that not all apsects of the video will apply. Please try to remember any specifics we discussed about your install while watching the video and understand that the video may not 100% match the information we discussed. ".ucfirst($video_text2)." provided by the manufacturer as a general guide.";	
					
				}
				
				$body = "Thank you very much for your purchase. Our business thrives on referrals from customers like you. We hope we have gained your trust with our services, so much so that we hope you would refer any friends, family or co-workers our way. We do have a little incentive for you, it's as easy as 123.<br><br>1. Give the Referral Code below to anyone you're sending our way.<br>There is a spot at the bottom of the quote request form to put that number.<br>2. The person you referred makes a purchase.<br>3. We contact you for an address and send you a Target Gift Card as a thank you for referring a customer to us.<br><br>Below is a picture of where to put the referral code. You can show anyone that picture so they know what to look for. Your code can be used as many times as you like, season after season. Each referred purchase gains you a gift card.";		
							
				// Start building the email template
				$e = new QuoteEmailTemplate;
					
				$data = [
					'name' => $get['name'],
					'body' => [$body, TRUE],
					'body2' => [$body2, TRUE],
					'videos' => [$videos, FALSE],
					'referral_id' => $id, 
					'signature' => thanksSignature(TRUE, TRUE)
				];
				
				$e->data($data);
				$e->load('completed');
				$send_html = TRUE;
				$em_body = $e->getHtmlString();
				$em_alt_body = $e->getTextString();
				
				if(Config::get('html_emails') === FALSE)
				{
					$send_html = FALSE;
					$em_body = $em_alt_body;
					$em_alt_body = '';	
				}
				
				$subject = "Thank you for your purchase at ".Settings::get('site_name');
				
				$diff_ok = (!empty($r['gift_date'])) ? determineDateDiff(TODAY, $r['gift_date'], -1) : FALSE;
				$send_cc = (Config::get('send_gift_cc_email') === TRUE && !empty($r['gift_email']) && $diff_ok === TRUE) ? TRUE : FALSE;
				
				// Setup mailer array
				$em = array();
				$em['body'] = $em_body;
				$em['alt_body'] = $em_alt_body;		
				$em['subject'] = $subject;
				$em['to'] = $get['email'] /*SECONDARY_EMAIL*/;
				$em['to_name'] = $get['name'];
				$em['smtp'] = FALSE;
				$em['html'] = $send_html;
					
				//echo $reminder_body.'<br>';	
				//print_r($em); 
				buildPhpMailerEmail($em);
				
				if($send_cc === TRUE)
				{
					$em = array();
					$em['body'] = $em_body;
					$em['alt_body'] = $em_alt_body;
					$em['subject'] = $subject;
					$em['to'] = $get['gift_email'] /*SECONDARY_EMAIL*/;
					$em['to_name'] = $get['gift_name'];
					$em['smtp'] = FALSE;
					$em['html'] = $send_html;
							
					buildPhpMailerEmail($em);
				}
			}
		}	
	}

	// Gets all quotes responses in the db
	public function allQuoteResponses($start)
	{
		$get = $this->db->query("SELECT SQL_CALC_FOUND_ROWS * 
							  FROM `".QUOTE_RESPONSES."` 
							  ORDER BY `id` DESC
							  LIMIT $start, ".Config::get('quote_requests_per_page')."");
		
		$reqs = $get->fetchAll();
		(!empty($reqs)) ? $reqs[] = $get->foundRows() : '';
		$this->results = $reqs;
		return $this;
	}
	
	// Gets the quoted response for the given quote-id in the url
	function selectResponse($id, $edit = FALSE)
	{
		$id = (int)$id;
		
		$sql1 = ", `".QUOTE_REQUESTS."`.`options`, `".QUOTE_REQUESTS."`.`units`, `".QUOTE_REQUESTS."`.`comments`, `".QUOTE_REQUESTS."`.`key_type`, `".QUOTE_REQUESTS."`.`key_id`, `".QUOTE_REQUESTS."`.`trans`, `".QUOTE_REQUESTS."`.`id` AS `quote_id`, `".QUOTE_REQUESTS."`.`engine`, `".QUOTE_REQUESTS."`.`gift_name`, `".QUOTE_REQUESTS."`.`gift_phone`, `".QUOTE_REQUESTS."`.`gift_email`, `".QUOTE_REQUESTS."`.`gift_date`, `".QUOTE_REQUESTS."`.`vehicle_make_id`, `".QUOTE_REQUESTS."`.`vehicle_model_id`, `".QUOTE_REQUESTS."`.`vehicle_year_id`, `".QUOTE_REQUESTS."`.`key_image_upload`";
		
		if($edit === FALSE){ $sql1 = ''; }
		
		$sql = "SELECT `".QUOTE_RESPONSES."`.*, 
						`".QUOTE_REQUESTS."`.`email`,
						".CONCAT_VEHICLE." AS `vehicle`,
						`".QUOTE_REQUESTS."`.`name`,
						`".QUOTE_REQUESTS."`.`vehicle_sufix`,
						".VEHICLE_NAME_PARTS." 
						$sql1 
				   FROM `".QUOTE_RESPONSES."`
				   INNER JOIN `".QUOTE_REQUESTS."` ON `".QUOTE_REQUESTS."`.`id` = `".QUOTE_RESPONSES."`.`request_id`
				   ".VEHICLE_JOIN."
				   WHERE `".QUOTE_RESPONSES."`.`id`='$id'";
		
		$get = $this->db->query($sql);
		return $get->fetch();
	}

	// Re-sends a email quote.
	public function resendQuote()
	{
		$id = (int)Input::val('id|p');
		
		return $this->composeQuote($id);
	}
	
	//Builds a Compose Button for quotes
	public function composeQuote($id)
	{
		$info = $this->selectResponse($id);
		$signature = Config::get('quote_signature').Config::get('quote_signature_part2').thanksSignature(TRUE);
				
		$e = new QuoteEmailTemplate;
		$data = [
			'name' => $info['name'],
			'quote_link' => $info['quote_link'],
			'signature' => [$signature, TRUE]
		];
		
		$e->data($data);
		$e->load('quote');
		$send_html = TRUE;
		$em_body = $e->getHtmlString();
		$em_alt_body = $e->getTextString();
		
		if(Config::get('html_emails') === FALSE)
		{
			$send_html = FALSE;
			$em_body = $em_alt_body;
			$em_alt_body = '';
		}
			
			
		// Setup mailer array
		$em['body'] = $em_body;
		$em['alt_body'] = $em_alt_body;		
		$em['subject'] = 'Remote Start Quote '.$info['name'].' - '.$info['vehicle'].' '.$info['vehicle_sufix'];
		$em['to'] = $info['email'];
		$em['to_name'] = $info['name'];
		$em['smtp'] = FALSE;
		$em['html'] = $send_html;
		$em['bcc_to'] = '';
		$em['bcc_name'] = '';
		
		return buildPhpMailerEmail($em);
		
		?>
		<!--<script>
		$('#send').click(function() {
			window.open('<?php echo 'https://mail.google.com/mail/?view=cm&fs=1&to='.urlencode($quote['email']).'&su=Remote Start Quote '.urlencode($quote['name']).' - '.urlencode($quote['vehicle']); ?>','_blank');
		});
		</script>-->
		<?php
	
	}

	public function getQuoteMenuInfo()
	{
		$code = sanitize(Input::val('rsmn_quote_code|c'));
		$get = $this->db->prepare("SELECT `".QUOTE_RESPONSES."`.*,
									`".QUOTE_REQUESTS."`.`cookie_code`		
								FROM `".QUOTE_REQUESTS."` 
								INNER JOIN `".QUOTE_RESPONSES."` ON `".QUOTE_RESPONSES."`.`request_id` = `".QUOTE_REQUESTS."`.`id`
								WHERE `".QUOTE_REQUESTS."`.`cookie_code` = ?");
		$get->execute(array($code));
		return $get->fetch();
	}

	function updateQuoteRead($id)
	{
		$type = '';
		$now = time();
		
		if(Input::exists('t|g'))
		{ $type = ", `viewed_by_txt` = 1"; }
		elseif(Input::exists('e|g'))
		{ $type = ", `viewed_by_email` = 1"; }
			
		$this->db->query("UPDATE `".QUOTE_RESPONSES."` SET `date_read_quote` = NOW(), `read_quote` = 1".$type." WHERE `id` = ".$id);
	}

}

QuoteSystem.class.php

<?php

class QuoteSystem
{
	protected $db;
	
	public function __construct()
	{
		$this->db = DB::getInstance();
	}
	
	public function results()
	{
		return $this->results;
	}

	// Updates the response type for each quote posted
	public function changeResponseType($tbl)
	{
		$key="";
		foreach(Input::val('iemailed|p') as $c["$key"] => $value)
		{ $this->db->query("UPDATE `".$tbl."` SET `response` = 'Emailed' WHERE `id` = '".(int)$c[$key]."'"); }	
	}

	// Searches the db for matching results for the given search criteria
	public function searchQuote($start)
	{
		$field = Input::val('tbl_field|g');
		$srch = Input::val('searched|g');
		
		$join = "LEFT JOIN `".QUOTE_RESPONSES."` ON `".QUOTE_RESPONSES."`.`request_id` = `".QUOTE_REQUESTS."`.`id`
				 LEFT JOIN `".QUOTE_DEPOSITS."` ON `".QUOTE_DEPOSITS."`.`q_id` = `".QUOTE_RESPONSES."`.`id`
				 LEFT JOIN `".QUOTE_DEP_RESP."` ON `".QUOTE_DEP_RESP."`.`quote_id` = `".QUOTE_RESPONSES."`.`id`
				 LEFT JOIN `".SCH."` ON `".SCH."`.`deposit_id` = `".QUOTE_DEPOSITS."`.`id`
				 ".VEHICLE_JOIN;
	
		$orderby = "`".QUOTE_REQUESTS."`.`name` ASC";
		
		switch($field)
		{
			case "Name": $where = "WHERE `".QUOTE_REQUESTS."`.`name` LIKE :s ";
			break;
			
			case "Vehicle": $where = "WHERE `".QUOTE_REQUESTS."`.`vehicle` LIKE :s ";
			break;
			
			case "Vehicle Sufix": $where =  "WHERE `".QUOTE_REQUESTS."`.`vehicle_sufix` LIKE :s ";
			break;
			
			case "Phone": $where =  "WHERE `".QUOTE_REQUESTS."`.`phone` LIKE :s ";
			break;
			
			case "Date": $srch = explode("-", $srch);
						 $srch = $srch[2].'-'.$srch[0].'-'.$srch[1];
						 $old = strtotime("-4 days", strtotime($srch));
						 $ahead = strtotime("+5 days", strtotime($srch));
						 $where = "WHERE `".QUOTE_REQUESTS."`.`date_sent` >= '$old' AND `".QUOTE_REQUESTS."`.`date_sent` <= '$ahead'";
						 $orderby = "`".QUOTE_REQUESTS."`.`date_sent` ASC";
			break; 
			
			case "Quote Id": $srch = (int)$srch;
							 $where = "WHERE `".QUOTE_RESPONSES."`.`id` = $srch ";			 
			break; 
			
			case "Email": $where = "WHERE `".QUOTE_REQUESTS."`.`email` LIKE :s ";
			break; 
			
			case "Schedule": $srch = explode("-", $srch);
							 $srch = $srch[2].'-'.$srch[0].'-'.$srch[1];
							 $old = date("Y-m-d", strtotime("-1 day", strtotime($srch)));
							 $ahead = date("Y-m-d", strtotime("+2 day", strtotime($srch)));
							 $where = "WHERE `".SCH."`.`install_time` BETWEEN '$old' AND '$ahead'";
							 $orderby = "`".SCH."`.`install_time` ASC";
			break;
	
			case "Read": $srch = (int)$srch;
						 $where = "WHERE `".QUOTE_RESPONSES."`.`read_quote` = $srch";
						 $orderby = "`".QUOTE_RESPONSES."`.`date_sent` DESC";
			break;
			
			case "WHERE": 
						 $srch = str_replace('!gt!', '>', $srch);
						 $srch = str_replace('!lt!', '>', $srch);
						 $where = "WHERE ".$srch;
			break;
		}
		
		$sql  = "SELECT `".QUOTE_REQUESTS."`.`id`,
						`".QUOTE_REQUESTS."`.`email`, 
						`".QUOTE_REQUESTS."`.`name`,
						`".QUOTE_REQUESTS."`.`response`, 
						`".QUOTE_REQUESTS."`.`date_sent` AS `request_sent`, 
						`".QUOTE_REQUESTS."`.`vehicle` AS `car_requested`,
						`".QUOTE_REQUESTS."`.`vehicle_sufix`,
						`".QUOTE_RESPONSES."`.`quote_active`,
						`".QUOTE_RESPONSES."`.`id` AS `response_id`, 
						`".QUOTE_RESPONSES."`.`date_sent` AS `quote_date_sent`, 
						`".QUOTE_RESPONSES."`.`purchased`, 
						".CONCAT_VEHICLE." AS `vehicle`, 
						`".QUOTE_RESPONSES."`.`read_quote` AS `read`, 
						`".SCH."`.`install_time`, 
						`".SCH."`.`cancelled`, 
						`".SCH."`.`schedule_id`,
						`".SCH."`.`sch_notes`,
						`".QUOTE_DEP_RESP."`.`id` AS `dep_resp_id`, 
						`".QUOTE_DEPOSITS."`.`part_num` AS `dep_part_num`, 
						`".QUOTE_DEPOSITS."`.`id` AS `deposit_id`,
						`".QUOTE_DEPOSITS."`.`product_id`, 
						`".QUOTE_DEPOSITS."`.`response` AS `dep_type` ";
						
		$sql .= "FROM `".QUOTE_REQUESTS."` ";
		$sql .= $join .' '. $where;
		$sql .= " ORDER BY ".$orderby." ";
		/*$sql .= "LIMIT $start, ".Config::get('quote_requests_per_page');*/
		
		$int_fields = array('Quote Id', 'Date', 'Schedule', 'Read');
		$like_fields = array('Name', 'Email', 'Vehicle', 'Vehicle Sufix', 'Phone');
		$where_fields = array('WHERE');
		
		if(in_array($field, $like_fields))
		{ 
			$param = '%'.$srch.'%';
			$get = $this->db->prepare($sql);
			$get->bind(':s', $param); 
			$get->execute() /**/;
		}
		elseif(in_array($field, $int_fields))
		{ $get = $this->db->query($sql) /**/; }
		elseif(in_array($field, $where_fields))
		{ $get = $this->db->query($sql) ; }
			
			//echo $get->printSql().'<br><br>';
			//echo $get->errorReason();
			
		$this->results = $get->fetchAll();
		return $this;
	}

}

Any advice or guidance is appreciated.

scootstah · July 1, 2015

In processQuote() you're doing a whole bunch of validation. The way you are doing it is going to be very repetitive with different components, because now you have to implement the same repeated validation logic into every component. So, you can move all of that logic to some kind of validation or form component. Also, it looks like a lot of the code in processQuote() belongs in a controller of some sort.

Overall your classes definitely have too much responsibility. When you create a class, you have to ask yourself what the class is going to do. It should have a very specific function, and only do that thing. If your class ends up doing more than one thing, you should break the extra thing into its own class and then tie them together with dependency injection. The same goes for methods within a class. Each method should only do one specific thing. It's very hard to write tests if your method is performing validation, updating database rows, and sending emails all at the same time.

EDIT: From the class names, I'm not really sure what their specific purpose was supposed to be. "QuoteSystem" sounds very broad, and "QuoteResponse" could mean several things. Is it supposed to be an object that is a response to a quote, or is it supposed to be an output response for a quote? If you can clarify the specific purpose of these classes we might be able to help more.

Edited July 1, 2015 by scootstah

fastsol · July 1, 2015

The QuoteResponse in my mind was a class that deals with anything that had to do with the response of the quote request. I agree with the breaking out the validation, but I haven't been able to figure out a way to do that universally among the classes and still achieve the validation I want. I've looked at validation type classes and didn't feel they offer much of a better solution cause it's still repeating code within the class method to setup everything.

Could you do a quick example of break out classes or methods to make the processquote() "better", doesn't have to actually function, just parts you would move out to other areas.

scootstah · July 1, 2015

Can you show me where processQuote() is being used? What is calling that, and when?

fastsol · July 1, 2015

Sure. It gets called after submitting a form with in the admin area. I will only post the relevant code, the rest of the code just contains the setup of the form.

        if(Input::noVal('quote-id|g') && Input::noVal('response-id|g'))
	{ redirect(Config::get('admin_redirect_page')); }
	elseif(Input::exists('submit|p') && Input::exists('action|g') && in_array(Input::val('action|g'), array("add", "edit")))
	{ $results = $qResponse->processQuote(); } 
	elseif(Input::exists('quote-id|g') && Input::val('action|g') == "add") {$results = $qRequest->selectQuoteRequest()->results();}
	elseif(Input::exists('response-id|g') && Input::val('action|g') == "edit") {$results = $qResponse->selectResponse(Input::val('response-id|g'), TRUE);}
	
	$check_num = '';
	if(Input::noVal('response-id|g') && Input::notExists('submit|p'))
	{
		$check = DB::getInstance()->query("SELECT `id` FROM `".QUOTE_RESPONSES."` WHERE `request_id` = ".(int)Input::val('quote-id|g'));
		if($check_num = $check->rowCount() > 0)
		{ echo formComplete('A Quote has already been made for this request.', TRUE); }
	}
		
	if(!empty($results) && empty($check_num))
	{

The page that holds this code is not in a class, it's just an admin area viewing/posting page.

This is the whole page, just in case you want more, but the code above is where the processquote() is. This code is an include file with in a wrapped admin area. So whatever happens on this page will display or happen within a predefined div within the page, so it can't/won't effect the rest of the admin area, like menus or manipulation of the page around it.

<?php
if(!defined('body_id')){ header("Location: /404.php"); exit; }

$qRequest = new QuoteRequest;
$qResponse = new QuoteResponse;

/*if(Input::exists('update|p'))
{
	$qRequest->changeResponseType(QUOTE_REQUESTS);
	echo formComplete("Emailed Updated", TRUE);
}
else*/if(empty(Input::super('post')) && Input::notExists('action|g'))
{
	$results = $qRequest->allQuoteRequests($start)->results();
	if(!empty($results))
	{nexPrevStandardAdmin(end($results), Config::get('quote_requests_per_page'));}
?>
<form action="" method="post">
<!--<div class="right"><?php //if(!empty($results)){echo '<input name="update" type="submit" value="Update Emailed" />';} ?></div>-->
<table id="admin_table" cellpadding="4" class="admin-tbl">
	<tr>
		<?php
		$th = array("Name", "Vehicle", "Date", "ID#", "Read", "Actions");
		echo adminThList($th);
		?>
	</tr>
	<?php
	if(empty($results)){}
	else
	{
		foreach($results as $r)
		{
			//print_r($r);
			
			if(!is_array($r)){}
			else
			{	 
				$already_quoted='';
				$has_read='';
				
				$name = (empty($r['response'])) ? '<span style="color: #FF0000">'.$r['name'].'</span>' : $r['name'];
				
				/*if(empty($r['response_id']) && $r['already_quoted'] > 0)
				{ $already_quoted = 'style="background: yellow"'; }*/
				
				if($r['email'] == Config::get('no_email'))
				{ $has_read = NO_EMAIL_ICON; }
				elseif($r['viewed_by_email'] == FALSE && $r['viewed_by_txt'] == FALSE && $r['read_quote'] == TRUE)
				{ $has_read = CHECK_MARK; }
				elseif($r['viewed_by_email'] == FALSE && $r['viewed_by_txt'] == FALSE && $r['read_quote'] == FALSE && empty($r['response_id']))
				{}
				elseif($r['viewed_by_email'] == FALSE && $r['viewed_by_txt'] == FALSE && $r['read_quote'] == FALSE)
				{ $has_read = RED_X; }
				else
				{
					$has_read .= ($r['viewed_by_email'] == TRUE) ? ENVELOPE_ICON : '';
					$has_read .= ($r['viewed_by_txt'] == TRUE) ? SMS_ICON : '';
				}
				
				$icon_spacing = '    ';
				
				// Used to mark quotes are emailed, keeping just in case need it later.
				//<input type="checkbox" name="iemailed['.$r['id'].']" value="'.$r['id'].'">
	
				echo '<tr>
						<td style="max-width:150px">'.adminLinks('qsearch', 'tbl_field=Name&srch=Search&searched='.$r['name'], '', $name).'</td>
						<td '.$already_quoted.'>'.adminLinks('qsearch', 'tbl_field=Vehicle&srch=Search&searched='.$r['actual_vehicle'], '', $r['actual_vehicle']).'</td>
						<td class="center">', date("m-d-y", $r['date_sent']), '</td>
						<td class="center">', (!empty($r['response_id'])) ? adminLinks('sresponse', 'quote-id='.$r['response_id'], 'view', $r['response_id']) : '', 
							(!empty($r['response_id']) && $r['quote_active'] == FALSE) ? '<br>'.RED_X : '', '</td>
						
						<td class="center">'.$has_read.'</td>
						
						<td class="center icon-spacing">', (empty($r['response_id']) && $r['response'] != "Emailed") ? '' : 
												(($r['response'] == "Emailed") ? EMAILED_ICON : ''),
												
						 (empty($r['response_id'])) ? adminLinks('', 'quote-id='.$r['id'], 'add', ADD_QUOTE_ICON) : 
												'' /*adminLinks('', 'quote-id='.$r['id'], 'view', Q_INFO_ICON)*/,
												
						 (!empty($r['response_id']) && empty($r['deposit_id'])) 
												? adminLinks('', 'response-id='.$r['response_id'], 'edit', UPDATE_Q_ICON) : '',
												
						 (!empty($r['response_id']) && empty($r['deposit_id'])) 
												? adminLinks('sdeposit', 'quote-id='.$r['response_id'], 'add', DEPOSIT_ICON) : 
												((!empty($r['deposit_id'])) ? DEPOSIT_MADE_ICON : ''),
												
						 (empty($r['response_id'])) ? adminLinks('', 'quote-id='.$r['id'], 'delete', GARBAGE): '', '</td>';
						
						echo adminExtendedDisplay($r);
						echo '</tr>';
			}
		}
	}
	?>
</table>
</form>
<?php
}
elseif(Input::val('action|g') == "view")
{
	$results = $qRequest->selectQuoteRequest()->results();
	
	$nodisplay = array('id', 'response', 'date_sent', 'signature', 'read_quote', 'purchased', 'body', 'notes', 'unit_pricing', 'code', 'request_id', 'vehicle_make_id', 'vehicle_model_id' , 'vehicle_year_id');
	$html='';
	foreach($results as $key => $val)
	{
		if(!in_array($key, $nodisplay))
		{ 
			if($key == "city" && !empty($val))
			{ $html .= ucwords($key).' = '.getCityNameAndState($val).'<br>'; }
			elseif($key == "key_id" && !empty($val))
			{ 
				$img_loc_string = getKeyImages($val);
				$html .= '<img src="'.IMAGE_LAY_SITE.$img_loc_string[0]['image'].'"><br>'; 
			}
			elseif($key == "options")
			{ $html .= nl2br(ucwords($val)); }
			else
			{ 	
				if($val == "Push To Start"){ $val = '<span class="bold">'.$val.'</span>'; }
				elseif($val == "Manual"){ $val = '<span class="bold">'.$val.'</span>'; }
				elseif($val == "Turn A Knob"){ $val = '<span class="bold">'.$val.'</span>'; }
				$html .= ucwords($key).' = '.$val.'<br>'; 
			}

		}
	}
	echo '<h1>Original Request</h1>'.$html;
}
elseif(Input::val('action|g') == "delete")
{
	if(Input::exists('delete|p'))
	{
		if($qRequest->deleteRequest() === TRUE)
		{echo formComplete("Quote Request Deleted", TRUE);}
	}
	elseif(Input::exists('quote-id|g'))
	{
		$results = $qRequest->selectQuoteRequest()->results();
		
		$form_fields = array(
			array("tag" => array("function", "class=\"bold\""),
				  "text" => "name:",
				  "name" => "name",
				  "class_fields" => FALSE,
				  "type" => "text",
				  "required" => FALSE,
				  "value_results_name" => "name",
				  "admin_notes" => FALSE,
				  "options" => array(),
				  "function" => $results['name']
				  ),
			array("tag" => array("function", "class=\"bold\""),
				  "text" => "vehicle:",
				  "name" => "vehicle",
				  "class_fields" => FALSE,
				  "type" => "text",
				  "required" => FALSE,
				  "value_results_name" => "vehicle",
				  "admin_notes" => FALSE,
				  "options" => array(),
				  "function" => $results['actual vehicle']
				  ),
			array("tag" => array("function", "class=\"bold\""),
				  "text" => "date sent:",
				  "name" => "date_sent",
				  "class_fields" => FALSE,
				  "type" => "text",
				  "required" => FALSE,
				  "value_results_name" => "date_sent",
				  "admin_notes" => FALSE,
				  "options" => array(),
				  "function" => date("m-d-Y", $results['date_sent'])
				  ),
			"form_button" => array("name" => "delete", 
								   "type" => "submit", 
								   "value" => "delete",
								   "attributes" => "")
		);
		
		adminForm($form_fields, (isset($results)) ? $results: '');
	}
}
else
{
	if(Input::noVal('quote-id|g') && Input::noVal('response-id|g'))
	{ redirect(Config::get('admin_redirect_page')); }
	elseif(Input::exists('submit|p') && Input::exists('action|g') && in_array(Input::val('action|g'), array("add", "edit")))
	{ $results = $qResponse->processQuote(); } 
	elseif(Input::exists('quote-id|g') && Input::val('action|g') == "add") {$results = $qRequest->selectQuoteRequest()->results();}
	elseif(Input::exists('response-id|g') && Input::val('action|g') == "edit") {$results = $qResponse->selectResponse(Input::val('response-id|g'), TRUE);}
	
	$check_num = '';
	if(Input::noVal('response-id|g') && Input::notExists('submit|p'))
	{
		$check = DB::getInstance()->query("SELECT `id` FROM `".QUOTE_RESPONSES."` WHERE `request_id` = ".(int)Input::val('quote-id|g'));
		if($check_num = $check->rowCount() > 0)
		{ echo formComplete('A Quote has already been made for this request.', TRUE); }
	}
		
	if(!empty($results) && empty($check_num))
	{
?>
<div id="admin-form" class="form-bkgnd">
<div id="original-request" style="padding-left: 6px;">
	<?php
		if(Input::notExists('submit|p'))
		{
			$nodisplay = array('id', 'quote_active', 'request_active', 'response', 'date_sent', 'signature', 'read_quote', 'purchased', 'body', 'notes', 'unit_pricing', 'code', 'request_id', 'vehicle_make_id', 'vehicle_model_id', 'vehicle_year_id');
			$html='<div id="request-info-left">';
			$car_addition='';
			$display_count = 0;
			
			foreach($results as $key => $val)
			{
				if(!in_array($key, $nodisplay))
				{ 
					if($key == "city" && !empty($val))
					{ $html .= ucwords($key).' = '.getCityNameAndState($val).'<br>'; }
					elseif($key == "key_id" && !empty($val))
					{ 
						$img_loc_string = getKeyImages($val);
						$html .= '<img src="'.IMAGE_LAY_SITE.$img_loc_string[0]['image'].'"><br>'; 
					}
					elseif($key == 'key_image_upload' && !empty($val))
					{ $html .= '<a href="'.PLUGINS_QUOTES.'serve-key-image.php?image='.$val.'" target="_blank">
								<img src="'.PLUGINS_QUOTES.'serve-key-image.php?image='.$val.'" class="key-image-upload-admin"> Click To Enlarge</a><br>'; }										
					elseif($key == "options")
					{ $html .= nl2br(ucwords($val)); }
					elseif(empty($val))
					{ $html .= ucwords($key).' = '.$val.'<br>'; }
					else/*if($key == "key_type")*/
					{ 	
						if($val == "Push To Start")
						{ 
							if(Input::val('action|g') != "edit")
							{ $car_addition .= ' PTS'; }
							$val = '<span class="bold">'.$val.'</span>'; 
						}
						elseif($val == "Manual")
						{ 
							if(Input::val('action|g') != "edit")
							{ $car_addition .= ' Manual Trans'; }	
							$val = '<span class="bold">'.$val.'</span>'; 
						}
						elseif($val == "Turn A Knob")
						{ 
							if(Input::val('action|g') != "edit")
							{ $car_addition .= ' Intellikey'; }	
							$val = '<span class="bold">'.$val.'</span>'; 
						}
						elseif($val == "Hybrid" || $val == "Diesel")
						{
							if(Input::val('action|g') != "edit")
							{ $car_addition .= ' '.$val; }	
							$val = '<span class="bold">'.$val.'</span>';
						}
						/*elseif($key == '40bit' || $key == '80bit')
						{ $car_addition .= $key; }*/
						
						$html .= ucwords($key).' = '.$val.'<br>'; 
					}
					//else{ $html .= ucwords($key).' = '.$val.'<br>'; }
					$display_count++;
						
					if($display_count == 9)
					{ $html .= '</div><div id="request-info-right">'; } 
				}
			}
			$html .= '</div><div class="clear"></div>';
			$_SESSION['quote_original_info'] = $html;
			$html='';
		}
		echo (isset($_SESSION['quote_original_info'])) ? $_SESSION['quote_original_info'] : '';
	?>
</div><br>

<?php echo (Input::hasVal('quote-id|g')) ? '<button onclick="window.location.href=\'/a.php?menu=Quotes&admin_page=quotes_send_email&action=add&quote-id='.(int)Input::val('quote-id|g').'\'">Email Client</button><br><br>' : ''; ?>

<button class="copy-span">Copy To Email</button><span class="copy-to-textarea">Your car does require a bypass module for the chip in the key.</span><br><br>
<button class="copy-span">Copy To Email</button><span class="copy-to-textarea">The price will include keyless entry and trunk pop on the new remotes so you can retain those functions while the car is remote started.</span><br><br>
<button class="copy-span">Copy To Email</button><span class="copy-to-textarea">The keyless entry is included FREE of charge on your vehicle and is a $20 value.</span><br><br>
<button class="copy-span">Copy To Email</button><span class="copy-to-textarea">The trunk pop is included FREE of charge on your vehicle and is a $20 value.</span><br><br>
<button class="copy-span">Copy To Email</button><span class="copy-to-textarea">You will need to give up a key for the install. Meaning you would arrive with 2 keys and leave with 1 key in hand. The other key would be hidden permanently in the dash. If you need/want to have more than 1 key after the install, it is suggested to have an additional key cut and programmed by a locksmith or dealership before the install is performed.</span><br><br>
<button class="copy-span">Copy To Email</button><span class="copy-to-textarea">Great news, your car does not require any additional parts or labor when utilizing the Direct Fit kits.</span><br><br>
<button class="copy-span">Copy To Email</button><span class="copy-to-textarea">Your factory remote key will still operate as it does now and will still be used to unlock the doors.</span><br><br>

	<!--<script type="text/javascript" src="scripts/jscripts/tiny_mce/tiny_mce.js"></script>
	<script type="text/javascript" src="scripts/textarea-mce.js"></script>
-->	<?php
	/*if(isset($drone)){ echo '<div id="drone_notes"><span class="bold">DroneMobile Notes:</span>
							<br>Make sure to check if the bypass being used will support Telematics Gateway when using the DroneMobile.</div>'; }
*/	
	//$car_make = explode(" ", $results['vehicle']);
	$car_model = stripKeyTypeFromName($results['vehicle_model_name']);
	$get_notes = adminVehicleNotes($results['vehicle_make_id']);
	echo (!empty($get_notes)) ? '<div id="car_notes"><span class="bold">'.$results['vehicle_make_name'].' Notes</span><br>'.
			nl2br(preg_replace("/\w*?".preg_quote($car_model)."\w*/i", '<span class="highlight-purple">$0</span>', $get_notes['notes'])).'</div>' : '';
	
	if(Input::notExists('submit|p') && Input::exists('response-id|g')) { $_SESSION['units_requested'] = $results['units'] = explode(",  ", $results['units']); }
	elseif(Input::notExists('submit|p') /*&& !isset($_GET['response-id'])*/) { $_SESSION['units_requested'] = explode(",  ", $results['units']); }
	/*$units = (isset($results['units']) && !is_array($results['units'])) ? explode(",  ", $results['units']) : ((isset($results['units'])) ? $results['units'] : '');*/
	
	$quote_items = (Input::exists('response-id|g') && Input::val('action|g') == "edit" && Input::notExists('submit|p')) ? (new QuoteItems)->selectQuoteItems()->results() : array();
	
	$num = 1;
	foreach(allUnits("(`".CATEGORIES."`.`cat_name` = 'Two Way Remotes' OR `".CATEGORIES."`.`cat_name` = 'One Way Remotes' OR `".CATEGORIES."`.`cat_name` = 'Make Specific Models'
						 OR `".CATEGORIES."`.`cat_name` = 'Custom Packages')", 
					 "`".CATEGORIES."`.`cat_name`, `".PRODUCTS."`.`prod_name`, `".PRODUCTS."`.`price` ASC") as $u)
	{
		if(!empty($quote_items))
		{
			foreach($quote_items as $key => $val)
			{
				if($val['product_id'] == $u['id'])
				{
					$results['units'][$u['prod_name']]['price'] = $val['quote_price'];
					$results['units'][$u['prod_name']]['note'] = $val['item_note'];
				}
			}
		}
		
		$html  = '<div><label>';// Open wrap div
		
		$html .= (in_array($u['prod_name'], $_SESSION['units_requested'])) ? '<span class="bold">'.$u['prod_name'].'</span>' : $u['prod_name'];
		
		$html .= (isOnSale($u['sp_start'], $u['sp_end']) === TRUE) ? ' <span class="on-sale" id="price-'.$num.'">'.$u['sp_price'].'</span>' : ' <span id="price-'.$num.'">'.$u['price'].'</span>';
		
		$html .= '<img class="expand-symbol expand" data-expand-id="'.$num.'" src="'.EXPAND_SYMBOL.'"></label><input name="units['.$u['prod_name'].'][price]" type="text" value="';
		
		$html .= (isset($results['units'][$u['prod_name']]['price'])) ? $results['units'][$u['prod_name']]['price'] : '';
		
		$html .= '" style="width: 40px;" id="prod-expand-'.$num.'"><input name="units['.$u['prod_name'].'][note]" type="text" value="';
		
		$html .= (isset($results['units'][$u['prod_name']]['note'])) ? $results['units'][$u['prod_name']]['note'] : '';
		
		$html .= '" style="width: 400px;">';
		
		$html .= '<input name="units['.$u['prod_name'].'][product_id]" type="hidden" value="';
		
		$html .= (isset($results['units'][$u['prod_name']]['product_id'])) ? $results['units'][$u['prod_name']]['product_id'] : $u['id'];
		
		$html .= '">';
		
		$html .= '<input name="units['.$u['prod_name'].'][original_price]" type="hidden" value="'.$u['price'].'">';
		
		$html .= '<input name="units['.$u['prod_name'].'][sale_price]" type="hidden" value="'.$u['sp_price'].'">';
		
		$html .= '<input name="units['.$u['prod_name'].'][on_sale]" type="hidden" value="';
		$html .= (isOnSale($u['sp_start'], $u['sp_end']) === TRUE) ? 1 : 0;
		$html .= '">';
		
		$html .= '<div id="expand_'.$num.'" class="item-option-expand">';
		
		foreach(Config::get('expand_options') as $eo => $eo_val)
		{ $html .= '<input type="checkbox" value="'.$eo_val.'" class="expand_value" data-ex-id="'.$num.'">'.$eo.'  '; }
		
		$html .= '</div></div>';// Close expand and wrap divs
		
		$html_array[] = $html;
		$num++;
	}
	$output = uList($html_array, "unit-list"); $html_array=''; $html='';
	
	/*foreach(allUnits("`".PRODUCTS."`.`Bypass Modules`=1") as $b)
	{
		$html[] = '<input type="radio" name="bypass" value="'.$b['prod_name'].'"> '.$b['prod_name'];
	}
*/	/*$byp = allUnits("`".PRODUCTS."`.`Bypass Modules`=1");
	$bypasses[] = '';
	foreach($byp as $p) { $bypasses[] = $p['prod_name']; }
*/	
	//$results['signature'] = (isset($results['signature'])) ? $results['signature'] : $signature;
	$results['email_body'] = (Input::val('action|g') == "edit" && Input::notExists('submit|p')) ? $results['body'] : ((isset($results['email_body'])) ? $results['email_body'] : '');
	$results['vehicle_sufix'] = (Input::val('action|g') == "add" && empty($results['vehicle_sufix'])) ? trim($car_addition) : $results['vehicle_sufix'];
	//$results['vehicle'] = (isset($results['vehicle'])) ? str_replace(array(' (standard key)', ' (push-to-start)'), '', $results['vehicle']) : '';
	require_once(ROOT.PLUGINS_QUOTES.'admin_vehicle_select.php');
	
	$form_fields = array(
		array("tag" => array("label"),
			  "text" => "name:",
			  "name" => "name",
			  "class_fields" => TRUE,
			  "type" => "text",
			  "required" => TRUE,
			  "value_results_name" => "name",
			  "admin_notes" => FALSE,
			  "options" => array("input_op" => 'maxlength="45"'),
			  ),
		array("tag" => array("function"),
			  "text" => "vehicle:",
			  "name" => "vehicle",
			  "class_fields" => TRUE,
			  "type" => "text",
			  "required" => TRUE,
			  "value_results_name" => "vehicle",
			  "admin_notes" => FALSE,
			  "options" => array(),
			  "function" => $vhtml
			  ),
		array("tag" => array("label"),
			  "text" => "vehicle sufix:",
			  "name" => "vehicle_sufix",
			  "class_fields" => TRUE,
			  "type" => "text",
			  "required" => FALSE,
			  "value_results_name" => "vehicle_sufix",
			  "admin_notes" => FALSE,
			  "options" => array("input_op" => 'maxlength="100"'),
			  ),
		array("tag" => array("label"),
			  "text" => "email body:",
			  "name" => "email_body",
			  "class_fields" => TRUE,
			  "type" => "textarea",
			  "required" => TRUE,
			  "value_results_name" => "email_body",
			  "admin_notes" => FALSE,
			  "options" => array("input_op" => 'style="width: 600px; height: 200px;"'),
			  ),
		array("tag" => array("function"),
			  "text" => "available units:",
			  "name" => "",
			  "class_fields" => TRUE,
			  "type" => "text",
			  "required" => FALSE,
			  "value_results_name" => "",
			  "admin_notes" => FALSE,
			  "options" => array(),
			  "function" => $output
			  ),
		/*array("tag" => array("function"),
			  "text" => "Bypass Used:",
			  "name" => "",
			  "class_fields" => TRUE,
			  "type" => "text",
			  "required" => FALSE,
			  "value_results_name" => "",
			  "admin_notes" => FALSE,
			  "options" => array(),
			  "function" => selectBox('bypass', $bypasses, (isset($results['bypass'])) ? $results['bypass'] : '', 'val', TRUE)
			  ),
*/		array("tag" => array("label"),
			  "text" => "notes:",
			  "name" => "notes",
			  "class_fields" => TRUE,
			  "type" => "textarea",
			  "required" => FALSE,
			  "value_results_name" => "notes",
			  "admin_notes" => FALSE,
			  "options" => array(),
			  ),
		/*array("tag" => array("label"),
			  "text" => "signature:",
			  "name" => "signature",
			  "class_fields" => TRUE,
			  "type" => "textarea",
			  "required" => FALSE,
			  "value_results_name" => "signature",
			  "admin_notes" => FALSE,
			  "options" => array("input_op" => 'style="width: 600px;"'),
			  ),*/
		"hiddens" => array("email" => (isset($results['email'])) ? $results['email'] : '',
						   "code" => (isset($results['code'])) ? $results['code'] : '',
						   "quote_id" => (isset($results['quote_id'])) ? $results['quote_id'] : Input::val('quote-id|g'),
						   "send_sms" => (isset($results['send_sms'])) ? $results['send_sms'] : '',
						   "phone" => (isset($results['phone'])) ? $results['phone'] : '',
						   "vehicle_make_name" => (isset($results['vehicle_make_name'])) ? $results['vehicle_make_name'] : '',
						   "vehicle_model_name" => (isset($results['vehicle_model_name'])) ? $results['vehicle_model_name'] : ''),
		"form_button" => array("name" => "submit", 
							   "type" => "submit", 
							   "value" => "submit",
							   "attributes" => "")

	);
	
	adminForm($form_fields, (isset($results)) ? $results : '', 'response-form');
	?>
</div>

	<script>
	$(document).ready(function(){
		$('.expand-symbol').on('click', function(){
			var num = $(this).data('expand-id');
			$('#expand_'+num).slideToggle();
		});
		
		$('.expand_value').on('click', function(){
			var value = parseInt($(this).val());
			var expand_id = $(this).data('ex-id');
			var price_input = $('#prod-expand-'+expand_id);
			
			if($(price_input).val() == ''){ var price= parseInt($('#price-'+expand_id).text()); }
			else{ var price = parseInt($(price_input).val()); }
			
			if($(this).prop('checked')){ var new_price = price + value; }
			else{ var new_price = price - value; }
			
			$(price_input).val(new_price);
		});
		
		$('.copy-span').on('click', function(){
			var text = $(this).next('span').text() + "\r\n\r\n",
				old_text = $("textarea[name='email_body']").val();
			
			$("textarea[name='email_body']").val(old_text + text);
		});
	});
	</script>

	<span>
	Confirmed Cars where factory keyless works while running:<br>
	2011 Rogue standard key<br>
	2007 Explorer<br>
	2004 Sentra<br>
	2010 Focus<br>
	2003 Murano<br>
	2003 Ranger<br>
	2005 Altima<br>
	1999 Cherokee<br>
	2005 Sierra<br>
	2010 Mazda 3 Flip-key<br>
	2010 Mazda 6 Flip-key<br>
	2008 Outback w/Remote key<br>
	All Toyota Remote Keys<br>
	All Nissan Push-to-Start and Intellikey style<br>
	All Fords with remote keys<br>
	</span><br>
	<span>
	Cars that the factory keyless DOES NOT work while running:<br>
	All Hondas<br>
	All Hyundai/Kia except select PTS versions<br>
	Most 2002-older Fords<br>
	Mazda Intellikey<br>
	</span>
	<?php
		$docs = getDocs("admin_files"); 
		?>
		<br>
		<table cellpadding="6" id="admin_table" class="admin-tbl hide">
		<tr>
			<th>Folder / Files</th>
			<th>Relative Path</th>
		</tr>
		<?php
		$td = "gray-td";
		foreach($docs as $doc => $val)
		{	
			echo '<tr class="'.$td.'"><td colspan="2">'.FOLDER.' '.$doc;
			
			unset($val[0], $val[1]);
			if(!isset($val[2]))
			{echo ' - Folder is empty</td></tr>';}
			else
			{echo '</td></tr>';} 
			foreach($val as $v)
			{echo '<tr><td class="indent">'.$v.'</td>
					<td>admin_files/'.$doc.'/'.$v.'</td>
				   </tr>';}
		}
		
		?>
		</table>
<?php
	}
}
?>

Edited July 1, 2015 by fastsol

citypaul · July 1, 2015

Your classes definitely do have WAY too much responsibility.

It may surprise you to learn this, but you are in no way writing object oriented code here. This is procedural code, not object oriented.

The reason smaller objects are better is because it makes it far easier to change and adapt your system later. It also means you can get plenty of reusability out of objects. It's all about making them focused and giving them a clear reason to exist. Right now your code is a nightmare and I'd hate having to maintain it - adding some new functionality to this code would be horrible.

Your code is also far too knowledgeable about the wider system in which it lives. You should not be accessing POST variables AT ALL in your objects like this. Instead, you should be passing things into your objects. Right now you're tied to the exact structure of the HTML form now because of this. So if you changed the form value of "vehicle_sufix" to something else, your code would break, which obviously is not what you want.

It's quite hard to teach this stuff without being able to show it in practice, but essentially you would be well advised to break out your code into much smaller units.

I've just read through your code a bit more. It's very messy and will be impossible to change. You're gonna get bugs all over the place - you'll change one thing and something else will break and so on and so forth.

I don't have time to break your stuff apart in the way you asked. Instead, I'll recommend two books to you.

Here's the first:

http://www.amazon.co.uk/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882

And here's the second:

http://www.amazon.com/Practical-Object-Oriented-Design-Ruby-Addison-Wesley/dp/0321721330

And yes, the second is a Ruby book. However, it's all about OO design and Ruby just happens to be the language chosen. I use lessons from that book at all the time.

You really should read those books, as most of the people on these forums are amateurs and don't know how to write object oriented code.

fastsol · July 1, 2015

I fully agree citypaul. There is a tremendous amount of code that is intertwined. Much of this was written a few years ago while I was still learning mainstream php and have adapted it to better and better practices over the time, but I have always known that it's far from perfect. I fully understand this is procedural code, especially since I literally took a bunch of functions and placed them into a class, simply to try and organize them and maybe it would have just been better to make more organized function pages than classes that aren't really classes.

I really really want to improve this code as I expect to expand it immensely in the future and I want it to be easier and more efficient when doing so. I believe I have the ability to make this better, but I think my main struggle in figuring out how to break this apart is dealing with the validation and then building the output desired without passing a ton of things back and forth amongst several classes that again probably don't have anything to do with each other.

If I could get even a basic example based on my code of how to break it apart, that would likely help a ton.

scootstah · July 1, 2015

as most of the people on these forums are amateurs and don't know how to write object oriented code.

That's a pretty douchey thing to say. Especially for somebody with 3 posts.

Just an FYI....object oriented is not the end-all, be-all. If you think otherwise, it is you who is the amateur.

citypaul · July 1, 2015

That's a pretty douchey thing to say. Especially for somebody with 3 posts.

Just an FYI....object oriented is not the end-all, be-all. If you think otherwise, it is you who is the amateur.

It's the truth though. I read through a bunch of topics and read a lot of code before writing that comment. Most of the people here are learning, so you shouldn't expect too many experts. That's not to say that that will be the case for all people though of course. Some people here may be excellent

And I never said object oriented is the be all and end all, but this person's code uses classes and he's talking about the single responsibility principle, so he is clearly interested in learning about good OOP design. I was merely explaining that the code he has at the moment isn't OOP, but that's ok as everyone has to learn. I started out the same way and had the same questions.

To the OP: I'll have a hunt around to see if I can find a good video or something.

citypaul · July 1, 2015

To the OP: So I found a youtube video by Robert Martin, the author of the Clean Code book I recommended above, and incidentally the guy who coined the phrase SOLID principles (literally the guy who came up with the Single Responsibility Principle), so you can't go wrong with this. I've not watched it but I'm sure it'll be a great starting point:

Edit: I've seen plenty of his videos in the past and he often starts by going on about space for no apparent reason. If he does that just skip that bit, unless you want to learn about space. I've no idea why he does that...

Edited July 1, 2015 by citypaul

scootstah · July 1, 2015

If I could get even a basic example based on my code of how to break it apart, that would likely help a ton.

The problem is where do you start? If you change one thing, you break five others. Then you fix those, and break more. It's a tough situation. It is difficult to explain in a couple of paragraphs. My best advice is to dig through some other codebases and see how other people have solved these problems. Symfony2 is a good one. They've done a really great job at decoupling their components.

Most object oriented web apps these days share at least a few commonalities. Typically they will have some way of routing requests to a controller.

The controller is a class, with some methods inside. The controller is like a module, and the methods are the actions of that module. For example you might have a "NewsController", with methods like "create", "edit", and "delete". A controller should be very lightweight, and just act as the middle man to tie things together. The controller should usually return a response of some kind, and this response is then usually sent to the browser and the application request is then considered complete.

If it were me refactoring your app, I would start with this lifecycle process. The code that you edited in for your admin page is in dire need of refactoring. You need to have separation of concerns. Right now you have HTML all mixed in with application logic... this is a very terrible way to work. Your view logic should be entirely separate. I like Twig. Twig is a template engine which gives you a very easy way to separate your view logic. There are other ways to do it, but that is my go-to.

Once you can break your view logic out, and have more reasonable controllers, then you can get to work on your original two classes that you posted.

Unfortunately you're going to be making a lot of dramatic, breaking changes. But, it's all for the better!

And while you're doing all that refactoring, now is as good a time as any to get going with unit testing.

Edited July 1, 2015 by scootstah

citypaul · July 1, 2015

The problem is where do you start? If you change one thing, you break five others. Then you fix those, and break more. It's a tough situation. It is difficult to explain in a couple of paragraphs. My best advice is to dig through some other codebases and see how other people have solved these problems. Symfony2 is a good one. They've done a really great job at decoupling their components.

Most object oriented web apps these days share at least a few commonalities. Typically they will have some way of routing requests to a controller.

The controller is a class, with some methods inside. The controller is like a module, and the methods are the actions of that module. For example you might have a "NewsController", with methods like "create", "edit", and "delete". A controller should be very lightweight, and just act as the middle man to tie things together. The controller should usually return a response of some kind, and this response is then usually sent to the browser and the application request is then considered complete.

If it were me refactoring your app, I would start with this lifecycle process. The code that you edited in for your admin page is in dire need of refactoring. You need to have separation of concerns. Right now you have HTML all mixed in with application logic... this is a very terrible way to work. Your view logic should be entirely separate. I like Twig. Twig is a template engine which gives you a very easy way to separate your view logic. There are other ways to do it, but that is my go-to.

Once you can break your view logic out, and have more reasonable controllers, then you can get to work on your original two classes that you posted.

Unfortunately you're going to be making a lot of dramatic, breaking changes. But, it's all for the better!

And while you're doing all that refactoring, now is as good a time as any to get going with unit testing.

Well despite our initial squabble, I tried to "like" this comment but the system wouldn't allow me, presumably because of my low number of posts.

This is pretty accurate advice. Unit testing is a very good thing to learn. TDD is even better, which is really a way of maximising the use of automated tests to work out your design and reduce the fear of change. One step at a time though.

I'd recommend reading those books I mentioned, and watch that video. This stuff won't come to you over night. It's only by reading, watching good videos and of course constantly putting these ideas into practice that things will start to "click" for you.

scootstah · July 1, 2015

While we're talking about books, I recommend these as well:

http://www.amazon.com/Objects-Patterns-Practice-Matt-Zandstra/dp/1430260319

http://www.amazon.com/Refactoring-Experts-Voice-Open-Source/dp/1430227273

fastsol · July 1, 2015

Thank you. I watched the video and will do plenty of research and playing to see what I can come up with.

QuickOldCar · July 1, 2015

You really should read those books, as most of the people on these forums are amateurs and don't know how to write object oriented code.

I can see this being true as most asking questions on coding sites are beginners.

If an OP posts procedural code naturally the responses would be returned as procedural.

As we all know helping people with OOP coding would be a nightmare in a forum.

I'll even add that most who even know how to code in OOP may not write good code, most likely even in procedural.

To the OP: So I found a youtube video by Robert Martin, the author of the Clean Code book I recommended above, and incidentally the guy who coined the phrase SOLID principles (literally the guy who came up with the Single Responsibility Principle), so you can't go wrong with this. I've not watched it

Anyone can write a book and express their own opinions and try to convince you their way is better, but since are talking Uncle Bob will go along with you.

Uncle Bob has his own site.

https://sites.google.com/site/unclebobconsultingllc/

fastsol · July 1, 2015

I've watched some videos and read some things and I think I have a grasp as to what to do with the validation part of the processquote(). I also think I understand what I can do with the bottom part of the function to, but the middle area where I do a number of queries and some other logic I can't seem to grasp how to break that out into other classes or methods. Could anyone just give me a quick concept of how you would possibly break out this part of the code. I totally understand that it won't be anything real accurate, I'm just looking for an example of logic behind it.

$now = time();		
			$units_text = "Prices include all parts and labor:\r\n".$email_units;
			$where = '';
			$date_sent = '';
			$date_modified = NULL;
			
			// INSERT/UPDATE QUOTE_RESPONSES
			$params = [
				'request_id' => [$q_id, 'INT'],
				'date_modified' => NULL,
				'body' => $ebody,
				'code' => $code,
				'notes' => $notes,
				'unit_pricing' => $units_text
			];
			
			if($i->get('action') == "add")
			{			
				$params['code'] = randomCode(20);
				$code = $params['code'];
				
				$set = $this->db->insert(QUOTE_RESPONSES, $params);
			}
			elseif($i->get('action') == "edit")
			{
				$params['date_modified'] = date("Y-m-d H:i:s");
				$set = $this->db->update(QUOTE_RESPONSES, $params, '`id` = '.$resp_id, 1);
			}
			
			$last_id = ($i->get('action') == "add") ? $set->lastInsertId() : $resp_id;
			///////////// END ///////////////////////////////////
			
			// UPDATE REFERRALS
			if($i->get('action') == "add")
			{ (new QuoteReferral)->setQuoteId($last_id, $q_id); }
			//////////// END ////////////////////// 
			
			// Clear QUOTE_ITEMS		
			if($i->get('action') == "edit")
			{ $qi->deleteQuoteItems($resp_id); }
			/////////// END //////////////////////
			
			// UPDATE QUOTE_REQUESTS with posted vehicle info.
			$params = [
				'vehicle_make_id' => [$make_id, 'INT'],
				'vehicle_model_id' => [$model_id, 'INT'],
				'vehicle_year_id' => [$year, 'INT'],
				'vehicle_sufix' => $car_sufix
			];
			
			(new QuoteRequest)->updateById($params, $q_id);
			///////// END //////////////////////////////////////////
			
			// SET new QUOTE_ITEMS
			foreach($unit as $key => $val)
			{
				$params = [];
				
				if(!empty($val['price']))
				{
					$note = (!empty($val['note'])) ? sanitize($val['note']) : '';
					$unit_price = addSalesTax($val['price'], Config::get('tax_rate'));
					$add_amt = ($val['on_sale'] == TRUE) ? (int)$val['price'] - (int)$val['sale_price'] : (int)$val['price'] - (int)$val['original_price'];
					
					$params = [
						'item' => $key,
						'quote_price' => [$val['price'], 'INT'],
						'response_id' => [$last_id, 'INT'],
						'product_id' => [$val['product_id'], 'INT'],
						'item_note' => $note,
						'add_amt' => [$add_amt, 'INT']
					];
					
					$qi->insertQuoteItems($params);
				}
			}
			////////// END ///////////////////////////////
			
			if($i->get('action') == "add")
			{
				$link_url = 'http://remotelystartedmn.com/quotes.php?quote-id='.$last_id.'&c='.$code.'&direct';
				$email_link_url = $link_url.'&e=1';
				
		
				$set = $this->db->update(QUOTE_RESPONSES, [/*'signature' => $signature, */'quote_link' => $email_link_url], '`id`= '.$last_id);
			
				
				$qid = $i->get('quote-id', '', 'int');
				(new QuoteRequest)->updateResponseType($qid);

maxxd · July 2, 2015

The number of times you're running a conditional expression on the value of $i->get() is a definite smell. Personally, I handle this dynamically as so:

$function = 'do'.ucfirst(strtolower($i->get('action')));
$response = $this->$function($params);

You can consolidate all of your action-specific code within those methods and avoid not only all the conditional brackets, but confusing variable assignment like this

$last_id = ($i->get('action') == "add") ? $set->lastInsertId() : $resp_id;

because everything is black-boxed. So, in doAdd() you can refer the variable as $newRecordID and in doEdit() it can be $currentRecordID, improving the readability by specifying the purpose of the variable and the value it contains.

Of course, you'll want to check that the specified method exists in your object before attempting to call it, but that's the basic gist. You'll have functions doAdd() and doEdit() (for your example), but can easily handle other actions by adding doDelete(), doModify(), doMailSomebodySomethingAwesome(), etc.

Hope that made some sort of sense.

Edited July 2, 2015 by maxxd

fastsol · July 2, 2015

The number of times you're running a conditional expression on the value of $i->get() is a definite smell. Personally, I handle this dynamically as so:
$function = 'do'.ucfirst(strtolower($i->get('action')));
$response = $this->$function($params);
You can consolidate all of your action-specific code within those methods and avoid not only all the conditional brackets, but confusing variable assignment like this
$last_id = ($i->get('action') == "add") ? $set->lastInsertId() : $resp_id;
because everything is black-boxed. So, in doAdd() you can refer the variable as $newRecordID and in doEdit() it can be $currentRecordID, improving the readability by specifying the purpose of the variable and the value it contains.

Of course, you'll want to check that the specified method exists in your object before attempting to call it, but that's the basic gist. You'll have functions doAdd() and doEdit() (for your example), but can easily handle other actions by adding doDelete(), doModify(), doMailSomebodySomethingAwesome(), etc.

Hope that made some sort of sense.

I think I understand what you're getting at, but it doesn't seem to really solve a problem if you have to always do a method_exists or function_exists check every time you want to use the dynamic made function. Might as well just do it the way I am with still less code, unless I'm not understanding fully.

scootstah · July 2, 2015

I think I understand what you're getting at, but it doesn't seem to really solve a problem if you have to always do a method_exists or function_exists check every time you want to use the dynamic made function. Might as well just do it the way I am with still less code, unless I'm not understanding fully.

He's basically talking about the Routing layer that I was talking about earlier.

The Routing layer will look at the URI and determine where it needs to go. You can either do really loose approach where the first segment is always the controller, the second segment is always the method, and anything after that is arguments. Or, you can do it so that you have to define every route in a file and explicitly state which controller it goes to. I generally prefer the latter.

The first way would break up your URI string into segments. So if you had a URI string like: /news/view/17 then we could say that the first segment is supposed to be a controller, the second segment is supposed to be a method within that controller, and any additional segments are arguments to the controller method.

So like:

class NewsController
{
    public function viewAction($id)
    {
    }
}

You'd have some code that looks to see if that class and method exist, and then invoke it.

The other way is with explicitly defined routes. This is how a lot of big frameworks like Symfony2 work. So you'd defined a route with something like this:

$routes = array(
    '/news/view/{id}' => array(
        'controller' => 'NewsController',
        'method'     => 'view',
        'options'    => array(
            'httpMethod' => 'GET',
            'params'     => array(
                'id' => '\d+'
            ),
        )
    )
);

You'd have to do a bit more work in this case, but it scales better.

For what it's worth, the Symfony2 Routing component is decoupled and very usable in any project. A whole bunch of open source projects are using this now, including the new PHPBB forum software and Drupal 8. It's very fast and gives you a solid foundation for which to organize your controllers, without having to reinvent the wheel.

Edited July 2, 2015 by scootstah

maxxd · July 2, 2015

I am kinda talking about a Routing system, as scootstah says. However, it's also very possible to use it within the same object from the get-go.

//none of this is actually used in the code you posted, but I'm leaving it because I have to assume
//	it's important somewhere
$now = time();		
$units_text = "Prices include all parts and labor:\r\n".$email_units;
$where = '';
$date_sent = '';
$date_modified = NULL;

// INSERT/UPDATE QUOTE_RESPONSES
$params = [
	'request_id' => [$q_id, 'INT'],
	'date_modified' => NULL,
	'body' => $ebody,
	'code' => $code,
	'notes' => $notes,
	'unit_pricing' => $units_text
];

//this line becomes a hub of sorts - you can branch the flow anywhere you need to, including
//	other objects if that works with what you're doing.
$function = 'do'.ucfirst(strtolower($i->get('action')));
if(method_exists($this, $function)){
	$unit = $this->$function($params);
}

function doAdd(array $params/* any of the other parameters - see the comment */){
	$params['code'] = randomCode(20);
	$code = $params['code'];
	$set = $this->db->insert(QUOTE_RESPONSES, $params);
	$last_id = $set->lastInsertId();

//things start to get a bit weird here - where in the code you posted are $q_id, $code, and $i defined?
//	I mean, it really doesn't matter - you may just have to add them as parameters of the doX() methods,
//	but it is a bit confusing...

//update referrals
	(new QuoteReferral)->setQuoteId($last_id, $q_id);
	$link_url = 'http://remotelystartedmn.com/quotes.php?quote-id='.$last_id.'&c='.$code.'&direct';
	$email_link_url = $link_url.'&e=1';
	$set = $this->db->update(QUOTE_RESPONSES, [/*'signature' => $signature, */'quote_link' => $email_link_url], '`id`= '.$last_id);
	$qid = $i->get('quote-id', '', 'int');
	(new QuoteRequest)->updateResponseType($qid);
	
	return $whateverUnitIs;
}

function doEdit(array $params){
//again, there are a lot of undefined variables here that I assume are defined elsewhere in the code. So, pass those as
//	parameters into the doX() methods.
	$params['date_modified'] = date("Y-m-d H:i:s");
	$set = $this->db->update(QUOTE_RESPONSES, $params, '`id` = '.$resp_id, 1);
	$last_id = $resp_id;
//clear quote items
	$qi->deleteQuoteItems($resp_id);
	
	return $whateverUnitIs;
	
}

//Sorry, but I have no idea what this section is doing and what it has to do with the bulk of the code above
//	so I'm just kinda leaving it here...

// UPDATE QUOTE_REQUESTS with posted vehicle info.
$params = [
	'vehicle_make_id' => [$make_id, 'INT'],
	'vehicle_model_id' => [$model_id, 'INT'],
	'vehicle_year_id' => [$year, 'INT'],
	'vehicle_sufix' => $car_sufix
];

(new QuoteRequest)->updateById($params, $q_id);
///////// END //////////////////////////////////////////

// SET new QUOTE_ITEMS
foreach($unit as $key => $val)
{
	$params = [];
	
	if(!empty($val['price']))
	{
		$note = (!empty($val['note'])) ? sanitize($val['note']) : '';
		$unit_price = addSalesTax($val['price'], Config::get('tax_rate'));
		$add_amt = ($val['on_sale'] == TRUE) ? (int)$val['price'] - (int)$val['sale_price'] : (int)$val['price'] - (int)$val['original_price'];
		
		$params = [
			'item' => $key,
			'quote_price' => [$val['price'], 'INT'],
			'response_id' => [$last_id, 'INT'],
			'product_id' => [$val['product_id'], 'INT'],
			'item_note' => $note,
			'add_amt' => [$add_amt, 'INT']
		];
		
		$qi->insertQuoteItems($params);
	}
}
////////// END ///////////////////////////////

The nice thing, instead of the 5 or 6 instances of

if($i->get('action') == ...){

that you have to dig through to figure out which portion of the logic application you're dealing with when something goes wrong with the code, all the functionality for the add action and the edit action are consolidated and completely independent of each other.

scootstah · July 2, 2015

However, it's also very possible to use it within the same object from the get-go.

True, but then you're repeating that logic every time you need it. A typical routing layer sits in front of the controllers and everything is in one centralized location.

fastsol · July 2, 2015

Ahh, now I see the potential maxxd. As much as I'd love to integrate my cms into a far superior MVC type thing, it would be mind boggling hard and time consuming. It was built in procedural with tons of features and tons of files that would need complete recoding. So I'll just have to make it work in sections that are important to make easier to upgrade, like my quote system.

maxxd · July 2, 2015

True, but then you're repeating that logic every time you need it. A typical routing layer sits in front of the controllers and everything is in one centralized location.

Very true. But if you're only working in the one controller with one action target, you can use the same idea to route the functionality flow throughout that controller. No biggie, just a thing.

Ahh, now I see the potential maxxd. As much as I'd love to integrate my cms into a far superior MVC type thing, it would be mind boggling hard and time consuming. It was built in procedural with tons of features and tons of files that would need complete recoding. So I'll just have to make it work in sections that are important to make easier to upgrade, like my quote system.

Glad that example made at least a little sense!

scootstah · July 2, 2015

So I'll just have to make it work in sections that are important to make easier to upgrade, like my quote system.

If it were me I'd go the opposite direction. Leave your quote stuff alone, and fix your underlying foundation. Your quote system is working, and isolated to a couple classes. It's going to be challenging to break that into proper OOP when the rest of your system is procedural spaghetti.

fastsol · July 2, 2015

I've been battling this all day and haven't gotten nearly as far as I had hoped. I've made many new class pages and broken out many aspects, but I still can't get the flow down in my head. That processquote() does so much and it needs to do it in a certain order that I can't figure out how to break it apart without literally breaking it.

Sign In

I think my classes have to much responsibility

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Important Information