10
\$\begingroup\$

I was doing the following exercise:

Create an algorithm which asks the user a number from 1 to 7, and each number represents a day on the week. For example, 1 represents Sunday, 2 represents Monday and so on. The user will type this number and the program will show them the name of the day on the week and if it's a business day or a weekend.

I tried to do it in a object-oriented way and using Python. Can someone please verify if my code is within the Python and object-oriented "standards"? Because I'm not sure whether it is, and I need a review.

Here's my code:

class DayOnWeek:
    ERROR_MESSAGE = 'Invalid weekday. Try again:'
    NUMBER_NAME_ASSOCIATIONS = {
        1: 'Sunday',
        2: 'Monday',
        3: 'Tuesday',
        4: 'Wednesday',
        5: 'Thursday',
        6: 'Friday',
        7: 'Saturday'
    }

    def __init__(self):
        self._day_number = 0
        self._day_name = None
        self._category = None

    @property
    def day_name(self):
        return self._day_name

    @property
    def category(self):
        return self._category

    @property
    def day_number(self):
        return self._day_number

    @day_number.setter
    def day_number(self, value):
        if 1 <= value <= 7:
            self._day_number = value
            self._day_name = DayOnWeek.NUMBER_NAME_ASSOCIATIONS[self._day_number]
        else:
            raise ValueError(DayOnWeek.ERROR_MESSAGE)

        if 2 <= value <= 6:
            self._category = 'Business day'
        else:
            self._category = 'Weekend'


class DayOnWeekInterface:

    def __init__(self):
        self._day_on_week = DayOnWeek()

    def request_day_number(self):
        while True:
            try:
                day_num = input('Type the number of the day on the week (ex: 1 - Sunday):')
                day_num = int(day_num)
                self._day_on_week.day_number = day_num
                break
            except ValueError:
                print(DayOnWeek.ERROR_MESSAGE)
        self.show_day_on_week()

    def show_day_on_week(self):
        day_num = self._day_on_week.day_number
        day_name = self._day_on_week.day_name
        day_category = self._day_on_week.category
        print(f'The day {day_num} is {day_name.lower()}')
        print(f'{day_name} is a {day_category.lower()}')


DayOnWeekInterface().request_day_number()
\$\endgroup\$
0

3 Answers 3

10
\$\begingroup\$

Your DayOnWeek object is mutable, allowing you to set the .day_number property. For such a simple object is not a good idea. An immutable object is better.

There are exactly 7 days of the week. For a one-of-seven choice, the go-to data-storage structure is an Enum. For example:

from enum import Enum

class DayOfWeek(Enum):
    Sunday = 1
    Monday = 2
    Tuesday = 3
    Wednesday = 4
    Thursday = 5
    Friday = 6
    Saturday = 7

    @property
    def weekend(self):
        return self in {DayOfWeek.Saturday, DayOfWeek.Sunday}

    @property
    def category(self):
        return 'Weekend' if self.weekend else 'Business day'

    @property
    def number(self):
        return self.value

Borrowing Reideerien's request_day_number() function you'd use the DayOfWeek enum like:

def main():
    day_no = request_day_number()
    day = DayOfWeek(day_no)

    print(f'Day {day.number} is {day.name}')
    print(f'{day.name} is a {day.category} day')

if __name__ == '__main__':
    main()
\$\endgroup\$
2
  • \$\begingroup\$ Hi AJNeufeld. You have provided an alternate solution. However, answers must make at least one insightful observation, and must explain why the change is better than the OP's code. Failure to follow the IO rule can result in your answer being deleted. \$\endgroup\$
    – Peilonrayz
    Jun 16, 2021 at 8:23
  • \$\begingroup\$ @Peilonrayz Does not "Your DayOnWeek object is mutable, allowing you to set the .day_number property. For such a simple object [this] is not a good idea. An immutable object is better." qualify as an insightful observation? \$\endgroup\$
    – AJNeufeld
    Feb 15, 2023 at 17:13
9
\$\begingroup\$

It's great that you're thinking about classes, but you've misapplied them. This is a very simple problem that does not call for object-oriented code. The calendar module already has the name sequence, so you really only need a validation loop and a small output routine:

from calendar import day_name


def request_day_number() -> int:
    while True:
        try:
            value = int(input(
                'Type the number of the day on the week (ex: 1 - Sunday): '
            ))
        except ValueError:
            print('Invalid integer. Please try again.')
            continue

        if 1 <= value <= 7:
            return value
        print('Invalid weekday. Please try again.')


