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
Daniel
4,6122 gold badges18 silver badges40 bronze badges
asked Dec 24, 2017 at 0:01
\$\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.

answered Dec 24, 2017 at 9:30
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.