8
\$\begingroup\$

I studying for a Python course. I was finished working on task about creating a file class which can read, write lines to file. This class also needs magic methods (__add__, __str__, __iter__, __next__)

Can you review code and give a tip about improvements using the DRY principle (for example, how avoid to copy/paste with open).

from tempfile import gettempdir
from os import path
import uuid


class File():

    def __init__(self, file_path):
       self.file_path = file_path
       self.current_position = 0

    def __iter__(self):
       return self

    def __next__(self):
        try:    
            with open(self.file_path, 'r') as f:
                f.seek(self.current_position)
                line = f.readline().strip()
                if not line:
                    self.current_position = 0
                    raise StopIteration('EOF')
                self.current_position = f.tell()
                return line
        except IOError:
            return 'Cant read next line'

    def write(self, line):
        try:
            with open(self.file_path, 'w') as file_obj:
                file_obj.write(line)
        except IOError as err:
            return 'Cant write to file: {0}'.format(err)

    def read(self):
        try:
            with open(self.file_path, 'r') as file_obj:
                return file_obj.read().strip()
        except IOError:
            return ''

    def __add__(self, obj):
        if isinstance(obj, File):
            tmp_file_path = path.join(gettempdir(), str(uuid.uuid4()))
            tmp_file = File(tmp_file_path)
            tmp_file.write(self.read() + '\n' + obj.read())
            return tmp_file

    def __str__(self):
        return self.file_path


f_obj = File('somepath1')
f_obj2 = File('somepath2')
f_obj3 = f_obj + f_obj2
for line in f_obj:
    print(line)
print(f_obj3.read())
\$\endgroup\$
  • \$\begingroup\$ Is there some reason you want to keep opening and closing the file again and again, instead of keeping it open? \$\endgroup\$ – 200_success Apr 27 '18 at 0:38
5
\$\begingroup\$

__iter__ and __next__

Instead of re-opening the file each time and seeking to the previous position, you could write __iter__ as a generator function by using yield instead of needing to define __next__. Something like:

def __iter__(self):
    with open(file) as f:
        yield from f

Error handling

In case of error, you're returning a string 'Cant read next line'. What if you have a file that contains text 'Cant read next line'? How do you differentiate between these?

Well... you can't.

Instead of returning a string, throw an exception on error - that's what exceptions are meant for.

Also, this error message of yours isn't really correct. You most likely get IOError when opening a file... before even attempting to read first line from it.

It seems like your error handling code doesn't really do anything very useful with the IOError. So why not instead just let it bubble upwards and drop the try-catch block completely.

Duplication

You can define your own open method, so you don't have to repeat passing the file_path to it:

def open(self, mode='r'):
    open(self.file_path, mode)

You could also investigate writing you own context manager for opening the file with your custom error handling.

\$\endgroup\$
  • \$\begingroup\$ The answer is more than complete. Thank you! Now I know how to improve the code and learn the best practices. \$\endgroup\$ – Larionov Nikita Apr 27 '18 at 17:48
  • 1
    \$\begingroup\$ Your __iter__ yields just a single line. Don't you want yield from f? \$\endgroup\$ – zondo Apr 27 '18 at 20:57
  • \$\begingroup\$ Good point. I've updated the example. \$\endgroup\$ – Rene Saarsoo Apr 28 '18 at 12:53

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.