3
\$\begingroup\$

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
\$\endgroup\$
4
  • 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\$ – K00lman Feb 26 '20 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 Feb 26 '20 at 16:07
  • 1
    \$\begingroup\$ Please put correct spaces on the code for the class methods. \$\endgroup\$ – camp0 Feb 26 '20 at 16:37
  • \$\begingroup\$ Done. The whitespace somehow got removed. \$\endgroup\$ – FrozenAra Feb 26 '20 at 16:50
2
\$\begingroup\$

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 using del 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 the line 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 the remove_jedec_line() transformation where as mine will. The assumption here is that "COMP" only appears once.
\$\endgroup\$
1
  • \$\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 Feb 26 '20 at 20:13

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.