Created extensions adding business day functions #706
Conversation
I created a set of extension methods which provides a flexible set of methods that allow the determination if a date is a business day as well as determining the next or previous business day. Custom holiday and business schedules can be supplies to the methods to allow an end-user to determine what holidays, if any, are to be observed and the days that fit operating schedule of the business being modeled.
| // Check to see if a date is a business day defaulting to a Monday | ||
| // through Friday work week | ||
| $today = Carbon::now(); | ||
| CarbonExtensions::isBusinessDay($today); |
There was a problem hiding this comment.
\Carbon\Carbon::isWeekday already exists for that
There was a problem hiding this comment.
isWeekday is not the same as isBusinessDay. Weekdays are Monday-Friday whereas a business might only be open Friday-Sunday. Additionally, isBusinessDay checks to see if the day is a holiday if a holiday schedule is passed in as a parameter.
For example: A business operates on a Monday - Friday schedule. The current date is on a Friday and the following Monday is a holiday. You need to know what the date is going to be for 3 business days from now. Since the following Monday is a holiday the date would be for the following Thursday.
While the basic functionality of isBusinessDay does the same as isWeekday, the difference is that it allows for but does not require custom business and holiday schedules.
|
+1. Using setWeekendDays and afterward using addWeekdays seems to not have any effect at all. |
|
@briannesbitt whats your opinion on that? |
|
@rmblstrp It seems the test are failing... |
| * | ||
| * @author Christopher McGinnis <[email protected]> | ||
| * | ||
| * @param Carbon $date |
There was a problem hiding this comment.
for param, return and throw docblocks we use FQN class name
| { | ||
| do { | ||
| $date->addDay(); | ||
| } while (!self::isBusinessDay($date, $schedule)); |
There was a problem hiding this comment.
Can you call with static:: instead ?
| */ | ||
| public static function addBusinessDays(Carbon $date, $days, $schedule = null) | ||
| { | ||
| while ($days != 0) { |
There was a problem hiding this comment.
Any reason not to use a stricter comparison !== ?
|
|
||
| interface HolidaySchedule | ||
| { | ||
| public function isHoliday(Carbon $date); |
| use Carbon\HolidaySchedule; | ||
|
|
||
| /** | ||
| * Class FederalHolidays |
There was a problem hiding this comment.
Can you remove the class name declaration in this docblck ?
| * | ||
| * @property-read int $year; | ||
| * @property int $observedHolidays; | ||
| * @property-read Carbon $newYearsDay; |
There was a problem hiding this comment.
\Carbon\Carbon instead
| * | ||
| * @returns bool | ||
| */ | ||
| public function isHoliday(Carbon $date) |
There was a problem hiding this comment.
Carbon $carbon instead
| */ | ||
| class FederalHolidays implements HolidaySchedule | ||
| { | ||
| const NEW_YEARS_DAY = 0x0001; |
There was a problem hiding this comment.
Missing docblocks for const and properties
| // created a copy of the datetime object to we can remove the time values without affecting | ||
| // the passed in instance | ||
|
|
||
| return ($this->newYearsDay->isSameDay($date) && $this->isObserved(self::NEW_YEARS_DAY)) |
There was a problem hiding this comment.
I would separate this huge conditions in smaller conditions
| */ | ||
| protected function getNewYearsDay() | ||
| { | ||
| return Carbon::create($this->year, 1, 1, 0, 0, 0); |
There was a problem hiding this comment.
Why not calling Carbon::now()->startOfYear()?
|
Seems possible, but waiting to v2 might be a good idea. Not sure why "CarbonExtensions" is so generic where the interface is specific. Seems like a more specific class name makes more sense ? |
|
@briannesbitt Agreed. I put this together because we needed this functionality in a work projects. I'll retool it a bit so it makes more sense and get it ready for the 2.x release. |
|
up? |
Glavic
added
the
macro candidate
|
Hi, I started this library (compatible with the coming soon 1.26 version) based on @rmblstrp work: You're all invited to test, contribute, add holidays lists, etc. |
kylekatarnls
added
documentation
|
I close this issue as kylekatarnls/business-day should fulfill the need. If something is missing please open an issue here: https://github.com/kylekatarnls/business-day/issues |
I created a set of extension methods which provides a flexible set of
methods that allow the determination if a date is a business day as
well as determining the next or previous business day. Custom holiday
and business schedules can be supplies to the methods to allow an
end-user to determine what holidays, if any, are to be observed and the
days that fit operating schedule of the business being modeled.
NOTE: I know that this functionality has been asked for before but it seemed that you didn't want to include the functionality into the Carbon class. I felt that extending Carbon would be a bad idea because it would lock implementations to a certain subclass and created a hierarchical mess if there were multiple subclasses with some users wanting functionality that was contained in 2 or more subclasses. Because of those reasons I opted to create an extension class where other contributors could add in functionality to extend Carbon while keeping the Carbon class pure. If that functionality is deemed worthy enough to be promoted to the Carbon class it can be done easily without breaking backwards compatibility and without having duplication of code.