2
\$\begingroup\$

I wrote a class TranslateIt for translating text files with Yandex.Translate service. I feel the code is rather bulky. Is it better to decompose it to several methods? I'm eager to know opinions of experienced people.

import requests
import os
import chardet

class TranslateIt(object):
    """
    methods
    translateit (source, **kwargs)
    translateit - translates source into output
    source - txt file to be translated
    froml - source language, if not specified, yandex tries to autodetect
    tol - translation language, default is ru
    output - translayed txt file

    attributes
    params - parameters dict for yandex api
    encoding - encoding of source file detected by _checkEncoding method
    source_name, output_name - absolute paths of source and output files respectively
    code - result of translation
    message - error message if translation failed
    translated = translated text if succeeded

    constants
    MAX_SIZE - maximum size of a source file according to yandex api
    """

    key = 'trnsl.1.1.20170331T021116Z.53a353fef9d94e08.9617ecfdaf7fbe170972c4107aac8780e53dd21f'
    api_url = 'https://translate.yandex.net/api/v1.5/tr.json/translate'
    MAX_SIZE = 10000
    yandex_string = "\nTranslated by Yandex.Translate service, 'http://translate.yandex.ru/'" #added to translation result according to licence requirements

    #check the encoding of a source file
    def _checkEncoding(self, file_name):
        result = chardet.detect(open(file_name, "rb").read())
        open(file_name).close()
        return result['encoding']


    #translates source to output
    def translateIt(self, source, **kwargs):
        self.params = {'key': TranslateIt.key}
        #check if kwargs are valid
        kwargs_tuple = ('output', 'froml', 'tol')
        if not all([key in kwargs_tuple for key in kwargs.keys()]):
            raise NameError ('Invalid named parameter')
        # check if source file exists
        if os.path.exists(source) and source.endswith('.txt'):
            self.encoding = TranslateIt._checkEncoding(self, source)
            self.source_name = os.path.abspath (source)
            # if source exists, assign it's content to text parameter
            with open(self.source_name, 'r', encoding=self.encoding) as f:
                text = f.read()
                if len (text) < TranslateIt.MAX_SIZE:
                    self.params['text'] = text
                else:
                    raise ValueError('File size can not be more than {} symbols'.format(TranslateIt.MAX_SIZE))
        else:
            raise FileNotFoundError('No data source file found')

        #create translation pair
        if 'tol' not in kwargs.keys():
            kwargs['tol'] = 'ru'
        if 'from' not in kwargs.keys():
            self.params['options'] = 1
            self.params['lang'] = kwargs['tol']
        else:
            self.params['lang'] = kwargs['froml'] + '-' + kwargs['tol']

        #create output file:
        if 'output' in kwargs.keys():
            self.output_name = os.path.abspath(kwargs['output']) if kwargs['output'].endswith('.txt') else os.path.abspath(kwargs['output'] +'.txt')
        else:
            self.output_name = os.path.abspath('translated_' + self.params['lang'] + '_' + os.path.split(self.source_name)[-1])


        #translate, save result in output file if succeeded
        response_result = requests.get(TranslateIt.api_url, params=self.params).json()
        self.code = response_result['code']
        if self.code != 200:
            self.message = response_result['message']
            print ('Translation failed\n Error code: {0}, error message: {1}'.format(self.code, self.message))
        else:
            self.translated = response_result['text'][0] + TranslateIt.yandex_string
            with open(self.output_name, 'w', encoding='utf8') as f:
                f.write(self.translated)
            print ('Translation successful, result saved in {0}'.format(self.output_name))
\$\endgroup\$
  • \$\begingroup\$ See Stop Writing Classes. There's no state, you don't even have __init__. \$\endgroup\$ – jonrsharpe Apr 2 '17 at 8:51
  • \$\begingroup\$ Thanks for your feedback! I read an article with the same title. Actually, I am a beginner with a zero base and now trying to learn OOP basics. I supposed using class can give me more flexibility. \$\endgroup\$ – Mikhail Belousov Apr 2 '17 at 8:57
  • \$\begingroup\$ More flexibility than what, and the flexibility to do what? See also the YAGNI principle. \$\endgroup\$ – jonrsharpe Apr 2 '17 at 8:58
  • \$\begingroup\$ Usually flexilibility to re-use and expand code. For me it's more comfortable to use class attributes than variables in a functions. \$\endgroup\$ – Mikhail Belousov Apr 2 '17 at 9:07
  • \$\begingroup\$ Actually I understand what you mean. I saw class examples like CSVWriter class that are only used as instruments for other classes. My questions was not just about rationality of using the class, I'd like also to get the code reviewed as well if somebody has some spare time) \$\endgroup\$ – Mikhail Belousov Apr 2 '17 at 9:10
2
\$\begingroup\$

