Skip to main content
Tweeted twitter.com/#!/StackCodeReview/status/579854164102135808
deleted 66 characters in body; edited title
Source Link
Jamal
  • 34.9k
  • 13
  • 133
  • 237

PHP OOP - Service Layer Design

I have written the following service class for my application and at the moment it only contains a single method which is responsible for carrying out the necessary actions to add a new team to the database - more. More methods will be added later but I want to make sure that I'm on the right track with regards to OOP and it's various principles.

I've only recently started to write in the OOP style, so it would be great to get some feedback on whether this is the right approach to take. The class seems to have a lot of dependencies which must be injected through the constructor method so I'm sure there is a better way to do it. In addition, the code within the addTeam() method still looks very procedural like and probably violates the single responsibility principle too.

Any help in improving the code below would be appreciated.

PHP OOP - Service Layer Design

I have written the following service class for my application and at the moment it only contains a single method which is responsible for carrying out the necessary actions to add a new team to the database - more methods will be added later but I want to make sure that I'm on the right track with regards to OOP and it's various principles.

I've only recently started to write in the OOP style, so it would be great to get some feedback on whether this is the right approach to take. The class seems to have a lot of dependencies which must be injected through the constructor method so I'm sure there is a better way to do it. In addition, the code within the addTeam() method still looks very procedural like and probably violates the single responsibility principle too.

Any help in improving the code below would be appreciated.

Service Layer Design

I have written the following service class for my application and at the moment it only contains a single method which is responsible for carrying out the necessary actions to add a new team to the database. More methods will be added later but I want to make sure that I'm on the right track with regards to OOP and it's various principles.

I've only recently started to write in the OOP style, so it would be great to get some feedback on whether this is the right approach to take. The class seems to have a lot of dependencies which must be injected through the constructor method so I'm sure there is a better way to do it. In addition, the code within the addTeam() method still looks very procedural like and probably violates the single responsibility principle too.

Source Link

PHP OOP - Service Layer Design

I have written the following service class for my application and at the moment it only contains a single method which is responsible for carrying out the necessary actions to add a new team to the database - more methods will be added later but I want to make sure that I'm on the right track with regards to OOP and it's various principles.

The current addTeam() method essentially accepts a number of parameters, validates them to ensure that they are in the correct format and then determines if they violate any business rules i.e. team name must be unique and the league & country id's must already exist in the database.

I've only recently started to write in the OOP style, so it would be great to get some feedback on whether this is the right approach to take. The class seems to have a lot of dependencies which must be injected through the constructor method so I'm sure there is a better way to do it. In addition, the code within the addTeam() method still looks very procedural like and probably violates the single responsibility principle too.

Any help in improving the code below would be appreciated.

class TeamService implements TeamServiceInterface {

     private $validator;
     private $teamMapper;
     private $leagueMapper;
     private $countryMapper;

     function __construct(ValidatorInterface $validator, TeamMapperInterface $teamMapper, LeagueMapperInterface $leagueMapper, CountryMapperInterface $countryMapper)
     {
         // Attempt to populate the class's variables with the information that has been passed to the object
         $this->validator = $validator;
         $this->teamMapper = $teamMapper;
         $this->leagueMapper = $leagueMapper;
         $this->countryMapper = $countryMapper;
     }

     public function addTeam($name, $country, $league, $active)
     {
         // Set the rules for the data that we want to pass to our team object
         $validator->setRule($name, 'name', 'required|alpha_numeric_ampersand_space|min_length[3]|max_length[30]');
         $validator->setRule($country, 'country', 'required|is_natural_no_zero');
         $validator->setRule($league, 'league', 'required|is_natural');
         $validator->setRule($active, 'active', 'required|boolean');

         // Execute the validation rules
         if($validator->run() === false)
         {
             throw new Exception('The information provided doesn\'t meet the required validation rules.');
         }

         // Check to see if the team name already exists in the database
         if($this->teamMapper->isTeamByName($name) === true)
         {
             throw new Exception('The name specified for the team already exists in the system.');
         }

         // Check to see if the country id specified exists in the database
         if($this->countryMapper->isCountryById($country))
         {
             throw new Exception('The country id specified could not be found in the system.');
         }

         // Check to see if the league id specified exists in the database
         if($this->leagueMapper->isLeagueById($league) === null)
         {
             throw new Exception('The league id specified could not be found in the system.');
         }

         // Create an instance of the team class
         $team = new Team($name, $country, $league, $active);

         // Attempt to persist the new team to the database
         if($this->teamMapper->insert($team) === false)
         {
             // Throw an exception because a certain event happened
             throw new Exception('The were was a problem adding the team to the system.');
         }

         return true;
     }
}