5
\$\begingroup\$

I have been programming with Python on-and-off for about 2 years now, even now I still find that when I look at other applications I struggle to follow along with their classes/objects. Please help me improve upon my own OOP experience.

Here is one of my more recent projects for a Django Website.

pokemon.py

# pylint: disable=too-many-instance-attributes,too-many-locals,too-many-branches,too-many-statements
""" Represent Pokemon objects """
from pokedex.models import Pokemon as pokemon, PokemonTypes, PokemonStats, PokemonAbilities, \
    PokemonSpecies, PokemonGenderRatios, PokemonProse, PokemonMoves, Types

class PokemonList:
    """
      A Class to represent all Pokemon
    """
    def __init__(self, page):
        """
        Gets 100 pokemon at a time based off of the GET variable "page"
        :param page: Defaults to 1. Is used for pagination
        """
        self.poke_list = []

        if page == 1:
            all_rows = [list(x) for x in list(
                pokemon.objects.filter(id__range=(0, 100)).values_list(
                    'id', 'identifier', 'height', 'weight'))]
        else:
            OFFSET = int(page) * 100
            all_rows = [list(x) for x in list(
                pokemon.objects.filter(id__range=(OFFSET - 99, OFFSET)).values_list(
                    'id', 'identifier', 'height', 'weight'))]


        for row in all_rows:
            type_list = list(Types.objects.filter(
                pokemontypes_type__pokemon__id=row[0]).values_list('identifier', flat=True))
            row.append(type_list)
            self.poke_list.append(row)

