5
\$\begingroup\$

I was making this file editing class (writing to a file, reading a file etc.) in Python and was just looking for any feedback.

Note: I've been thinking about removing the file_name property and requiring file name on each method call.

import os

class EditFile:

    """
    format: <object_name> = EditFile(<file_name>)
    """

    # constructor
    def __init__(self, file_name):
        self.file_name = file_name  # can be changed at any time

    # string version
    def __repr__(self):
        return "<object_name> = EditFile(<file_name>)"

    # methods
    def write(self, text):  # writes text in a file
        try:
            f = open(self.file_name, "w")  # opens file & selects WRITE mode also creates a new file if it doesnt exist
            f.write(str(text))

        finally:
            f.close()

    def append(self, text):  # appends text to a file
        try:
            f = open(self.file_name, "a")  # opens file & selects APPEND mode
            f.write(text)

        finally:
            f.close()

    @property
    def read(self):  # reads the file
        try:
            f = open(self.file_name, "r")  # opens file & selects READ mode
            content = f.read()  # saves content of the file into content variable

        finally:
            f.close()

        return content  # returns content of the file

    def read_bytes(self, number_of_bytes = 0):  # reads certain amount of bytes
        try:
            int(number_of_bytes)

            if number_of_bytes == 0:  # checks if its default
                f = open(self.file_name, "rb")
                content = f.read()

                return content

            elif number_of_bytes >= 1:  # checks if number of bytes is a valid number
                f = open(self.file_name, "rb")
                content = f.read(number_of_bytes)

                return content

            else:
                return "error"  # returns "error" if input is invalid

        except ValueError:
            return "value error"

        finally:
            f.close()

    def write_bytes(self, text):  # writes in bytes
        try:
            f = open(self.file_name, "wb")  # selects WRITE BYTES mode
            f.write(str(text))

        finally:
            f.close()

    @property
    def clear(self):  # clears a file
        try:
            f = open(self.file_name, "w")  # opens file and selects WRITE mode
            f.write("")  # sets file content to none

        finally:
            f.close()

    @property
    def delete(self):  # deletes current file
        try:
            os.remove(self.file_name)  # tries to delete file

        except OSError:
            return "error"  # returns "error" if an error occurs (eg file doesnt exist)

# example program using most of the methods
file = EditFile("editme.txt")

file.write(str(input("Text:\n")))
print(file.read)
file.clear
file.delete

# editing file: editme2.txt
file.file_name = "editme2.txt"
file.write(str(input("Text:\n")))
print(file.read_bytes(5))
file.delete
\$\endgroup\$

1 Answer 1

8
\$\begingroup\$
"<object_name> = EditFile(<file_name>)"

is not a particularly useful __repr__. Note the guidance in the data model documentation:

If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment). If this is not possible, a string of the form <...some useful description...> should be returned.

In this case, that could be:

"EditFile({!r})".format(self.file_name)

For all of your file access, you should use the context manager provided by open, e.g.:

@property
def read(self):  # reads the file
    with open(self.file_name) as f:
        return f.read()

This will handle closing the file for you after the method ends.


It seems weird that read is a property but read_bytes is a method - as the latter requires an argument, and both names describe actions, perhaps they should both be methods? If you still wanted the property then a name describing a thing, like content, would be better for it.


I would generally expect attribute access not to change state, so it seems even weirder for clear and delete, again actions rather than things, to be properties. Just make them methods. Also the fact that they return either None (if successful) or a string (if there was an error) is a really strange interface for a consumer of the class to deal with.


Your error handling in general is a bit odd. Why take a useful exception and squash it down to a meaningless string? You're giving the users of your class less information. If you can't add to them or usefully handle them, let the errors continue through your code to the caller. And rather than:

return "error"  # returns "error" if input is invalid

raise an error with a useful message yourself, in this case e.g.:

raise ValueError("cannot read a negative number of bytes")

This actually tells the user something useful.

Also note that:

int(number_of_bytes)

although it checks that number_of_bytes could be made an integer, doesn't actually change what's assigned to the name; if it was '1', for example, weird things will happen next. Instead, use the result:

number_of_bytes = int(number_of_bytes)

Rather than using simple comments to describe your methods, use docstrings:

def write_bytes(self, text):
    """Writes in bytes."""
    ...

Unlike comments these can be used by other tools, e.g. the built-in help function, to make that information available in other forms.


self.file_name = file_name  # can be changed at any time

Here is a good use for a @property; you could use it to make the filename read-only by convention:

self._file_name = file_name

The leading underscore indicates that this should be considered private, then you can write a getter:

@property
def file_name(self):
    return self.file_name

Your other idea, requiring the filename every time a method is called, would turn this into a class without any instance state, in which case it probably shouldn't be a class at all.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.