2
\$\begingroup\$

I am building a simple calendar with recurring events.

CalendarEvent.php

id
start_date

CalendarRecurringPattern.php

parent_id
pattern_type // daily, weekly, monthly, yearly ...

Using pattern_type i build each following occurrence starting from start_date, using a recursive function and Carbon::class. What bugs me is the function getNextOccurrence(), I'd like to refactor the if/else block because it's not very clean. Here it is:

use \Carbon\Carbon;
....

public function getNextOccurrence(Carbon $startDate) {

    $pattern_type = $this->getPatternType() // daily, weekly...

    if ($pattern_type == 'daily') {
        $startDate->addDay();
    } else if ($pattern_type == 'weekly') {
        $startDate->addWeek();
    } else if ($pattern_type == 'monthly') {
        $startDate->addMonth();
    } else if ($pattern_type == 'yearly') {
        $startDate->addYear();
    }

}

Thanks!

\$\endgroup\$
2
  • 2
    \$\begingroup\$ the commonplace suggestion would be to use switch, but honestly I don't see what's wrong with a lot of nested if statements. Once I was eager to refactor this kind of code, but now I learned to value its verbosity and clean meaning, so I wouldn't change it into something more "elegant" but less readable \$\endgroup\$ Jun 20, 2018 at 13:04
  • \$\begingroup\$ Thanks. I'm still growing as a developer and I'm doing my best to avoid bad habits. Recently I have been reading a lot about unit testing, a field in which I have no experience, and that function seemed not very clean from a testing perspective. \$\endgroup\$
    – Enrico
    Jun 20, 2018 at 15:01

1 Answer 1

3
\$\begingroup\$

I don't personally see a problem with the way its currently structured as there aren't that many options, but you could do it a couple of other ways;

Option 1

Array function name mapping (hacky name I just came up with), you could do the following;

$functionMap = [
    "daily" => "addDay",
    "weekly" => "addWeek",
    "monthly" => "addMonth",
    "yearly" => "addYear"
];

if(!isset($functionMap[$pattern_type])){
    throw new \Exception("Can't find patern type $pattern_type", 1);
}

$startDate->$functionMap[$pattern_type]();

This might be seen as a sin by the PHP community but it works none the less

Option 2

You could use a switch which probably isn't a sin by the community.

switch ($pattern_type) {
    case "daily":
        // Do the daily task
        break;
    case "weekly":
        // Do the weekly task
        break;
    case "monthly":
        // Do the monthly task
        break;
    case "yearly":
        // Do the yearly task
        break;
    default: 
        throw new \Exception("Can't find pattern type $pattern_type", 1);
}

Conclusion

I would add some exception / error that can be picked up if a non matched pattern_type is passed.

I would also convert the "magic strings" E.G "daily" to constants in case you need to re-use, change or update them in the future!

In terms of line count;

  • your way 10 lines of code
  • option 1 11 lines of code
  • option 2 15 lines of code

The switch & if statements may have some advantages when doing code analysis aswel where the expected required input values can be picked up.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for this. I'm wondering, why should the option n.1 be seen as a sin by the community? I find myself using the same concept many times with javascript. \$\endgroup\$
    – Enrico
    Jun 20, 2018 at 15:04
  • 1
    \$\begingroup\$ For example unit testing, unit testing would show this code as 100% tested even if you only tested one $pattern_type because there is only one path the code can take \$\endgroup\$
    – Dan
    Jun 20, 2018 at 17:07

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.