class Pokemon:
    """
        A Class to represent a pokemon.
    """

    def __init__(self, poke_id):
        """
        Instantiate a pokemon object with base information
        :param poke_id: id of the pokemon to get from the database
        """
        # Select base information about a pokemon
        poke = pokemon.objects.get(id=poke_id)
        self.id = poke.id
        self.static_path = 'pokedex/img/profiles/' + str(self.id) + '.png'
        self.long_id = str(self.id).zfill(3)
        self.identifier = poke.identifier
        self.species_id = poke.species.identifier
        self.height = poke.height / 10.0
        self.weight = poke.weight / 10.0
        self.base_experience = poke.base_experience
        self.evolution_chain = self._get_evolution_chain()
        self.type = []
        self.available_types = ['normal', 'fire', 'water', 'electric', 'grass', 'ice', 'fighting', 
                                'poison', 'ground', 'flying', 'psychic', 'bug', 'rock', 'ghost', 
                                'dragon', 'dark', 'steel', 'fairy']
        self.type_static = []

        # Get Type (may be multiple)
        types = PokemonTypes.objects.filter(pokemon=self.id).select_related('type').values_list(
            'type__identifier', flat=True)

        for t in types:
            self.type.append(t)
            self.type_static.append('pokedex/img/types' + t + '-type.png')

        # Get base stats
        stat = [list(x) for x in list(
            PokemonStats.objects.filter(pokemon=self.id).select_related('stat').values_list(
                'stat__identifier', 'base_stat'))]

        self.stats = []
        for s in stat:
            tempdict = {
                s[0]: s[1]
            }
            self.stats.append(tempdict)

        # Get Abilities
        ability = PokemonAbilities.objects.filter(
            pokemon=self.id).select_related('ability').values_list(
                'ability__identifier', flat=True)
        self.abilities = []

        for item in ability:
            self.abilities.append(item)        

        # Get Moves
        self.moves = _get_moves()

        # Get Gender Rates
        gender_obj = PokemonGenderRatios.objects.select_related('ratio').get(pokemon=self.id)
        if gender_obj is not None:
            self.percent_male, self.percent_female = gender_obj.ratio.percent_male, \
                gender_obj.ratio.percent_female

        # Get Description if applicable
        try:
            desc_obj = PokemonProse.objects.get(pokemon=self.id)
            self.description = desc_obj.description
        except PokemonProse.DoesNotExist:
            pass

    def prev_pokedex(self):
        """
        Returns a dictionary of information about the previous pokemon in pokedex 
        (path to image, id, and identifier)
        """

        prev_id = self.id - 1
        if prev_id > 721 or prev_id < 1:
            return None
        path_to_image = '/pokedex/img/profiles/' + str(prev_id) + '-small.png'
        identifier = pokemon.objects.values_list('identifier', flat=True).get(id=prev_id)

        prev_pokemon = {
            'image': path_to_image,
            'id': prev_id,
            'identifier': identifier
        }

        return prev_pokemon

    def next_pokedex(self):
        """ 
        Returns a dictionary of information about the next pokemon in pokedex 
        (path to image, id, and identifier) 
        """

        next_id = self.id + 1
        if next_id > 721 or next_id < 1:
            return None
        path_to_image = '/pokedex/img/profiles/' + str(next_id) + '-small.png'
        identifier = pokemon.objects.values_list('identifier', flat=True).get(id=next_id)

        next_pokemon = {
            'image': path_to_image,
            'id': next_id,
            'identifier': identifier
        }

        return next_pokemon

    def _get_evolution_chain(self):
        """ Queries for Pokemon Evolution Chain """
        # Gather Evolution Chain
        evolution_chain = {
            'id': self.id,
            'identifier': self.identifier
        }

        # Get previous evolution
        try:
            evolves_from = PokemonSpecies.objects.get(id=self.id)
        except PokemonSpecies.DoesNotExist:
            evolves_from = None
        if evolves_from is not None and evolves_from.evolves_from_species_id is not None:
            evolution = {'id': evolves_from.evolves_from_species_id}
            evolution['identifier'] = pokemon.objects.get(id=evolution['id']).identifier
            evolution_chain.insert(0, evolution)

            try:
                prev_evolv = PokemonSpecies.objects.get(id=evolution['id'])
            except PokemonSpecies.DoesNotExist:
                prev_evolv = None
            if prev_evolv is not None and prev_evolv.evolves_from_species_id is not None:
                evolution = {'id': prev_evolv.evolves_from_species_id}
                evolution['identifier'] = pokemon.objects.get(id=evolution['id']).identifier
                evolution_chain.insert(0, evolution)

        # Get Next Evolution
        try:
            next_evol = PokemonSpecies.objects.get(evolves_from_species_id=self.id)
        except PokemonSpecies.DoesNotExist:
            next_evol = None
        if next_evol is not None and next_evol.id is not None:
            evolution = {'id': next_evol.id}
            evolution['identifier'] = pokemon.objects.get(id=evolution['id']).identifier
            evolution_chain.append(evolution)

            try:
                next_evol = PokemonSpecies.objects.get(evolves_from_species_id=evolution['id'])
            except PokemonSpecies.DoesNotExist:
                next_evol = None
            if next_evol is not None and next_evol.id is not None:
                evolution = {'id': next_evol.id}
                evolution['identifier'] = pokemon.objects.get(id=evolution['id']).identifier
                evolution_chain.append(evolution)
        return evolution_chain

    def _get_moves(self):
        """ Get available moves """
        moves = [list(x) for x in list(
            PokemonMoves.objects.filter(pokemon=self.id).select_related(
                'move__type', 'move').values_list(
                    'move__identifier', 
                    'move__type__identifier', 
                    'move__power', 
                    'move__pp', 
                    'move__accuracy'))]
        moves_list = []
        move_ids = []
        for move in moves:
            if move[0] not in move_ids:
                temp_list = [move[0], move[1], move[2], move[3], move[4]]
                moves_list.append(temp_list)
                move_ids.append(move[0])
        return moves_list

    def __str__(self):
        """ Returns Identifier when str() is called"""
        return self.identifier
\$\endgroup\$
4
  • 3
    \$\begingroup\$ Hi, what is the pokedex module you are using? Is it something you wrote or some well known Pokemon library? \$\endgroup\$
    – Dair
    Commented Sep 6, 2018 at 6:46
  • \$\begingroup\$ I'm sorry, can you elaborate what you mean? This is a Django project that has ORM models that represent different pokemon "entities", most of the data is from Veekun's Pokedex SQLite3 db. I have a lot of custom tables + imported data, as well as I have converted it to Postgresql. \$\endgroup\$
    – CHBresser
    Commented Sep 6, 2018 at 13:06
  • \$\begingroup\$ I would highly recommend creating a question around doing a code review of your models that you have specified in your models.py. I'm in the process of guessing how they are defined, and I (along with others) may have suggestions. \$\endgroup\$ Commented Sep 23, 2018 at 20:39
  • \$\begingroup\$ I can if it would provide quality feedback, but they are just imported from the db using django's inspectdb management method, I have a few extra tables and I had to set related names but that is about it. \$\endgroup\$
    – CHBresser
    Commented Sep 24, 2018 at 13:12

