Service Layer Design

Question

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.

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;
     }
}

Answer 1

Maybe it's a matter of preference, but IMO Exceptions shouldn't be used for validation errors (see:Don't Use Exceptions For Flow Control). First, it's not that exceptional and second, you should still handle user friendly response: re-display form with error messages (or error list only in case of ajax request). Insetad exception one-liner you should build (View)Model that gets messages from validator and (invalid) $_POST data.

Other Exceptions (league/country/insert) are ok. They're thrown when user have done something illegal (bypassed GUI). On the other hand it doesn't deserve any specified message then (serve debugging purpose only).

The thing that looks smelly is collaborating with league and country mappers. I wouldn't use them at all in this context. I would either handle it with db constraints (foreign keys) that would throw me db exception* or (if I wanted to decide details later) handle it within $this->teamMapper->insert() method. If you intend to use ORM there are propably some sophisticated methods for such constraints too, but can't tell for sure - got allergy to ORM.

*) The downside of using key constraints only is relying on something that is hidden outside the sourcecode. Db schema might be messed up (during migration for example) and vulnerability might pass unnoticed. Unless you have full controll over your code it's better to make checks anyway (preferably using transactions).

Answer 2

So there is an updated version of your code, the main idea is to build a separate function to validate the datas !

But before, some little advices:

• Don't forget the visibility operator for method definition: public, private or protected.
• You don't need to repeat Team in the addTeam method, we are already in the TeamService class, so it's obvious we are going to add teams.

And there is an updated version of your code:

class TeamService implements TeamServiceInterface
{

    function add($name, $country, $league, $active) //if we are in the TeamService, you don't have to repeat yourself
    {
        $teamMapper = new TeamMapper(new CountryMapper(), new LeagueMapper(new TeamMapper(), new MatchesMapper()));

        if (!$this->teamMapper->validate($name, $country, $league)) return false;

        $this->teamMapper->insert('...');
    }
}

class TeamMapper implements TeamMapperInterface
{

    private $countryMapper;
    private $leagueMapper;

    //don't forget the visibility operator
    public function __construct(CountryMapperInterface $countryMapper, LeagueMapperInterface $leagueMapper)
    {
        $this->countryMapper = $countryMapper;
        $this->leagueMapper = $leagueMapper;
    }

    public function validate($name, $country, $league)
    {
        if ($this->teamMapper->isTeamByName($name) === true
            || $this->countryMapper->isCountryById($country)
            || $this->leagueMapper->isLeagueById($league)
        ) return false;

        return true;
    }

    public function insert($name, $country, $league) //we assume datas are valide !
    {
        if(!$this->validate($name, $country, $league)) // in this cas it's not a control flow
            throw new \Exception('Validation error you must validate your data before using it!');

// If all the criterion are met then add the team to the he database

// *** MYSQL Code To Add Team To Database ***

    }
}

And at least, I don't really understand what are these mapper class for? If it's to build a layer between your entities and the database it's often called a Repositoy like TeamRepository.