7
\$\begingroup\$

Object-oriented program for a calorie counter sporting polymorphic methods for both genders.

Principles to adhere to:

  • Uniform Access Principle
  • Polymorphism
  • Dependency inversion principle through the Factory Method pattern
  • Single responsibility principle

Concerns for future extension:

  • BPM curve for exercise acts of longer duration, can this be transparently added?

Other concerns:

  • Unit prefixes for names, e.g. weight_kg, exercise_duration_min. Are these bad?
  • Overengineered? Yes, I'm not going to argue against that; we don't have more than one method in our classes which eliminates the need to have them at all. Assuming we had more than one method, however, is it still overdone?
  • Are any of the principles I claimed to follow not followed or properly followed? Especially thinking of the Factory Method pattern.

By the way, the formula for calculating calories burned doesn't seem to be right, but that doesn't really matter for our purposes.

# oo-calorie_counter.py
from abc import ABC, abstractmethod


def main():
    """Setup and initialization.
    """
    person_attributes = get_person_attributes()
    person = PersonFactory.make_person(person_attributes)

    print("You've burnt approximately {0} calories".format(round(person.calories_burned, 1)))


def get_person_attributes():
    """Prompts a user for their details
    """
    person_attributes = {
        'gender': input('Your gender: '),
        'weight_kg': int(input('Your weight in Kg: ')),
        'age': int(input('Your age: ')),
        'bpm': int(input('Your heart rate: ')),
        'exercise_duration_min': int(input('Your your exercise duration in minutes: '))
    }

    return person_attributes

class PersonFactory:
    @staticmethod
    def make_person(person_attributes):
        """
        Instantiate the proper a person of proper gender based on the provided attributes.
        Args:
            person_attributes: A dictionary containing the keys:
            ('gender', 'age, 'weight_kg', exercise_duration_min, bpm)

        """
        gender = person_attributes['gender']
        del person_attributes['gender']

        if gender.lower() == 'male':
            return Male(**person_attributes)
        elif gender.lower() == 'female':
            return Female(**person_attributes)
        else:
            raise RuntimeError('Unknown gender')

class Person(ABC):
    def __init__(self, weight_kg, bpm, age, exercise_duration_min):
        self.weight_kg = weight_kg
        self.age = age
        self.bpm = bpm
        self.exercise_duration_min = exercise_duration_min

    @property
    @abstractmethod
    def calories_burned(self):
        raise NotImplementedError


class Male(Person):
    @property
    def calories_burned(self):
        return ((self.age * 0.2017) - (self.weight_kg * 0.09036) + (self.bpm * 0.6309) - 55.0969) * self.exercise_duration_min / 4.184


class Female(Person):
    @property
    def calories_burned(self):
        return ((self.age * 0.074) - (self.weight_kg * 0.05741) + (self.bpm * 0.4472) - 20.4022) * self.exercise_duration_min / 4.184


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

I like your code. It is easy to understand and pretty straight-forward.

Regarding your question of the measuring units: yes, every variable should have them since you are not operating exclusively on SI units. You have calories (instead of joule) and years and minutes (instead of seconds).

I would just change some of the smaller details, from top to bottom:

  • The doc comment for main is wrong. That function not only initializes the program, it is the whole program, including computation and output.

  • Rename get_person_attributes to read_person_attributes or input_person_attributes. Functions called get_* usually don't have side-effects and are usually not complicated.

  • The gender prompt could be more specific: 'Your gender (male, female): '

  • The official spelling for kilogram is kg, not Kg. Although kilo is a large multiplier, it is the only one with a lowercase letter.

  • The prompt for the heart rate could include in bpm.

  • The Your your is a typo.

  • In make_person, the proper a person of proper gender sounds wrong (though I'm not a native English speaker).

  • You compute gender.lower() twice, which is unnecessary.

  • Do you prefer burned or burnt? Choose one and use it everywhere.

Having each of the complicated formulas in a single line is very good. One might be attempted to split it into several small formulae or extract the factors to a separate class CaloriesCalculatorFactors, but that would make the code harder to understand.

\$\endgroup\$
  • \$\begingroup\$ Your suggestions all make sense, it was sloppy to not proofread anything. Still, the issue of whether there are any fundamental design issues is still open. Although, as you say, "it's straight-forward" that covers it, right? \$\endgroup\$ – Martin Kleiven Nov 19 '17 at 17:55
  • \$\begingroup\$ I intentionally didn't comment on the use of design patterns since I don't like most of them. Your use of the Builder pattern is good though, since you just say "I want a person with these attributes". To be precise, you should rather ask for a CaloriesCalculator instead of a Person, but that's close to nitpicking. \$\endgroup\$ – Roland Illig Nov 19 '17 at 20:39
  • \$\begingroup\$ Okay. I just have one burning question left. get_person_attributes have a side-effect where it deletes an element of the dictionary passed as an argument. To what extent is this a bad idea? If at all. \$\endgroup\$ – Martin Kleiven Nov 19 '17 at 21:39
  • 1
    \$\begingroup\$ For most of your program executions, it will not matter because the caller typically doesn't reuse the person_attributes. On the one hand that's good because your program works as intended. But some day it will break mysteriously when you call the PersonFactory twice with the same argument. The exception will be pretty clear in that case, so it wouldn't be much effort to fix the problem then. But what if the code evolves over time and more and more design patterns are involved, so that the exception is not thrown near its actual cause? Then it can become complicated. \$\endgroup\$ – Roland Illig Nov 19 '17 at 21:45
  • 1
    \$\begingroup\$ h (hecto) and da (deca) are also unit prefixes > 1 with lower case letters. Just saying. \$\endgroup\$ – Richard Neumann Nov 20 '17 at 9:24

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.