3
\$\begingroup\$

I wrote a little class that will calculate 6 business days from today. The reason it's in a class is because it will be used as a part of a bigger program.

I would like a way to refactor this class and make it a little more object oriented along with a little prettier.

from datetime import datetime, timedelta


class BusinessDay(object):
    """
    Calculate a business day of 6 days skipping Saturday and Sunday
    """
    return_date = []  # Array to store the calculated day

    def __init__(self, start_day):
        """
        :type start_day: Object, datetime object
        """
        self.start_day = start_day

    def calc_bus_day(self):
        """Calculate 6 business days from the self.start_day example:
        >>> import BusinessDay
        >>> test = BusinessDay(datetime(2016, 9, 14))
        >>> test.calc_bus_day()
        >>> #2016-09-22
        """
        days_skipped = self.start_day + timedelta(8)

        if datetime.isoweekday(days_skipped) in range(2, 6):
            print("Date {} is a weekday.".format(self.start_day))
            self.return_date = days_skipped
            print("Six days from today will be {}".format(self.return_date))
        else:
            print("Date {} is a weekend.".format(self.start_day))
            self.return_date = days_skipped + timedelta(2)
            print("Six days from the weekend will be {}".format(self.return_date.strftime("%m-%d-%Y")))

        if self.return_date.isoweekday() is 1 or 7:
            if self.return_date == 1:
                self.return_date = self.return_date + timedelta(2)
                return self.return_date - timedelta(2)
            else:
                self.return_date = self.return_date + timedelta(1)
                return self.return_date - timedelta(2)
\$\endgroup\$
  • \$\begingroup\$ Why would you want to make this code object-oriented? What are the objects here, the classes? What do they add? This code should be a single function. Don't just blindly try to make code OO; try to make code the best it can be, and use whatever tools are necessary to achieve that. \$\endgroup\$ – gardenhead Sep 14 '16 at 22:27
  • \$\begingroup\$ @gardenhead It's going to be apart of a much bigger program, and this is just the first section of this class. \$\endgroup\$ – YoYoYo I'm Awesome Sep 15 '16 at 0:18
  • \$\begingroup\$ That doesn't answer my question though. Large program =/= make everything an object. If you're structuring a large project, the primary concern is modularity, and you have to carefully think about how you're going to structure your code to achieve that modularity. In this case, a vanilla function (either grouped with similar functionality in a module or by itself) is the clearest way to go. \$\endgroup\$ – gardenhead Sep 15 '16 at 0:36
  • \$\begingroup\$ @gardenhead I was thinking about this last night, and I agree with you, I'm going to move this into a different class with a bunch of other settings type functions \$\endgroup\$ – YoYoYo I'm Awesome Sep 15 '16 at 12:35
7
\$\begingroup\$

Bug

if self.return_date.isoweekday() is 1 or 7:

Is always true because 7 is true, adding parenthesis will help you understand:

 if  ( self.return_date.isoweekday() is 1 ) or 7:

To get proper behaviour use:

 if self.return_date.isoweekday() in (1, 7):

Repetition

self.return_date = self.return_date + timedelta(2) 
return self.return_date - timedelta(2)

And:

self.return_date = self.return_date + timedelta(1) 
return self.return_date - timedelta(2)

Use a if/ternary for the first timedelta arguments and flatten these lines.

No class

You have a single function with a single argument, simplify and avoid a class. (You can re-use your function without a class, just put it in a separate module and import it.)

