I think my classes have to much responsibility

[fastsol]

Well I've decided to abandon this project for now.  I need to learn too much to make it right and I don't have the time right now.  Hopefully next spring I can rebuild the entire site in a solid framework and all will be well.

 

But for now, I do have a couple small classes that I would like an opinion on how to improve.  They are for connecting to and using the Google Calendar API.  One class handles the connecting and the other makes the calls.  I know for sure that one class is totally dependent on another static class in my system and I understand that that is wrong.  I understand the concept of dependency injection but find the overall concept of throwing classes around to inject isn't good either.  I'm sure that a solid framework solves those issues, but for now this is what I have to work with.

 

So guidance would be greatly appreciated, examples speak louder

 

GoogleAuth class

<?php

class GoogleAuth
{
	protected $client;
	
	public function __construct(Google_Client $googleClient)
	{
		$this->client = $googleClient;
		
		if($this->client)
		{
			$this->client->setApplicationName(Config::get('cal_cred/cal_project_name'));
			$this->client->setAccessType("offline");
			
	 		/*if (!empty($_SESSION['service_token'])) {
			  $this->client->setAccessToken($_SESSION['service_token']);
			}*/
			
			$cred = new Google_Auth_AssertionCredentials(	 
				Config::get('cal_cred/cal_email_address'),	 	 
				Config::get('cal_cred/cal_scopes'),	 	
				file_get_contents(Config::get('cal_cred/cal_p12_file'))	 	 
			);	
			 	
			$this->client->setAssertionCredentials($cred);
			//diagDiv($this->client);
			if($this->client->getAuth()->isAccessTokenExpired())	 	
			{ $this->client->getAuth()->refreshTokenWithAssertion($cred); }	
			
			//$_SESSION['service_token'] = $this->client->getAccessToken(); 	
		}
	}
}


?>

GoogleCalendar class

<?php

class GoogleCalendar
{
	protected $setup;
	protected $event_params;
	protected $cal_event;
	protected $createdEvent;
	protected $gsc;
	protected $auth;
	protected $cal;
	protected $lastInsertId;
	protected $deletedEvent;
	protected $updatedEvent;
	protected $error;

	public function __construct()
	{
		require_once(ROOT.'site_specific/other_pages/Google/autoload.php');
		
		$this->client = new Google_Client;
		$this->auth = new GoogleAuth($this->client);
		$this->gsc = new Google_Service_Calendar($this->client);
	}
	
	// Must provide: desc, start and end elements. location is an optional element.
	// start and end must be in this format 2015-06-04T11:30:00
	public function event_params(array $params)
	{
		$this->event_params = $params;
		return $this;
	}
	
	public function calendarEvent()
	{
		$this->cal_event = '';
		
		$this->cal_event = new Google_Service_Calendar_Event();
		
		$this->cal_event->setSummary($this->event_params['desc']);
		
		(!empty($this->event_params['location'])) ? $this->cal_event->setLocation($this->event_params['location']) : '';
		
		$start = new Google_Service_Calendar_EventDateTime();
		$start->setTimeZone(Config::get('cal_cred/cal_timezone'));
		$start->setDateTime($this->event_params['start']);
		$this->cal_event->setStart($start);
		
		$end = new Google_Service_Calendar_EventDateTime();
		$end->setTimeZone(Config::get('cal_cred/cal_timezone'));
		$end->setDateTime($this->event_params['end']);
		$this->cal_event->setEnd($end);
		
		return $this;
	}
		
	public function insert_event()
	{
		try
		{
			$this->createdEvent = $this->gsc->events->insert(Config::get('cal_cred/cal_name'), $this->cal_event);
			$this->lastInsertId = $this->createdEvent->getId();
		}
		catch(Google_Service_Exception $e)
		{
			$this->error = $e;
		}
		
		return $this;
	}
	
	public function update_event($event_id)
	{
		try
		{
			$this->updatedEvent = $this->gsc->events->update(Config::get('cal_cred/cal_name'), $event_id, $this->cal_event);
		}
		catch(Google_Service_Exception $e)
		{
			$this->error = $e;
		}
		
		return $this;
	}
	
	public function quickAdd_event($info)
	{
		try
		{
			$this->quickAddEvent = $this->gsc->events->quickAdd(Config::get('cal_cred/cal_name'), $info);
			$this->lastInsertId = $this->quickAddEvent->getId();
		}
		catch(Google_Service_Exception $e)
		{
			$this->error = $e;
		}
		
		return $this;
	}
	
	public function delete_event($event_id)
	{
		try
		{
			$this->deletedEvent = $this->gsc->events->delete(Config::get('cal_cred/cal_name'), $event_id);
		}
		catch(Google_Service_Exception $e)
		{
			$this->error = $e;
		}
		
		return $this;
	}
	
	public function lastInsertId()
	{
		return $this->lastInsertId;
	}
	
	public function error()
	{
		return $this->error;
	}
	
	public function getItemById($id)
	{
		return $this->gsc->events->get(Config::get('cal_cred/cal_name'), $id);
	}
}

?>

This is how I would typically use these classes

if(Config::get('use_gcal') === TRUE)
{
	$gc = new GoogleCalendar;
	
	$event = $gc->getItemById($google_id);
	
	$time_format = "Y-m-d\TH:i:s";
	$stime = date($time_format, $post_time);
	$etime = date($time_format, strtotime("+2 hours", $post_time));
	
	$google_params = [
		'desc' => $event->summary,
		'location' => html_entity_decode(Config::get('waiting_spot_options/'.$wait_id), ENT_QUOTES),
		'start' => $stime,
		'end' => $etime
	];
	
	$gc->event_params($google_params);
	$gc->calendarEvent();
	$gc->update_event($google_id);
}

[scootstah]

I understand the concept of dependency injection but find the overall concept of throwing classes around to inject isn't good either.

DI is much better, in fact. It makes testing possible, and decouples your components. What if you wanted to take some of your classes and reuse them in another project? With hard coded dependencies it's basically not feasible to do so.

 

Your other problem is that you're using static classes. You sort of lose most of the benefits of classes if you do that, you might as well just have a bunch of functions.

 

If you instantiated Config, you could pass it as a constructor argument to GoogleAuth, or wherever it's needed. You've now decoupled your dependencies. If you want to reuse GoogleAuth in the future, you know that it depends on a Config class, but it is not implementation specific.

[fastsol]

Yeah, I understand those apsects.  The Config class is static mainly cause it's just used to get values from a certain array of values across the entire system.  It makes it easier and faster to check certain vars by not having to use isset first and then check it's value, cause the class does all that for you.  And yes I suppose it could have been made non-static but then I had to deal with scope and I just wanted a easier way around certain things.  I fully understand scope and how the DI takes care of it.

 

Beyond that stuff, are the classes decent?  I know they're small so they can't be too bad and it's not like I don't know what I'm doing with php, just not totally with class methodology.

[ignace]

Beyond that stuff, are the classes decent?

No, they are not. Your constructor does a require_once() which means that with every instance you'll load the same file over and over. Move it somewhere else or at the top of the file before your class definition.

 

public function __construct(Google_Client $googleClient)
	{
		$this->client = $googleClient;
		
		if($this->client)

Not sure why you think you need to test $this->client, in case PHP wouldn't PHP?

[fastsol]

Yes I suppose the if($this->client) is redundant since the class is already type hinting for such a client.  I did know about the require_once issue too, not really sure why at the time I put in the class rather than the global system.  Thanks for the insights.