10
\$\begingroup\$

I have a very large class which serves as the BaseClass for many AdapterClasses. The base class consists of methods that can be categorized as follows:

  • Translation: fundamental translation classmethods used by all AdapterClasses. Requires class attributes from AdapterClasses (used internal / external) The tricky part for this one is I would like to maintain class level access from AdapterClasses, but that eluded me. Now I have instance level access from AdapterClasses and class level access from Translator directly.
  • Operations: fundamental methods used by all AdapterClasses (internal)
  • Adapter methods: methods that can be overwritten by AdapterClasses for specialized needs. (internal)
  • API methods: Only used externally, but need access to all internal methods (external)

I was thinking separation as follows would make development a lot easier and make the public api a bit clearer. Before I take on this refactoring I'd like some feed back on Python best practices, conventions, usage, organization / flow clarity, pattern useage, etc... Thoughts, comments, suggestions welcome.

class Translator(object):
    ac = None
    dictionaries = {'english_spanish':{'hello world':'hola mundo', "i'm just backwards":'Solo soy al reves'}}
    Ac = None #place holder for class level access
    def __init__(self, c):
        self.ac = c

    # tying to find a way to get class level access from AdapterMethods for this.
    def to_adapter(self, phrase):
        r = self.by_lang(phrase, self.ac.lang)
        return r

    ''' class level version
    @classmethod
    def to_adapter(cls, phrase):
        r = self.by_lang(phrase, cls.Ac.lang)
        return r
    '''
    @classmethod
    def by_lang(cls, phrase, lang=None):
        d = cls.get_dictionary(lang)
        if isinstance(d, dict):
            for k, v in d.items(): 
                if phrase in k:
                    return v
            return phrase
        else:
            return d

    @classmethod
    def get_dictionary(cls, lang='english'):
        for k, v in cls.dictionaries.items():
            if lang in k:
                k = k.split('_')
                if lang == k[0]:
                    return {v:k for k,v in v.items()}
                else:
                    return v
        return "We don't know that language."


class BaseApi(object):

    def __init__(self, c):
        self.c = c

    def say(self, phrase):
        return self.c.translate(phrase)


class BaseClass(object):
    tr = Translator
    api = BaseApi
    lang = 'english'
    id = 'baseclass'
    slogan = 'base slogan'

    def __init__(self):
        self.tr = Translator(self) # instance level access
        # self.tr.Ac = self # class level access (bad idea?)
        self.api = self.api(self)

    # operations methods
    def split(self, string):
        l = string.split(':')
        prefix = l[0]
        try:
            phrase = l[1]
        except:
            phrase = prefix
            prefix = ''
        return (phrase, prefix)

    def process_translate(self, phrase, prefix):
        return prefix+': '+phrase if prefix else phrase

    # adapter methods
    def translate(self, phrase):
        phrase, prefix = self.split(phrase)
        phrase = self.tr.to_adapter(phrase)
        return (phrase, prefix)

    def get_slogan(self):
        slogan = self.slogan
        return self.id+"'s slogan is: "+slogan

# AdapterClass
class DislexiaApi(BaseApi):

    def yas(self, phrase):
        hdys = super(DislexiaApi, self).say(phrase)   
        return hdys

class DislexiaClass(BaseClass):
    id = "dislexia"
    lang = 'english'
    slogan = "sdrawkcab tsuj m'i"
    api= DislexiaApi

    def make_dislexic(self, string):
        return string[::-1]

    def translate(self, phrase):
        phrase, prefix = super(DislexiaClass, self).translate(phrase)
        phrase = self.make_dislexic(phrase)
        return self.process_translate(phrase, prefix)


# Wrapper class returns the api of all Adapters. Nice and clean.
class WrapperClass():
    adapters = [DislexiaClass]
    def __init__(self):
        for adapter in self.adapters:
            setattr(self, adapter.id, adapter().api)

Usage:

# Translator 
print Translator.by_lang('hello world', 'spanish')
hola mundo
print Translator.by_lang('hola mundo', 'english')
hello world

# AdapterClass
d = DislexiaClass()
print d.get_slogan()
dislexia's slogan is: sdrawkcab tsuj m'i
d.translate(slogan)
dislexia's slogan is: i'm just backwards 