1 Answer 1

2
\$\begingroup\$

Long Import Lines

You can break these up using parentheses:

from pokedex.models import (
    Pokemon as pokemon, 
    PokemonTypes, 
    PokemonStats, 
    PokemonAbilities, 
    PokemonSpecies, 
    PokemonGenderRatios, 
    PokemonProse, 
    PokemonMoves, 
    Types
)

Evolution Chain

First, there is a bug. You use evolution_chain.insert, but evolution_chain is a dict, so this will raise an AttributeError. If you really need to insert at the front of a list, it's recommended to use a collections.deque with appendleft:

from collections import deque

class Pokemon:
     def evolution_chain(self):
         evolution_chain = deque([{'id': self.id, 'identifier': self.identifier, ''}])

         if evolves_from is not None...:
              evolution_chain.appendleft(...)

This is an expensive set of calls to try to get the chain on instantiation. I'd make it a functools.cached_property (python 3.8+) or use a getattr check on a property:

from functools import cached_property

class Pokemon:
      @cached_property
      def evolution_chain(self):
        evolution_chain = deque([{'id': self.id, 'identifier': self.identifier, ''}])

This also appears to be somewhat recursive to get both previous and next evolutions. Let's set this up so that it traverses up and down the tree

class Pokemon:
    def _evolutions(self, id, into=False):
        chain = []

        # enables us to go forward and backward
        if into:
            attr = 'evolves_from_species_id'
            evol_attr = 'id'
        else:
            attr = 'id'
            evol_attr = 'evolves_from_species_id'

        try:
            evolution = PokemonSpecies.objects.get(**{attr: id})
        except PokemonSpecies.DoesNotExist:
            return chain

        next_in_chain = getattr(evolution, evol_attr, None)

        if evolution is None or next_in_chain is None:
            return chain

        poke = pokemon.objects.get(id=evolution['id'])

        chain.append({
            'id': next_in_chain,
            'identifier': poke.identifier
        })

        chain.extend(self._evolutions(next_in_chain, into=into))

        return chain


    @cached_property
    def evolution_chain(self):
        evolution_chain = deque([
            {'id': self.id, 'identifier': self.identifier}
        ])

        evolution_chain.extendleft(
            self._evolutions(id=self.id, into=False)
        )

        evolution_chain.extend(
            self._evolutions(id=self.id, into=True)
        )

        return evolution_chain

get_moves

This should also be a cached_property. It also would be good to use tuple unpacking for readability on the attributes in the moves list. A collections.namedtuple is very useful for this:

from collections import namedtuple

# this gives you __str__ by default and also
# allows for dotted attribute access
Move = namedtuple('Move', ['identifier', 'type', 'power', 'pp', 'accuracy'])


class Pokemon:
    @cached_property
    def moves(self):
        attrs = [
            'move__identifier',
            'move__type__identifier',
            'move__power',
            'move__pp',
            'move__accuracy'
        ]

        # You can unpack these into a generator expression
        # so you iterate once over the list
        moves = (
            Move(identifier, type_, power, pp, accuracy)
            for (identifier, type_, power, pp, accuracy, *_)
            in (
                PokemonMoves
                .objects
                .filter(pokemon=self.id)
                .select_related('move__type', 'move')
                .values_list(*attrs)
            )
        )

        moves_dict = {}

        for move in moves:
            if move.identifier in moves_dict:
                continue

            moves_dict[move.identifier] = move

        return moves_dict

next_pokedex and prev_pokedex

In my opinion, this should be an iterator protocol on a Paginator of sorts, or maybe a PokeDex. There are quite a few things going on in this refactor, so I'll show the completed code and break down each section:

from collections import namedtuple

PokedexEntry = namedtuple('PokedexEntry', 'id identifier height weight type'.split())

