3
\$\begingroup\$

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;
     }
}
\$\endgroup\$
2
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ I think it would probably be easier to go with the foreign key constraints in the database then. Is this an acceptable approach though as you leave the validation of the business rules until last and could potentially have an invalid data model? I'm currently using codeigniter as my website framework so would it be better to validate the $_POST data in say the controller and then invoke the service once the data has passed the basis validity checks? \$\endgroup\$ – evans123 Mar 22 '15 at 18:28
  • \$\begingroup\$ I wouldn't move validation rules into controller and considering the fact that integrity validation is moved into db engine would make it even worse because model rules would be spread from top to bottom of the application which is hard to maintain. That said, there is a downside of using key constraints only (see: edited answer). \$\endgroup\$ – shudder Mar 22 '15 at 20:21
  • \$\begingroup\$ That's fair enough. Would you return the validation object from the service method instead of say "false" if any of the rules fail so that you could then access the error messages? If you handle the constraints within the $this->teamMapper->insert() would it be acceptable to run database queries on a table which isn't related to the mapper or would you have to inject the other mappers into it? It does seem that an ORM might be more suitable for handling this. Thanks for your help so far. \$\endgroup\$ – evans123 Mar 23 '15 at 7:56
  • \$\begingroup\$ Can't give you ultimate answer to handling invalid form - too many design choices to give precise answer (pros/cons/prefs). I could suggest full PRG pattern with redirection to temporary invalid object, but maybe you don't need to make it that complicated - can't tell. Still false+getErrors better than throw me anything and I figure it out approach (command query separation, avoid side effects). The same with mappers. You could strive for loose coupling/object translation on low levels, but mind its costs or what you'll sacrifice if make team coupled with its country/league. \$\endgroup\$ – shudder Mar 23 '15 at 10:38
  • \$\begingroup\$ I understand what you are saying about trying to strieve towards loose coupling etc., but I can’t quite get my head around how you’d go about doing it – probably because I’m thinking in a procedural way! If I removed the league / country variables from the team model completely and then injected them all into the $this->teamMapper->insert(TeamInterface $team, CountryInterface $country, LeagueInterface $league) method as individual objects would this achieve what I want? The necessary id’s could then be retrieved by accessing the individual classes getId() method. \$\endgroup\$ – evans123 Mar 23 '15 at 13:49
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Yeah, the mapper classes are used as a layer between the entities and the database. I called them mappers because that's what a website I was referring to called them as well, but I suppose repository is a more relevant term in this instance. If you needed to validate the inputs to the service class (length, type etc.) I'm assuming that you'd create a separate class for this and then just pass in all of the values and rules before returning the relevant response + errors. \$\endgroup\$ – evans123 Mar 25 '15 at 13:20
  • \$\begingroup\$ Take a look to this Symfony component: github.com/symfony/Validator \$\endgroup\$ – lilobase Mar 25 '15 at 13:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.