# WrapperClass
w = WrapperClass()
wd = w.dislexia
print wd.yas('hello world')
dlrow olleh
print wd.yas('hola mundo')
dlrow olleh
print wd.c.translate(wd.yas('hola mundo'))
hello world
\$\endgroup\$
4
\$\begingroup\$

API

I find the API hard to understand.

The objects also feel over-engineered. At the core, what you want is objects that accept some text and produce some other text. You could just establish the convention that calling an object performs that transformation.

On the other hand, Translator.by_lang('hello world', 'spanish') feels wrong. You want to split that up into the instantiation of a Spanish-to-English translator, and the use of that translator to transform 'hello world'.

Translator.by_lang(source='spanish', target='english')('hello world')

(Your implementation of by_lang() is also working too hard to do a dictionary lookup. See my Trans.__call__() method below.)

An attempt to look up a word for which we don't have a dictionary should raise an exception, I think. Returning "We don't know that language." as text makes it impossible to properly handle a phrase like "No sabemos ese idioma.".

I think that this design would be clearer:

>>> print Trans.en_to_es('hello world')
hola mundo
>>> print Trans.es_to_en('hola mundo')
hello world

>>> print Trans.dislexia.slogan
dislexia's slogan is: sdrawkcab tsuj m'i

>>> print Trans.dislexic_es_to_en('hello world')
dlrow olleh
>>> print Trans.dislexic_es_to_en('hola mundo')
dlrow olleh
>>> print Trans.dislexic_es_to_en(Trans.dislexic_es_to_en('hola mundo'))
hello world

Suggested implementation

class UnsupportedTranslation(Exception):
    def __init__(self, source, target):
        super(UnsupportedTranslation, self).__init__('No %s-to-%s dictionary' % (source, target))

class Translator(object):
    dictionaries = {'english_spanish':{'hello world':'hola mundo', "i'm just backwards":'Solo soy al reves'}}

    @classmethod
    def by_lang(cls, source, target):
        d = cls.dictionary(source, target)
        return Translator(source, target, d)

    @classmethod
    def dictionary(cls, source, target):
        def invert(dictionary):
            return {v: k for k, v in dictionary.iteritems()}
        try:
            return cls.dictionaries.get(source + '_' + target) or \
                   invert(cls.dictionaries.get(target + '_' + source))
        except KeyError:
            raise UnsupportedTranslation(source, target)

    def __init__(self, source, target, dictionary):
        self._source = source
        self._target = target
        self._dictionary = dictionary

    def __call__(self, text):
        '''Returns the translated text, or if the term is not found, the original text.'''
        return self._dictionary.get(text, text)

    @property
    def slogan(self):
        return self._target


## Adapter
class Dislexia(object):
    def __call__(self, text):
        '''Reverses the text.'''
        return text[::-1]

    @property
    def slogan(self):
        return "dislexia's slogan is: sdrawkcab tsuj m'i"

## Filter chain
class Trans:
    def __init__(self, filters):
        self._filters = filters

    def __call__(self, text):
        '''Passes the text through all filters in the chain.'''
        for filter in self._filters:
            text = filter(text)
        return text

    @property
    def slogan(self):
        return self._filters[-1].slogan

# Singletons
Trans.en_to_es = Translator.by_lang(source='english', target='spanish')
Trans.es_to_en = Translator.by_lang(source='spanish', target='english')
Trans.dislexia = Dislexia()
Trans.dislexic_es_to_en = Trans([Trans.es_to_en, Trans.dislexia])
\$\endgroup\$
  • \$\begingroup\$ Thank you for taking the time! It certainly does seem cleaner and allows class level access. I'm going to work through this and see if this type of re factor is feasible. If nothing else, i certainly have been exposed to a design i never would have thought of. \$\endgroup\$ – arctelix Feb 6 '15 at 0:18
  • \$\begingroup\$ Please remember to upvote all answers you find helpful. \$\endgroup\$ – 200_success Feb 6 '15 at 0:20
3
\$\begingroup\$
''' class level version
@classmethod
def to_adapter(cls, phrase):
    r = self.by_lang(phrase, cls.Ac.lang)
    return r
'''

Why leave a completely commented out method in the code base? Are you using source control? This seems like clutter. And especially so since there's no commented reason as to why it's commented out.

Remove the lines, and in the commit message leave an explanation as to why they were removed.

\$\endgroup\$
  • \$\begingroup\$ I will clarify the comment \$\endgroup\$ – arctelix Feb 5 '15 at 21:05

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.