\$\endgroup\$
  • 1
    \$\begingroup\$ Amen. Don't overbuild stuff just because you think OO is cool 'cause everyone says it is. Great answer, I would just add that instead of worrying about how pretty the code is, maybe they should of written tests to see that they have a major bug in it like the one you found. \$\endgroup\$ – ldog Sep 14 '16 at 23:54
  • \$\begingroup\$ @Idog this is only the first section of this class, took me forever to figure this out and I wanted some input, I do agree that it's a bit over complicated. I did test it, that's why I posted it, but this is really my first real program in Python, so you know, I have absolutely no idea what I'm doing lol. \$\endgroup\$ – YoYoYo I'm Awesome Sep 15 '16 at 0:23
  • 1
    \$\begingroup\$ @Caridorc can't believe I didn't catch the bug, I thought I had this perfect lol. Thank you for the answer and the input, much appreciated, like always. \$\endgroup\$ – YoYoYo I'm Awesome Sep 15 '16 at 0:24
2
\$\begingroup\$

a look at the code

    days_skipped = self.start_day + timedelta(8)

I might rename this variable. The code just skipped eight days, it might end up on an end_day, or something like that? (Might want to comment that its eight because there must be a weekend within those six days. This is still pretty clear, but it will get less straightforward if you ever allow for variable skips.)

    if datetime.isoweekday(days_skipped) in range(2, 6):
        print("Date {} is a weekday.".format(self.start_day))
        self.return_date = days_skipped
        print("Six days from today will be {}".format(self.return_date))

This is a bit confusing, since you're testing the date 8 days from now, but printing what the current day is. Why not just test if the starting day is from Monday to Friday, if that's what you mean: 1 <= datetime.isoweekday(self.start_day) <= 5. Also, in range(2,6) is True for values 2,3,4 and 5, but not for 6, which is probably not what you want.

    else:
        print("Date {} is a weekend.".format(self.start_day))
        self.return_date = days_skipped + timedelta(2)
        print("Six days from the weekend will be {}".format(self.return_date.strftime("%m-%d-%Y")))

Also, if this is supposed to skip to the start of the week in case the starting day falls on Sat or Sun, I think you should skip ahead either 1 or 2 days depending on which one it is, instead of always skipping 2.

    if self.return_date.isoweekday() is 1 or 7:

Like @Caridorc wrote in their answer, this is always true. But even you meant self.return_date.isoweekday() in (1, 7), why are Monday and Sunday special cases here?

        if self.return_date == 1:

I don't think this will ever hold. I guess you meantself.return_date.isoweekday() == 1

            self.return_date = self.return_date + timedelta(2)
            return self.return_date - timedelta(2)

This adds two, then subtracts two. Confusing, and no effect.

        else:
            self.return_date = self.return_date + timedelta(1)
            return self.return_date - timedelta(2)

Again, add one, subtract two? Wouldn't it be simpler to just subtract one? But if this subtraction runs on Sunday, you'd end up on a Saturday, right?

Should you return something after the final if? Now it only returns anything if the test for 1 or 7 matches.

algorithm

Actually, I can't understand your algorithm from this code, so I can't say if it's correct. Mostly because of the confusing conditionals. I would approach something like this by writing the logic down, in some human language if necessary, and then implement that.

(Actually, skipping a fixed number of days is almost painfully simple, since it'll always go from a known weekday to another known weekday. Mon->Tue, Tue->Wed, Wed->Thu, Thu->Fri, Fri,Sat or Sun->Mon, so 8,8,8,8,10,9 or 8 days ahead, depending on the starting weekday.)

structure

I think this could be condensed to a single function. Preferably one that would take the starting date and number of days to skip. Even if you need a larger class around it, it would be easy to just drop the function in as a utility.

\$\endgroup\$
  • \$\begingroup\$ 6 is Saturday, so 2,6 is exactly what I want \$\endgroup\$ – YoYoYo I'm Awesome Sep 14 '16 at 20:58
  • \$\begingroup\$ @YoYo, mmh, If the starting date is a Friday, then 8 days ahead is a Saturday, which will not match the range, and the code will print Date 2016-09-16 00:00:00 is a weekend.. But it's not, it's a Friday. (It'll print "is a weekend" for Fri, Sat and Sun) \$\endgroup\$ – ilkkachu Sep 14 '16 at 21:06

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.