5
\$\begingroup\$

I've been trying to teach myself to use object oriented code, and I thought a fun way to learn would be to write a class of planets, based on the rule system of Mongoose Traveller.

When a planet object is created, it algorithmically generates a planet. I defined a string method and a json method for returning the planet data. The function two_dice() simulates the sum of two dice rolls.

from models.common import two_dice
from random import randint

# Translate int values to string equivalent
numMap = {0: '0', 1: '1', 2: '2', 3: '3', 4: '4', 5: '5', 6: '6', 7: '7', 8: '8', 9: '9', 10: 'A', 11: 'B', 12: 'C',
          13: 'D', 14: 'E', 15: 'F', '-': '-'}


class Planet(object):
    def __init__(self, name, **kwargs):
        """
        creates a planet object with algorithmically defined attributes
        Attributes can be passed as keywords and creation process will account for them
        """
        self.name = name
        # Size is 2D - 2
        self.size = self._check_kwargs("size", 10, 0, kwargs)
        if self.size is None:
            self.size = two_dice()-2
        # Atmosphere is Size + 2D - 7. If size = 0, atmosphere = 0
        self.atmosphere = self._check_kwargs('atmosphere', 15, 0, kwargs)
        if self.atmosphere is None:
            self.atmosphere = self.size+two_dice()-7
            if self.size == 0:
                self.atmosphere = 0
            self.atmosphere = max(0, self.atmosphere)
        # Temperature is 2D. Affected by atmosphere type
        self.temperature = self._check_kwargs('temperature', 12, 0, kwargs)
        if self.temperature is None:
            self.temperature = two_dice()
            if self.atmosphere in [2, 3]:
                self.temperature -= 2
            elif self.atmosphere in [4, 5, 14]:
                self.temperature -= 2
            elif self.atmosphere in [8, 9]:
                self.temperature += 1
            elif self.atmosphere in [10, 13, 15]:
                self.temperature += 2
            elif self.atmosphere in [11, 12]:
                self.temperature += 6
            self.temperature = max(0, self.temperature)
            self.temperature = min(12, self.temperature)
        # Hydrographics is atmosphere + 2D - 7. Affected by size, temperature and atmosphere
        self.hydrographics = self._check_kwargs('hydrographics', 10, 0, kwargs)
        if self.hydrographics is None:
            if self.size <= 1:
                self.hydrographics = 0
            else:
                self.hydrographics = self.atmosphere + two_dice() - 7
                if self.atmosphere in [0, 1, 10, 11, 12]:
                    self.hydrographics -= 4
                if self.atmosphere != 13:
                    if self.temperature in [10, 11]:
                        self.hydrographics -= 2
                    elif self.temperature >= 12:
                        self.hydrographics -= 6
            self.hydrographics = max(0, self.hydrographics)
            self.hydrographics = min(10, self.hydrographics)
        # Population is 2D - 2.
        self.population = self._check_kwargs('population', 12, 0, kwargs)
        if self.population is None:
            self.population = two_dice() - 2
        # Government is population + 2D - 7.
        self.government = self._check_kwargs('government', 15, 0, kwargs)
        if self.government is None:
            self.government = self.population + two_dice() - 7
            if self.population == 0:
                self.government = 0
            self.government = max(0, self.government)
            self.government = min(15, self.government)
        # Culture is determined by rolling two dice, and concatenating the result.
        self.culture = randint(1, 6) + randint(1, 6) * 10
        if self.population == 0:
            self.culture = 0
        # Law level is government + 2D - 7
        self.law_level = self._check_kwargs('law_level', 9, 0, kwargs)
        if self.law_level is None:
            self.law_level = self.government + two_dice() - 7
            if self.population == 0:
                self.law_level = 0
            self.law_level = max(0, self.law_level)
            self.law_level = min(9, self.law_level)
        # Starport level is 2D, affected by population
        self.starport = self._check_kwargs('starport', 12, 0, kwargs)
        if self.starport is None:
            self.starport = two_dice()
            if self.population >= 10:
                self.starport += 2
            elif self.population >= 8:
                self.starport += 1
            elif self.population <= 2:
                self.starport -= 2
            elif self.population <= 4:
                self.starport -= 1
            self.starport = min(12, self.starport)
            self.starport = max(0, self.starport)
        # tech level is 1D, affected by lots of modifiers
        self.tech_level = self._check_kwargs('tech_level', 15, 0, kwargs)
        if self.tech_level is None:
            self.tech_level = randint(1, 6)
            if self.atmosphere <= 3 or self.atmosphere >= 10:
                self.tech_level += 1
            if self.size in [0, 1]:
                self.tech_level += 2
            elif self.size in [2, 3, 4]:
                self.tech_level += 1
            if self.hydrographics in [0, 9]:
                self.tech_level += 1
            elif self.hydrographics == 10:
                self.tech_level += 2
            if self.population in [1, 2, 3, 4, 5, 8]:
                self.tech_level += 1
            elif self.population == 9:
                self.tech_level += 2
            elif self.population == 10:
                self.tech_level += 4
            if self.government in [0, 5]:
                self.tech_level += 1
            elif self.government in [13, 14]:
                self.tech_level -= 2
            elif self.government == 7:
                self.tech_level += 2
            if self.starport >= 11:
                self.tech_level += 6
            elif self.starport >= 9:
                self.tech_level += 4
            elif self.starport >= 7:
                self.tech_level += 2
            elif self.starport <= 2:
                self.tech_level -= 4
            if self.population == 0:
                self.tech_level = 0
            self.tech_level = max(0, self.tech_level)
            self.tech_level = min(15, self.tech_level)

    @staticmethod
    def _check_kwargs(keyword, maxvalue, minvalue, kwargs):
        """
        Checks the given keyword exists, and is between given max & min values
        Returns keyword value if exists, otherwise None
        """
        if keyword in kwargs:
            if maxvalue >= kwargs[keyword] >= minvalue:
                return kwargs[keyword]
            else:
                raise ValueError("{} must be between {} and {}".format(keyword, maxvalue, minvalue))
        else:
            return None

    def json(self):
        return {'name': self.name,
                'size': self.size,
                'atmosphere': self.atmosphere,
                'temperature': self.temperature,
                'hydrographics': self.hydrographics,
                'population': self.population,
                'government': self.government,
                'culture': self.culture,
                'law_level': self.law_level,
                'starport': self.starport,
                'tech_level': self.tech_level}

    def __str__(self):
        attributes_list = [self.size, self.atmosphere, self.hydrographics, self.population, self.government,
                           self.law_level, '-', self.tech_level]
        if self.starport <= 2:
            starport = 'X'
        elif self.starport <= 4:
            starport = 'E'
        elif self.starport <= 6:
            starport = 'D'
        elif self.starport <= 8:
            starport = 'C'
        elif self.starport <= 10:
            starport = 'B'
        else:
            starport = 'A'
        return self.name + ' ' + starport + ''.join(list(map(lambda X: numMap[X],attributes_list)))

