Does anyone see anything wrong with how this is written as it shows below?
public function injure($injuredAt = null)
{
if ($this->canBeInjured()) {
$injuredDate = $injuredAt ?? now();
$this->injuries()->create(['started_at' => $injuredDate]);
$this->save();
if (optional($this->currentTeam)->isBookable()) {
$this->currentTeam->touch();
$this->currentTeam->refresh();
}
return $this;
}
}
public function canBeInjured()
{
if ($this->isUnemployed() || $this->isReleased() || $this->hasFutureEmployment() || $this->isRetired()) {
throw new CannotBeInjuredException('Entity cannot be injured. This entity does not have an active employment.');
}
if ($this->isInjured()) {
throw new CannotBeInjuredException('Entity cannot be injured. This entity is currently injured.');
}
if ($this->isSuspended()) {
throw new CannotBeInjuredException('Entity cannot be injured. This entity is currently suspended.');
}
return true;
}
if ($this->isNotInActiveEmployment()) {
throw new CannotBeInjuredException('Entity cannot be injured. This entity does not have an active employment.');
}
@automica Done! Great refactor. The only other thought I had was about if the injure() was actually fluid. By this I mean Is there any problem with the fact that that injure() will never return false. It iwll other return the current class instance because the canBeInjured() will be responsible for throwing the Exception? Is this an okay thing to do?
@xtremer360 as $this->canBeInjured()) will either throw an exception or return true your injured method will currently only succeed or reflect the same exception,
The only time you might get false is if the save fails so you might want to extract the save part to a separate method, wrap a try catch around that part, and return a false then.
@xtremer360 I think its pretty unlikely for your db to fail. if it's going to, it probably would have done earlier, so its up to you if you want the additional check.