Some ideas:

  • If the arguments are a fixed list, don't use **kwargs. I find **kwargs is only really useful if you have some middleware code which needs to forward an arbitrary set of arguments to another piece of code. That way people don't need to rely on your documentation (or when the documentation inevitably gets out of date, the code) to understand which parameters the method takes. Using named parameters you can also get rid of loops over **kwargs.
  • IMO attributes and constants shouldn't be documented at all. Any IDE will be able to auto-complete them, and their names should be sufficiently self-explanatory that you don't need extra documentation.
  • Starting a method name with check implies an assertion is happening. Use get when that is what you're doing.
  • This is a personal style item, but I prefer that the interesting/important code is at the top of the file. _checkEncoding is not that interesting.
  • If key is a secret it should not be hard-coded.
  • Don't second guess TranslateIt's API. For example, don't verify the max size on the client side. Instead you should rely on the API to tell you when the size is too large and forward the information to the user. This way your implementation is simpler and more importantly doesn't get out of sync with the API.
  • You should use the with open() pattern in _checkEncoding.
  • Use pep8 to fix style issues like method names.
  • Your script is not usable from a command line.
  • Why make yandex_string a field? It is only used once, and is never changed, so you might as well treat it as a constant.
  • The code is not testable as it stands. This is too big a subject to expand much on here, but suffice it to say that translateIt does much more than one thing.
\$\endgroup\$
  • \$\begingroup\$ Thanks for a feedback! 1. I'm trying to learn using a command line, can't do it for now \$\endgroup\$ – Mikhail Belousov Apr 2 '17 at 9:43
  • \$\begingroup\$ So, it's better to decompose the code into several smaller pieces, right? \$\endgroup\$ – Mikhail Belousov Apr 2 '17 at 9:49
  • \$\begingroup\$ Absolutely. Have a look at Working Effectively with Legacy Code for many tips on how to do it. \$\endgroup\$ – l0b0 Apr 2 '17 at 10:01
  • \$\begingroup\$ Got it, trying to decompose. One more thing: am I wright that I'd better to use argparse for working with a command line? Can you give me a hint where I can read about it? \$\endgroup\$ – Mikhail Belousov Apr 2 '17 at 10:03
  • \$\begingroup\$ The documentation, tutorial and a simple example should give you plenty to work with. \$\endgroup\$ – l0b0 Apr 2 '17 at 10:08
0
\$\begingroup\$

I tried to decompose and to use more appropriate names according to pep8. Using command line TBD. Below is just a coda that has been changed.

def _get_encoding(self, file_name):
    result = chardet.detect(open(file_name, "rb").read())
    open(file_name).close()
    return result['encoding']


def _get_source(self, source):
    if os.path.exists(source) and source.endswith('.txt'):
        #check the encoding of a source file
        self.encoding = TranslateIt._get_encoding(self, source)
        self.source_name = os.path.abspath (source)
        # if source exists, assign it's content to text parameter
        with open(self.source_name, 'r', encoding=self.encoding) as f:
            text = f.read()
        if len(text) < TranslateIt.MAX_SIZE:
            return text
        else:
            raise ValueError('File size can not be more than {} symbols'.format(TranslateIt.MAX_SIZE))
    else:
        raise FileNotFoundError('No data source file found')


def _set_langs(froml, tol):
    if froml=='auto':
        return tol, 1
    else:
        return(froml + '-' + tol), None


def _set_output(self, output):
    if output=='same':
        self.output_name = os.path.abspath('translated_' + self.params['lang'] + '_' + os.path.split(self.source_name)[-1])
    else:
        self.output_name = os.path.abspath(output) if output.endswith('.txt') else os.path.abspath(output +'.txt')

def _save_output(output_name, text):
    with open(output_name, 'w', encoding='utf8') as f:
                f.write(text)


def translate_it(self, source, output='same', froml='auto', tol='ru'):
    self.params = {'key': TranslateIt.key}
    # get source  file
    self.params['text'] = TranslateIt._get_source(self, source)
    #create translation pair
    self.params['lang'], self.params['options'] = TranslateIt._set_langs(froml, tol)
    #create output file
    TranslateIt._set_output(self, output)
    #translate, save result in output file if succeeded
    try:
        response_result = requests.get(TranslateIt.api_url, params=self.params).json()
        self.code = response_result['code']
        if self.code != 200:
            self.message = response_result['message']
            print('Translation failed\n Error code: {0}, error message: {1}'.format(self.code, self.message))
        else:
            self.translated_text = response_result['text'][0] + TranslateIt.yandex_string
            TranslateIt._save_output(self.output_name, self.translated_text)
            print('Translation successful, result saved in {0}'.format(self.output_name))
    except Exception:
        print("Got unexpected error")
        pass
\$\endgroup\$

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.