class PokeDex:
    def __init__(self, start_page=1, pagesize=100):
        self.pokemon = {}
        # idx can't start at 0, so we either start at the beginning
        # of a page, or we start at 1 if it's the first page
        self.start_idx = ((start_page - 1) * pagesize) or 1
        self.pagesize = pagesize

    def __contains__(self, pokemon_id):
        return pokemon_id in range(1, 721)

    def __getitem__(self, pokemon_id):
        if pokemon_id not in range(1, 721):
            raise KeyError("Pokemon id not in range 1..721")
        elif pokemon_id in self.pokemon:
            return self.pokemon[pokemon_id]

        pokemon = _get_from_offset(pokemon_id)
        return pokemon

    def __iter__(self):
        # just in case we start at page > 1
        r = range(self.start_idx, 721)
        self.start_idx = 1
        return iter(r)

    def get(self, k, default=None):
        """Mimics the dict.get call"""
        try:
            return self[k]
        except KeyError:
            return default

    def entries(self):
        """Similar to dict.items()"""
        for k in self:
            yield k, self[k]

    def _get_from_offset(self, id):
        """Does the heavy lifting for pagination"""
        start, end = self._calculate_offset(self, id)

        selected = (
            pokemon
            .objects
            .filter(id__range(start, end))
            .values_list(['id', 'identifier', 'height', 'weight'])
        )

        for poke_id, *poke in selected:
            type_list = self.type_from_pokemon(poke_id)
            entry = [*poke, *type_list]
            self.pokemon[poke_id] = PokedexEntry(*entry)

        return self.pokemon[id]


    def _type_from_pokemon(self, poke_id):
        type_list = (
            Types
            .objects
            .filter(pokemontypes_type__pokemon__id=poke_id)
            .values_list('identifier', flat=True)
        )
        return type_list

    def _calculate_offset(self, id):
        start = id // self.pagesize
        stop = a + self.pagesize

        return start, stop

    def page(self, page_num):
        offset = (page_num - 1) * self.pagesize
        pokemon = []

        # Need the offset or 1 in case we start at 
        # 0, that way we don't get a KeyError
        for id_ in range(offset or 1, offset + self.pagesize):
            # __getitem__ invoked here to crawl the page if an
            # id_ isn't found
            pokemon.append(self[id_])

        return pokemon

    def pages(self, start_page=1):
        """Iterator over all pages"""
        total_pages = (721 // self.pagesize) + 1
        for i in range(start_page, total_pages):
            yield self.page(i)

__getitem__

This allows the PokeDex to be indexed like a dictionary. The __contains__ function allows the user to see if a pokemon_id is contained within the pokedex id range using the if poke_id in self syntax. This means we can more easily look up if a Pokemon is in the PokeDex. If a pokemon isn't immediately found, then get_from_offset is employed to further fill out the pokedex.

The interesting upshot here is that we get iteration for free because of __getitem__. However, the iteration provided by __getitem__ starts at i=0, but we need the ids to start at 1. To get around this, we use __iter__ to return an iterator over range(1, 721).

get and entries

Both get and entries mimic the dictionary methods get and items to provide a familiar interface. entries will yield both the key and value contained in the pokemon dictionary.

get_from_offset

This is the paginator function that does the heavy lifting. It will page through all of the pokemon and set their entries in the pokedex. When the pokemon is not found in the pokedex, it retrieves the next page of pokemon. The caching in the PokeDex means that you don't have to do repeated lookups against the pokemon model.

calculate_offset

This uses the pagesize to arithmetically derive the start and stop points of the next pagination step.

page

This enables you to get each page of pokemon, as described in your initial API. So you can do:

dex = PokeDex(start_page=1, pagesize=100)

page_5 = dex.page(5)
# do something with page_5

pages

Iterates over the pages starting at a given start page.

PokeDex in action

This means you can iterate over the pokedex accordingly:

poke_dex = PokeDex()

for poke_id, poke in poke_dex.entries():
    # do something

Or for just the keys:

for poke_id in poke_dex:
    print(poke_id)

You can try to find a specific pokemon:

poke_dex[1]
# bulbasaur

poke_dex[800]
KeyError: Pokemon id not in range 1..721

And you can use a default return value to squash a KeyError via get:

poke_dex.get(800, 'not there')
'not there'

You can also just check to see if a pokemon is present:

5 in poke_dex
True
\$\endgroup\$

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.