Code refactoring proposals

epogrebnyak commented 8 years ago

To be listed below

Fak3 commented 8 years ago

Смешение функционала на уровне модулей.

Использование кода делится на два этапа:

парсинг doc-файла в машиночитаемый вид
использование машиночитаемых данных (публикация и всякая манипуляция)

Для упрощения понимания кода эти два этапа лучше максимально разнести по разным модулям(пакетам). Эти два модуля могут разделять только описания схем машиночитаемых данных. (в каком-нибудь виде) Сейчас модули(пакеты) reader и getter взаимно импортруют классы и функции из друг друга и очень трудно понять где между ними граница.

Смешение сущностей в иерархии классов.

class Publisher(Varnames) Создание подкласса необходимо только в том случае, если объекты подкласса будут использоваться для доступа к методам базового класса. Объекты класса Publisher используются для публикации данных, и никогда не используются для доступа ко входным данным. Для генерации входных данных в Publisher достаточно создать внутреннюю переменную self.input_data = Varnames()

Избыточная иерархия классов.

class InputDefinition() class CoreRowSystem(InputDefinition) class RowSystem(CoreRowSystem) Эти классы нигде не используются напрямую. Возможно их логику стоит переместить непосредственно в производный класс CurrentMonthRowSystem

Много микрофункций, состоящих из одной-двух строчек. Лучше заменить их вызовы на их содержимое.

https://github.com/epogrebnyak/rosstat-kep-data/blob/master/kep/getter/dataframes.py#L259 https://github.com/epogrebnyak/rosstat-kep-data/blob/master/kep/getter/dataframes.py#L207 https://github.com/epogrebnyak/rosstat-kep-data/blob/master/kep/getter/dataframes.py#L133 https://github.com/epogrebnyak/rosstat-kep-data/blob/master/kep/getter/publish.py#L48

Функции которые используются только в одном месте можно заменить на их содержимое.

https://github.com/epogrebnyak/rosstat-kep-data/blob/master/kep/reader/rs.py#L164 https://github.com/epogrebnyak/rosstat-kep-data/blob/master/kep/getter/publish.py#L33 https://github.com/epogrebnyak/rosstat-kep-data/blob/master/kep/getter/publish.py#L38

Fak3 commented 8 years ago

Упомянутые проблемы с иерархией классов и с функциями встречаются в коде повсеместно, я дал ссылки только на то что удалось найти за несколько часов. Сейчас подобных проблем в коде очень много, и порог вхождения для сторонних разработчиков измеряется днями

epogrebnyak commented 8 years ago

Дайте разбивку этапов на которые можно разбить проект, чтобы его перерабатывать. Можно ли считать, что блока три:

word2csv - преобразование файлов Word в CSV
csv2db - парсинг "машинночитаемых" CSV файлов и запись в базу данных
db_queries - формирование конечных наборов/выборок данных и их экспорт в итоговые CSV и Excel файлы

Fak3 commented 8 years ago

Лучше начинать с простых вещей которые помогут в процессе понять код. Пакет reader лучше отложить на последнее место, как самый сложный.

1) Развернуть микрофункции и функции которые используются только в одном месте. 2) Возможно стоит вынести из config.py всё что касается пакета reader, и поместить в его собственный отдельный конфиг. 3) Проработать и описать API пакета getter 4) Переделать иерархию классов пакета getter 5) Разработать единую схему обмена машиночитаемыми данными между reader\getter 6) Переделать иерархию классов пакета reader

epogrebnyak commented 8 years ago

Tentative class structure (no todo)

from word2csv import WordDocuments
from csv2db import RowSystem
from db_query import DataFrames

# folder is data folder with current month word files
# import it from config.py

# create raw csv file if it does not exist
WordDocuments(folder).convert_to_csv()

# raw csv file parsing result as generator
flat_data = RowSystem(folder).get_stream()

# get dataframes and save as files 
holder = DataFrames(gen=flat_data)
dfa, dfq, dfm = holder.get_all()
holder.save_as_csv()
holder.save_as_xl()

# example for parsing

csv_text = """Gross domestic product
billion USD
2000    1455    100 110 121 121
rate of growth, %
2000    1,075   0,99    1,10 1,10 1
"""

headers = {"Gross domestic product":"GDP"}
units = {"billion USD":"bln_usd", "rate of growth, %":"rog"}

labelled_rows = [["VAR_X_bln_usd", "2000", "1455",   "300",  "345",  "400", "410"],
                 [    "VAR_X_rog", "2000", "1,075", "0,99", "1,10", "1,10",   "1"]]

flat_data = [("VAR_X_bln_usd", "a", 2000, None, None, 1455),
             ("VAR_X_bln_usd", "q", 2000,    1, None,  300),
             ("VAR_X_bln_usd", "q", 2000,    2, None,  345),
             ("VAR_X_bln_usd", "q", 2000,    3, None,  400),
             ("VAR_X_bln_usd", "q", 2000,    4, None,  410),
             ("VAR_X_rog", "a", 2000, None, None, 1.075),
             ("VAR_X_rog", "q", 2000,    1, None,  0.99),
             ("VAR_X_rog", "q", 2000,    2, None,  1.10),
             ("VAR_X_rog", "q", 2000,    3, None,  1.10),
             ("VAR_X_rog", "q", 2000,    4, None,     1)]