This is my first attempt at writing a class. Although it is currently stand alone, I would like for it to be able to fit inside a larger project. Are there any ways I can improve this so that it better follows best practices, and removes repetitive code?

\$\endgroup\$
  • \$\begingroup\$ I'm assuming you're building a space RPG planet generator of sorts? \$\endgroup\$ – Mast Jan 28 '18 at 16:53
  • 1
    \$\begingroup\$ Yes, it's a recreation of the planet generation rules in the pen and paper rpg Traveller. You can find the rules here if you're interested traveller-srd.com/core-rules/world-creation \$\endgroup\$ – Joshua Kidd Jan 28 '18 at 17:25
5
\$\begingroup\$
  1. There does not seem to be a way to pass culture as a keyword argument. This looks like an oversight.

  2. There is no check for invalid keyword arguments. This means that if we make a typographical error, we won't find out about it, but the result will be wrong:

    >>> planet = Planet('Hrod', atmopshere=10)
    >>> planet.atmosphere
    7
    

    It would be better to raise an exception like this:

    TypeError: 'atmopshere' is an invalid keyword argument for this function
    
  3. The function two_dice seems oddly specific. Traveller uses a lot of 2d6 rolls, but in the code I can see expressions like randint(1, 6) and two_dice() - 7. I think something like this would be helpful:

    import re
    
    def roll(dice):
        """Roll dice and return their sum. The argument must be a string
        describing the roll, for example '2d6+3' to roll two 6-sided dice
        and add three.
    
        """
        match = re.match(r'([1-9]\d*)d([1-9]\d*)([+-]\d+)?$', dice)
        if not match:
            raise ValueError(f"Expected dice but got {dice!r}")
        n, sides, bonus = match.groups()
        sides = int(sides)
        return sum(randint(1, sides) for _ in range(int(n))) + int(bonus or 0)
    

    Then you can write roll('1d6') or roll('2d6-7') which might be clearer to the reader and easier to check against the rulebook.

  4. The temperature is adjusted by the same value (−2) for two different sets of values for the atmosphere:

    if self.atmosphere in [2, 3]:
        self.temperature -= 2
    elif self.atmosphere in [4, 5, 14]:
        self.temperature -= 2
    

    Is that last line a typo for self.temperature -= 1?

  5. numMap maps integers from 0 to 15 to their hexadecimal equivalant, and also hyphen to itself. If it weren't for the hyphen you could write format(n, 'X') and not need the numMap. Looking at the use of numMap it seems that the point of the hyphen is to implement a special case in the __str__ method. But as it says in the Zen of Python,

    Special cases aren't special enough to break the rules.

    So I would recommend making __str__ a bit more complicated in order to elimate the special case, which would allow you to use format(..., 'X'), which in turn would allow you to get rid of numMap.

  6. There's a lot of repetition. For each attribute, we have a name, we have a maximum value, and we might need to include its hexadecimal digit in the string code. We can put this data into a table, like this:

    _ATTRIBUTES = [
        # Name,      maximum, code
        ('size',          10, True),
        ('atmosphere',    15, True),
        ('temperature',   12, False),
        ('hydrographics', 10, True),
        ('population',    12, True),
        ('government',    15, True),
        ('law_level',      9, True),
        ('starport',      12, False),
        ('tech_level',    15, False),
        ('culture',       66, False),
    ]
    

    And we could put the rules for generating each attribute into their own methods, like this:

    def _size_default(self):
        return roll('2d6-2')
    
    def _atmosphere_default(self):
        if self.size == 0:
            return 0
        else:
            return self.size + roll('2d6-7')
    
    _TEMPERATURE_ADJUST = [0, 0, -2, -2, -1, -1, 0, 0, 1, 1, 1, 6, 6, 1, -1, 1]
    
    def _temperature_default(self):
        return roll('2d6') + self._TEMPERATURE_ADJUST[self.atmosphere]
    
    def _hydrographics_default(self):
        if self.size <= 1:
            return 0
        hydrographics = self.atmosphere + roll('2d6-7')
        if self.atmosphere in (0, 1, 10, 11, 12):
            hydrographics -= 4
        if self.atmosphere != 13:
            if self.temperature in (10, 11):
                hydrographics -= 2
            elif self.temperature >= 12:
                hydrographics -= 6
        return hydrographics
    
    def _population_default(self):
        return roll('2d6-2')
    
    def _government_default(self):
        if self.population == 0:
            return 0
        else:
            return self.population + roll('2d6-7')
    
    def _culture_default(self):
        if self.population == 0:
            return 0
        else:
            return roll('1d6') * 10 + roll('1d6')
    
    def _law_level_default(self):
        if self.population == 0:
            return 0
        else:
            return self.government + roll('2d6-7')
    
    _STARPORT_ADJUST = [-2, -2, -2, -1, -1, 0, 0, 0, 1, 1, 2, 2, 2, 2, 2, 2]
    
    def _starport_default(self):
        return roll('2d6') + self._STARPORT_ADJUST[self.population]
    
    _TECH_LEVEL_ADJUST = [
        # attribute, {value:adjustment}
        ('size', {0:2, 1:2, 2:1, 3:1, 4:1}),
        ('atmosphere', {0:1, 1:1, 2:1, 3:1, 10:1, 11:1, 12:1, 13:1,
                        14:1, 15:1}),
        ('hydrographics', {0:1, 9:1, 10:2}),
        ('population', {1:1, 2:1, 3:1, 4:1, 5:1, 8:1, 9:2, 10:4}),
        ('government', {0:1, 5:1, 7:2, 13:-2, 14:-2}),
        ('starport', {0:-4, 1:-4, 2:-4, 7:2, 8:2, 9:4, 10:4, 11:6, 12:6,
                      13:6, 14:6, 15:6}),
    ]
    
    def _tech_level_default(self):
        if self.population == 0:
            return 0
        tech_level = roll('1d6')
        for attribute, adjustment in self._TECH_LEVEL_ADJUST:
            tech_level += adjustment.get(getattr(self, attribute), 0)
        return tech_level
    

    Now we can process the keyword arguments systematically, like this, fixing points §1 and §2 above, and avoiding the repetition:

    for attribute, maximum, _ in self._ATTRIBUTES:
        if attribute in kwargs:
            value = kwargs.pop(attribute)
            if not (0 <= value <= maximum):
                raise ValueError(f"{attribute!r} must be between 0 and "
                                 "{maximum} inclusive")
        else:
            value = getattr(self, '_' + attribute + '_default')()
            value = max(0, min(value, maximum))
        setattr(self, attribute, value)
    if kwargs:
        kwarg = next(iter(kwargs))
        raise TypeError(f"{kwarg!r} is an invalid keyword argument for "
                        "this function")
    

    After doing this, we don't need _check_kwargs any more, and the json and __str__ methods become:

    def json(self):
        result = dict(name=self.name)
        for attribute, _, _ in self._ATTRIBUTES:
            result[attribute] = getattr(self, attribute)
        return result
    
    _STARPORT_CODE = 'XXXEEDDCCBBAAAAA'
    
    def __str__(self):
        return "{} {}{}-{}".format(
            self.name,
            self._STARPORT_CODE[self.starport],
            ''.join(format(getattr(self, attribute), 'X')
                    for attribute, _, code in self._ATTRIBUTES if code),
            format(self.tech_level, 'X'))
    
\$\endgroup\$
  • \$\begingroup\$ Is there a particular reason why you use [1-9]\d* in the regex in roll(dice), and not \d+? \$\endgroup\$ – mkrieger1 Jan 29 '18 at 12:14
  • \$\begingroup\$ Also (maybe related), are these typos in _size_default (roll(2d-2)) and _atmosphere_default (roll(2d-7))? They don't seem to match the dice roll syntax. \$\endgroup\$ – mkrieger1 Jan 29 '18 at 12:22
  • \$\begingroup\$ @mkrieger1: (i) Errors now fixed, thank you. (ii) Requiring a non-zero digit catches mistakes like 0d6 or 2d0, and generates a better error message than ValueError: empty range for randrange() (1,1, 0). \$\endgroup\$ – Gareth Rees Jan 29 '18 at 12:31
  • \$\begingroup\$ Oh, I didn't realize that it was [1-9], not [0-9]. That explains a lot. \$\endgroup\$ – mkrieger1 Jan 29 '18 at 12:40

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.