HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Service Layer Design

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
layerservicedesign

Problem

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($cou

Solution

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

Context

StackExchange Code Review Q#84724, answer score: 2

Revisions (0)

No revisions yet.