trjhtr

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?

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'))