I am currently coding a tool for my work that modifies a textfile by removing unneeded parts of a string at specific spots in the file. I would like to know if there are any obvious things I am missing or doing too complicated. I know that some of the comments are considered too long for PEP8.
class vis_file_tools:
def __init__(self):
pass
# Splits the input line into list (by a space), removes the last position, joins them with spaces and returns the modified line.
def remove_jedec_line(self, input_line):
input_line_split = input_line.split(" ") # splits the string that was input through the function into a list
del input_line_split[-1] # deletes the last line from the list (In this case it removes the jedec)
output_line = " ".join(input_line_split) # joins back the list to a string. Joins the list with a space
output_line = output_line + "\n"
return output_line # returns the finished string to the caller of the function
def remove_jedecs_file(self, path_input_file, path_output_file):
with open(path_input_file, "r") as file: # Opens the input file read only
temp_data = ""
comp_line_reached = False
for line in file: # "line" in this case is already a variable. Do not us file.readline() when using the "for line" loop.
if "COMP" in line: # Sets the bool "comp_line_reached" to "true" when the loop reaches the "COMP" in the .vis file which marks the start of the components
comp_line_reached = True
temp_data = temp_data + line
elif comp_line_reached is False: # Simply writes the lines from the file into a variable until the component part is reached.
temp_data = temp_data + line
else:
line_no_jedec = self.remove_jedec_line(line)
temp_data = temp_data + line_no_jedec
with open(path_output_file, "w") as write_file:
write_file.write(temp_data)
temp_data = "" # Empties memory
-
1\$\begingroup\$ Your question seems fine, but you should change your title to be more of a general description of what your code does. Asking if it has any problems, like your title does, is implied in every question is not needed. For more info, check out the help center. (codereview.stackexchange.com/help/how-to-ask) \$\endgroup\$user211881– user2118812020年02月26日 15:38:57 +00:00Commented Feb 26, 2020 at 15:38
-
1\$\begingroup\$ I think I missunderstood the help beforehand. I tried to find a better title now. Thank you for the hint. \$\endgroup\$FrozenAra– FrozenAra2020年02月26日 16:07:04 +00:00Commented Feb 26, 2020 at 16:07
-
1\$\begingroup\$ Please put correct spaces on the code for the class methods. \$\endgroup\$camp0– camp02020年02月26日 16:37:39 +00:00Commented Feb 26, 2020 at 16:37
-
\$\begingroup\$ Done. The whitespace somehow got removed. \$\endgroup\$FrozenAra– FrozenAra2020年02月26日 16:50:34 +00:00Commented Feb 26, 2020 at 16:50
1 Answer 1
PEP-8
ClassNames
should be BumpyWords
; snake_case
is used for things like method_names
and variable_names
. So you class should be:
class VisFileTools:
...
Unnecessary __init__
method
This method does nothing; it may safely be omitted.
def __init__(self):
pass
Public / Private
remove_jedec_line
looks like a private method, used by the remove_jedecs_file
method. If remove_jedec_line
is not supposed to be used by actors outside of the class, it should be prefixed with a leading underscore:
def _remove_jedec_line(self, inputline):
...
Garbage Collection
temp_data = "" # Empties memory
This is unnecessary. The variable is about to go out of scope, which will naturally release the memory it is holding.
Stop Writing Classes
See the video Stop Writing Classes
Your class is unnecessary. It has no data members. It has one public method. It should just be a function.
Simplified code
Here is a function version of your code (untested):
def remove_jedecs_file(path_input_file: str, path_output_file: str) -> None:
"""
Read `path_input_file`, copying each line to `path_output_file`.
Once the start of components is reached (indicated by a line containing "COMP"),
the last "word" (jedec) of each line is removed.
"""
def remove_jedec_line(input_line: str) -> str:
return " ".join(input_line.split(" ")[:-1]) + "\n"
with open(path_input_file) as file, open(path_output_file, "w") as write_file:
comp_line_reached = False
for line in file:
if comp_line_reached:
line = remove_jedec_line(line)
else:
comp_line_reached = "COMP" in line
write_file.write(line)
Notes:
- Type-hints (Python 3.6+) have been added to the function (
: str
and-> None
) - A
"""docstring"""
has been added to describe the function. - A nested function is used for
remove_jedec_line()
, making it "private". [:-1]
returns all but the last element in a list, which is simpler than usingdel input_line_split[-1]
.- Lines are not being accumulated in
temp_data
. Instead, lines are written out immediately after being read in, which is a much lighter load on memory, and avoid \$O(N^2)\$ string concatenation. "COMP" in line
is a "slow" search operation, looking for that substring in theline
string. When found, it sets a flag. Here, once the flag is set, we can skip the slow substring search. But I'm introducing a slight change in behaviour: if subsequent lines also contain"COMP"
, the OP version would not do theremove_jedec_line()
transformation where as mine will. The assumption here is that"COMP"
only appears once.
-
\$\begingroup\$ Thanks a lot. I have to read up on the data members stuff for classes. Comp in this case basically declares a part of the file that gets repeated for a specific amount of times. After that amount is done a new section of the text starts with comp. So I have to recheck the appearence. I think. I will try the function when I am at work next time. Thank you for your help. I was planning on adding more functions to the class. But I dont know much about data members yet so I need to read up on that if its better to just create more functions. \$\endgroup\$FrozenAra– FrozenAra2020年02月26日 20:13:41 +00:00Commented Feb 26, 2020 at 20:13