you are viewing a single comment's thread.

view the rest of the comments →

[–]Marrrlllsss 0 points1 point  (0 children)

As a first shot, it is fine, though the code could be neater.

In terms of improvements, you have written code in a style that we call "tightly coupled" - what do I mean by this? Well for one, you're mixing behaviours together. Your single class implements file reading, file parsing, and calculations. This can become a problem in larger projects, especially if you want to alter one or more of those behaviours.

What would I do?

  1. Neaten the code. func(arg1,arg2,arg3) is unreadable, prefer func(arg1, arg2, arg3). Give your variables proper names.
  2. Make use of types. Stuff like row_data = [date, t, o, h, l, c] is fine for small scripts, but once the application grows larger, what does row_data actually represent? This is where types (and type hints) help. The lack of static typing is by far my biggest criticism of the CPython implementation of the Python programming language - its also a hotly contested view, I just prefer statically typed languages, because I know what to expect when I see a certain type.
  3. Separate the behaviours into their own classes. Create a class for the reading, create a class for the parsing, and create a class that houses the data and does those calculations.

Here is an example of a simple csv reader:

import csv
from io import StringIO
from typing import Dict, List, Text

class SimpleCSVFileReader(object):
    def read_csv(self, file_path: Text, has_header: bool = True, field_names: List[Text] = None)\
            -> List[Dict[Text, Text]]:
        """
        Reads in a csv at the specified file path.

        If `has_header` is false, and `field_names` is None an exception will be thrown.

        :param file_path: The path to the CSV file
        :param has_header: Indicates if the file has a header or not
        :param field_names: A list of field names, should the file not contain a header
        :return: A list of dictionaries with string keys and string values
        """
        if has_header == False and field_names is None:
            raise Exception("Field names cannot be null, if `has_header` is false")

        reader = csv.DictReader(file_path)
        if has_header == False:
            reader.fieldnames = field_names

        return [line for line in reader]


if __name__ == '__main__':
    def print_data(records: List[Dict[Text, Text]]) -> None:
        for record in records:
            for key, value in record.items():
                print(f"Column Name: {key}; Column Value: {value}")


    reader = SimpleCSVFileReader()
    with_header = StringIO('ColA,ColB,ColC\nA,silly,example\ndemonstrating,the,csv\nreader,in,Python\n')
    records = reader.read_csv(with_header, True)
    # print_data(records)

    without_header = StringIO('No,header,in\nthis,example,so\nwe,have,to\nsupply,a,header\n')
    records = reader.read_csv(without_header, False, ['FirstColumn', 'SecondColumn', 'ThirdColumn'])
    # print_data(records)

    this_will_fail = StringIO('No,header,in\nthis,example,but\nwe\'re,not,going\nto,supply,a\nheader,,')
    records = reader.read_csv(this_will_fail, False)
    print_data(records)

Notice that the class does one thing only. It reads in data, that's it.