Design: method in class or general function?

[Peter Otten]

Leam Hall wrote:

> On 09/08/2017 03:06 AM, Peter Otten wrote:
>> leam hall wrote:
>> 
>>> On Thu, Sep 7, 2017 at 8:16 AM, Steve D'Aprano
>>> <steve+python at pearwood.info> wrote:
>>>
>>>> On Thu, 7 Sep 2017 07:20 pm, Leam Hall wrote:
>>>>
>>>>> OOP newbie on Python 2.6.
>>>>
>>>> Python 2.6 is ancient, and is missing many nice features. You should
>>>> consider
>>>> using the latest version, 3.6.
>>>>
>>>
>>> I've wrestled with that discussion for a while and Python 3 loses every
>>> time. There's literally no good reason for me to move to Python 3
>>> earlier than mid-2020's. Please accept the fact that there are hundreds
>>> of thousands of servers, if not millions, running Python 2.x. Whether or
>>> not Python 3 has any neat cool stuff is irrelevant to those of us
>>> seeking to use Python to get today's work done.
>>>
>>>> I create instances of Character class with an attribute dict of
>>>>> 'skills'. The 'skills' dict has the name of a skill as the key and an
>>>>> int as a value. The code adds or modifies skills before outputting the
>>>>> Character.
>>>>>
>>>>> Is it better design to have a Character.method that takes a 'skill'
>>>>> key and optional value or to have a general function that takes an
>>>>> instance, a dict, a key, and an optional value?
>>>>
>>>> I'm afraid your example is too generic for me to give an opinion. Do
>>>> you literally mean a method called "method"? What does it do?
>>>>
>>>
>>>
>>> Using this:
>>> https://github.com/makhidkarun/py_tools/blob/master/lib/character.py
>>>
>>> Line 19 sets "self.skills" either from the passed in data or from
>>> 
https://github.com/makhidkarun/py_tools/blob/master/lib/character_tools.py
>>> #L34-L48
>>>
>>> So Character.skills is a dict with a string key and an int value. I need
>>> to be able to add skills and my first attempt is a function:
>>> 
https://github.com/makhidkarun/py_tools/blob/master/lib/character_tools.py
>>> #L52-L56
>>>
>>> Should the "add_skills" function be a method in the character class or
>>> be made a more generic function to add/modify a key/value pair in a dict
>>> that is an attribute of an instance? Other tasks will require the
>>> add/modify functionality but coding that increases complexity. At least
>>> for me, anyway.
>>>
>>> Sorry about being unclear earlier, coffee was still kicking in and I'm
>>> still a newbie that mixes up terms.
>> 
>> I'm pleading "method" as it allows per-class implementation.
>> 
>> Say you use per-career subclasses of a general Character class. There are
>> default per-career skill sets, but usually a Character can acquire a
>> skill that is not needed in his career -- with the exception that Rogues
>> cannot tap dance ;)
>> 
>> Below is a way to implement that with a specialised add_skill() method:
>> 
>> $ cat basic_oo.py
>> from __future__ import print_function
>> import random
>> from collections import defaultdict
>> 
>> 
>> class Character(object):
>>      DEFAULT_SKILLS = ['Blade', 'GunCbt', 'Admin', 'Streetwise']
>> 
>>      def __init__(self):
>>          self.skills = defaultdict(int)
>> 
>>      def add_random_skills(self, terms):
>>          skillnames = self.DEFAULT_SKILLS
>>          for _ in range(2*terms):
>>              self.add_skill(random.choice(skillnames))
>> 
>>      def add_skill(self, name, amount=1):
>>          self.skills[name] += amount
>> 
>>      def __str__(self):
>>          skills = ", ".join(
>>              "{}={}".format(name, amount)
>>              for name, amount in sorted(self.skills.items())
>>              if amount != 0
>>          )
>>          return "{}({})".format(self.__class__.__name__, skills)
>> 
>> 
>> class Rogue(Character):
>>      def add_skill(self, name, amount=1):
>>          if name == "TapDance":
>>              raise ValueError("Sorry, this rogue will never tap dance")
>>          super(Rogue, self).add_skill(name, amount)
>> 
>> 
>> class Marine(Character):
>>      DEFAULT_SKILLS = ['GunCbt', 'VaccSuit', 'Leadership', 'Vehicle']
>> 
>> 
>> def main():
>>      NUM_CHARACTERS = 5
>>      CHARACTERS = [Marine, Rogue]
>> 
>>      characters = [
>>          random.choice(CHARACTERS)() for _ in range(NUM_CHARACTERS)
>>      ]
>> 
>>      for c in characters:
>>          c.add_random_skills(5)
>>          c.add_skill("RepairBicycles", random.randrange(3))
>>          try:
>>              c.add_skill("TapDance", 3)
>>          except ValueError as err:
>>              print(err)
>> 
>>      for c in characters:
>>          print(c)
>> 
>> 
>> if __name__ == "__main__":
>>      main()
>> 
>> $ python basic_oo.py
>> Sorry, this rogue will never tap dance
>> Sorry, this rogue will never tap dance
>> Sorry, this rogue will never tap dance
>> Rogue(Admin=3, Blade=4, GunCbt=2, Streetwise=1)
>> Marine(GunCbt=5, Leadership=4, TapDance=3, VaccSuit=1)
>> Rogue(Blade=3, GunCbt=2, RepairBicycles=2, Streetwise=5)
>> Rogue(Admin=1, Blade=2, GunCbt=5, RepairBicycles=1, Streetwise=2)
>> Marine(GunCbt=1, Leadership=3, RepairBicycles=2, TapDance=3, VaccSuit=2,
>> Vehicle=4)
> 
> 
> Okay Peter, I took your idea and mangled it beyond recognition. There's
> a design constraint I hadn't mentioned: an instance of Character should
> be able to have multiple careers.
> 
> Also, an instance can be created from scratch, created from a full set
> of data, or created from a partial set of data.
> 
> Here's my current code, focusing on creating a lot of merchants:
> 
https://github.com/makhidkarun/py_tools/blob/merchant/lib/character.py#L60-L61
> 
> python chargen.py
> Toby Verstraeten     564775     [M] Age: 41
>    Merchant (5 terms)
>    Computer-2 Engineering-5 Navigation-2 Pilot-1
> 
> Captain Ian Domici           789987     [M] Age: 24
>    Firster (3 terms) Merchant (3 terms) Noble (2 terms)
>    Broker-2 GunCbt-1 Streetwise-2
> 
> Rosa                 66495B     [F] Age: 24
>    Merchant (1 terms)
>    Computer-1 Navigation-1
> 
> 
> As you can see, lots to work on. However, with a very loose definition
> of "work", this works.
> 
> The goal is to have a set of Career classes. The program will allow a
> user to select one or more Careers. The program will create a basic
> character and then modify the character by the selected Career class(es).
> 
> If the Career does not exist in a file then the character gets assigned
> a generic Career based on social status.
> 
> Careers are each in their own file and the program builds the list of
> available careers by slurping up those file names.
>    Adding a Career should just be adding a properly formatted file.
>    Import happens when a Career is selected to modify the character
> 
> I've done this in Ruby so my guess is it can be done in Python. Even
> Python 2.  :D
> 
> The Career seems to be a "Decorator" pattern given my limited
> understanding of design patterns. Concur? If so I'll go study that some
> more.

The first thought that triggered was @decorator, and only then, because that 
didn't make sense, the focus changed on "design patterns". While I would 
tend to "composite" rather than "decorator" it's been some time since I read 
the book...
 
> Do you feel this path should still make a Career a class?

As long as it's only a dictionary entry

    self.character.careers['Merchant'] = self.terms

and the addition of some skills a function would work fine, too.
If you expect that you will later add other features which vary over careers 
you should consider a Career base class that you can subclass as needed. If 
you use a class it's worthwhile to keep the instance around

    self.character.careers['Merchant'] = self  # a careers set 
                                               # would work, too.

(If you want to avoid the reference cycle do not store the character as an 
attribute.)