epogrebnyak commented 8 years ago

1) Развернуть микрофункции и функции которые используются только в одном месте.

не приоритетное

2) Возможно стоит вынести из config.py всё что касается пакета reader, и поместить в его собственный отдельный конфиг.

под вопросом, может быть

3) Проработать и описать API пакета getter

API достаточно простой (получить фреймы dfa, dfq, dfm + записать их в + функционал по varnames - названиям и расшифровкам переменных)

4) Переделать иерархию классов пакета getter

нужно делать

5) Разработать единую схему обмена машиночитаемыми данными между reader\getter

Можно уточнить, где есть признаки обмена данными между reader\getter, кроме того что getter получает информацию от reader? Varnames() - он берет данные из разных частей?

6) Переделать иерархию классов пакета reader

можно делать

Fak3 commented 8 years ago

API достаточно простой (получить фреймы dfa, dfq, dfm + записать их в + функционал по varnames - названиям и расшифровкам переменных)

API должен исходить из конкретных юзкейсов, которые у меня пока не получается представить. Кто будет потенциальным пользователем API? Какие именно практические цели поможет ему достичь этот проект?

epogrebnyak commented 8 years ago

Юзеркейс тут: https://github.com/epogrebnyak/rosstat-kep-data#Как-использовать-эти-данные Кейс для админа базы данных: https://github.com/epogrebnyak/rosstat-kep-data#Как-обновить-данные

Fak3 commented 8 years ago

Юзеркейс тут: https://github.com/epogrebnyak/rosstat-kep-data#Как-использовать-эти-данные

Интересует скорее не "как" использовать, а "зачем" и кому это будет нужно? То есть конкретно вот эти два вопроса:

Кто будет потенциальным пользователем API?
Какие именно практические цели поможет ему достичь этот проект?

Касаемо того как использовать - возникает вопрос зачем нужна sqlite БД, если она в юзкейсе не используется?

epogrebnyak commented 8 years ago

Я затрудняюсь кто и зачем скпщать: пользователи экономической статистики, для построения иллюстраций и моделей. Им нужен обновляемый цсв файл. Как еще обьяснить? 7 апр. 2016 г. 17:11 пользователь "Evstifeev Roman" < notifications@github.com> написал:

Юзеркейс тут: https://github.com/epogrebnyak/rosstat-kep-data#Как-использовать-эти-данные

Интересует скорее не "как" использовать, а "зачем" и кому это будет нужно? То есть конкретно вот эти два вопроса:

Кто будет потенциальным пользователем API?

Какие именно практические цели поможет ему достичь этот проект?

Касаемо того как использовать - возникает вопрос зачем нужна sqlite БД, если она в юзкейсе не используется?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/epogrebnyak/rosstat-kep-data/issues/128#issuecomment-206924258

Fak3 commented 8 years ago

для построения иллюстраций и моделей. Им нужен обновляемый цсв файл.

Ок. Имеющийся юзкейс покрывает только сsv файл. Хочется понять для чего тогда нужен класс KEP и БД?

epogrebnyak commented 8 years ago

База данных сейчас может бвть не нужна, в итоге она понадобится, чтобы хранить данные из нескольких источников, а не только рещультат парсинга последнего csv. Еще может быть нудна для хранения старвх версий данных (когда в данных за март есть пересмотр данных за январь).

Fak3 commented 8 years ago

KEP - это обертка, млжет быть, иожет не быть.

Предазначение этой обертки непонятно - какие преимущества ее использования по сравнению с созданием стандартного dataframe прямо из csv? Это неплохо бы описать в юзкейсе. Текущий юзкейс описывает как пользователь может использовать данные без изучения дополнительных API, в одну строчку кода. Необходимость в каких-то еще обертках в этом случае у меня не получается оправдать:

dfm = pd.read_csv("data_monthly.txt", converters={'time_index':pd.to_datetime}, index_col='time_index')

epogrebnyak commented 8 years ago

Дайте подумаю... KEP - действительно тонкая обертка для создания датафреймов, она может быть или может не быть. Наверное он мне дорог как упражнение, если считаете что лишний, можно его в новой версии убрать.

Предазначение этой обертки непонятно - какие преимущества ее использования по сравнению с созданием стандартного dataframe прямо из csv? Это неплохо бы описать в юзкейсе. Текущий юзкейс описывает как пользователь может использовать данные без изучения дополнительных API, в одну строчку кода. Необходимость в каких-то еще обертках в этом случае у меня не получается оправдать:

dfm = pd.read_csv("data_monthly.txt", converters={'time_index':pd.to_datetime}, index_col='time_index')

epogrebnyak / data-rosstat-kep

Code refactoring proposals #128