def main():
    day_no = request_day_number()
    name = day_name[day_no - 1]
    print(f'Day {day_no} is {name}')

    if 2 <= day_no <= 6:
        kind = 'business'
    else:
        kind = 'weekend'
    print(f'{name} is a {kind} day')


if __name__ == '__main__':
    main()

If you were married to the idea of an object-oriented representation, here is one relatively sane (if somewhat overbuilt) way:

from calendar import day_name


class Weekday:
    def __init__(self, index: int):
        self.index = index
        if not (0 <= index < 7):
            raise ValueError(f'{self.one_based} is an invalid week day index')

    @property
    def one_based(self) -> int:
        return self.index + 1

    def __str__(self) -> str:
        return day_name[self.index]

    @property
    def category(self) -> str:
        if self.index in {0, 6}:
            return 'weekend'
        return 'business'

    @classmethod
    def from_stdin(cls) -> 'Weekday':
        while True:
            try:
                value = int(input(
                    'Type the number of the day on the week (ex: 1 - Sunday): '
                ))
                return cls(value - 1)
            except ValueError as e:
                print(f'Invalid input: {e}. Please try again.')

    @property
    def description(self) -> str:
        return (
            f'Day {self.one_based} is {self}\n'
            f'{self} is a {self.category} day\n'
        )


def main():
    day = Weekday.from_stdin()
    print(day.description)


if __name__ == '__main__':
    main()
\$\endgroup\$
5
\$\begingroup\$

There's some issues:

  • when a new instance of your class is initialised, it has a state that's not supported by the rest of the code (i.e. a _day_number of 0, with no matching name)
  • The name DayOnWeek isn't very descriptive, why not just Weekday?
  • Why not implement the category property as a property that decides the category based on the value of _day_number instead of storing the category in a hidden variable? (which is initialised to None as well, causing further problems)
  • Similarly, why set _day_name to the name from the internal constant dictionary, instead of just returning the name directly from that in the property?
  • The error message in the DayOnWeek class is specific to functionality in the DayOnWeekInterface, it should live there, or it should be phrased correctly.
  • "Saturday is a Weekend"?
  • You've created DayOnWeekInterface to separate out the interaction with the user, that's OK, but not a very common way to go about it, what exactly is the idea here?
  • You've implemented a bunch of stuff that's just duplicating what existing libraries can do; not a great idea, unless you're coding this as a homework assignment that explicitly asks you to do so.
  • All the properties and attributes have _day in their names, while they really are all directly applying to a Day object anyway, better naming would be simpler.

However, following the general design you chose, this would be a revised version without those issues:

class Weekday:
    ERROR_INVALID_NUMBER = 'Invalid day number.'

    DAY_NAMES = {
        1: 'Sunday',
        2: 'Monday',
        3: 'Tuesday',
        4: 'Wednesday',
        5: 'Thursday',
        6: 'Friday',
        7: 'Saturday'
    }

    def __init__(self, number=1):
        self.number = number

    @property
    def name(self):
        return self.DAY_NAMES[self._number]

    @property
    def category(self):
        return 'business' if 2 <= self._number <= 6 else 'weekend'

    @property
    def number(self):
        return self._number

    @number.setter
    def number(self, value):
        if 1 <= value <= 7:
            self._number = value
        else:
            raise ValueError(self.ERROR_INVALID_NUMBER)


class WeekdayInterface:
    def __init__(self):
        self._weekday = Weekday()

    def request_value(self):
        while True:
            try:
                self._weekday = Weekday(int(input('Type the number of the day on the week (ex: 1 - Sunday):')))
                self.show()
            except ValueError as e:
                print(e)

    def show(self):
        print(f'The day {self._weekday.number} is {self._weekday.name.lower()}')
        print(f'{self._weekday.name} is a {self._weekday.category.lower()} day.')


WeekdayInterface().request_value()
\$\endgroup\$
2
  • \$\begingroup\$ I'm not in love with the "Weekday" name. I wouldn't normally call Saturday a weekday. It's a day of the week, but not a weekday. \$\endgroup\$
    – amalloy
    May 26, 2021 at 21:07
  • \$\begingroup\$ Agreed, DayOfWeek or just Day might have been better. \$\endgroup\$
    – Grismar
    May 27, 2021 at 7:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.