Ctrl →

zlodiak · Апрель 7, 2019 17:53:58

скажите пожалуйста, всегда ли нужно соблюдать принцип единой ответственности? я стараюсь писать код так чтобы класс решал одну задачу и по возможности лежал в отдельном модуле

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

вот посмотрите пожалуйста как выглядит мой код с основной логикой приложения:

 #!/usr/bin/env python3
import config
from parser import Parser
from db_connection import DB_connection
from logger import Logger
if __name__ == '__main__':
    logger = Logger()
    with DB_connection(config.dbname, config.user, config.password, config.host, logger) as db_connection:
        try:
            db_cursor = db_connection.cursor()
        except Exception as e:
            msg = 'Database ' + str(config.dbname) + ' connection is failed ' + str(e)
            logger.write_to_log(msg)   
        try:
            parser = Parser(config.headers, logger)
            html = parser.get_page_html(config.url)
            links_to_inner = parser.get_links_to_inner(html)
        except Exception as e:
            msg = 'Parse operations for ' + str(config.url) + ' is failed ' + str(e)
            logger.write_to_log(msg)         
        if links_to_inner:
            for link in links_to_inner:
                html = parser.get_page_html(link)
                content_page_inner = parser.get_content_page_inner(html)
                try:
                    req = 'INSERT INTO parse_results2 (public_date, position, company, pay, description) VALUES (%s,%s,%s,%s,%s)'
                    vals = (
                        content_page_inner['public_date'],
                        content_page_inner['position'],
                        content_page_inner['company'],
                        content_page_inner['pay'],
                        content_page_inner['desc']
                    )
                    db_cursor.execute(req, vals);        
                except Exception as e:
                    msg = 'Write to table is failed: ' + str(e)
                    logger.write_to_log(msg)  
            if 'db_cursor' in dir():
                db_cursor.close()

код отдельных классов не привожу потому что они не имеют отношения к вопросу. но если интересно, то репозиторий здесь

Rodegast · Апрель 7, 2019 19:29:14

> я стараюсь писать код так чтобы класс решал одну задачу и по возможности лежал в отдельном модуле

С чего ты взял что класс должен лежать в отдельном модуле?

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

Твой код получает список ссылок и сохраняет их в БД. Т.е. он решает 2 задачи. Разбей его на функцию которая возвращает список ссылок и на функцию которая записывает их в БД.

С дураками и сектантами не спорю, истину не ищу.
Ели кому-то правда не нравится, то заранее извиняюсь.

zlodiak · Апрель 7, 2019 20:40:09

Rodegast
> я стараюсь писать код так чтобы класс решал одну задачу и по возможности лежал в отдельном модуле С чего ты взял что класс должен лежать в отдельном модуле?> проблема в том, что код, который содержит основную логику приложения, и который управляет описанными выше классами по-моему в принципе не может подчиняться принципу единой ответственности - то есть решать одну задачу.Твой код получает список ссылок и сохраняет их в БД. Т.е. он решает 2 задачи. Разбей его на функцию которая возвращает список ссылок и на функцию которая записывает их в БД.

так?:

 import config
from classes.parser import Parser
from classes.db_connection import DB_connection
from classes.logger import Logger
from classes.sql_requester import Sql_requester
if __name__ == '__main__': 
    def get_links():
        try:
            parser = Parser(config.headers, logger)
            html = parser.get_page_html(config.url)
            links_to_inner = parser.get_links_to_inner(html)
            return {
                'links_to_inner': links_to_inner,
                'parser': parser
            }
        except Exception as e: 
            msg = 'Parse operations for ' + str(config.url) + ' is failed ' + str(e)
            logger.write_to_log(msg)            
            return False  
    def parse_links(links, sql_requester):
        for link in links['links_to_inner']:
            html = links['parser'].get_page_html(link)
            content_page_inner = links['parser'].get_content_page_inner(html)
            sql_requester.fill_table(content_page_inner)
    logger = Logger()
    with DB_connection(config.dbname, config.user, config.password, config.host, logger) as db_connection:
        try:
            db_cursor = db_connection.cursor() 
            sql_requester = Sql_requester(db_cursor, logger)
        except Exception as e:
            logger.write_to_log('Connection is failed')
        else:
            if sql_requester.clear_table():  
                links = get_links()   
                if links: parse_links(links, sql_requester)
        finally:
            db_cursor.close()

FishHook · Апрель 8, 2019 06:15:47

zlodiak
так

Что на вас такое нашло то? Почему вы вернулись к процедурному стилю? Было же все хорошо, паттерны изучали, и тут вдруг опять какие-то глупости. Как вы к этому коду будете ранее изученые паттерны применять?
Да и вообще, вы же понимаете, что спрятав свои функции за if __name__ == ‘__main__’, вы фактически сделали их нетестируемыми. Модульные тесты вы теперь сделать не можете, и вот ради чего?
Почему функция get_links возвращает False? Разве это предикат? Разве вы задаете вопрос are_links_exist?

Вообще код какой-то бредовый.
1) Переписать программу в ООП стиле.
2) Применять паттерны “Стратегия”, “Внедрение зависимостей”
3) Убрать все неявное - никаких словарей в ответе метода
4) DB_connection принимает слишком много позиционных аргументов - яркий маркер плохо декомпозированной архитектуры.
5) Не надо бояться эксепшенов, если в программе случается нештатная ситуация, я жду что эта программа упадет с ошибкой, а не тихо зщавершится, шепнув чего-то там в лог. Если я буду использовать ваши модули в своей программе, предоставьте мне самому возможность выбирать, как обработать исключение, не делайте этого за меня.
6) Ниже if __name__ == ‘__main__’ не должно быть никакой логики - только инициализации. В идеале - один вызов. Вот, например, так запускается довольно большой проект на Джанго:

   
if __name__ == "__main__":
    os.environ.setdefault("DJANGO_SETTINGS_MODULE", "project.settings")
    from django.core.management import execute_from_command_line
    execute_from_command_line(sys.argv)

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

Rodegast · Апрель 8, 2019 11:04:13

> так?

Нет, примерно вот так:

 import config
from parser import Parser
from db_connection import DB_connection
from logger import Logger
 
def get_links(logger):
        parser = Parser(config.headers, logger)
        html = parser.get_page_html(config.url)
        res = []
        for x in parser.get_links_to_inner(html):
                res.append(parser.get_content_page_inner(parser.get_page_html(x)))
        return res
 
def writer(data, logger):
        if data:
                with DB_connection(config.dbname, config.user, config.password, config.host, logger) as db_connection:
                        db_cursor = db_connection.cursor()
                        for x in data:
                                db_cursor.execute(
                                        "INSERT INTO parse_results2 (public_date, position, company, pay, description) VALUES (%s,%s,%s,%s,%s)",
                                        (x['public_date'], x['position'], x['company'], x['pay'], x['desc'])
                                )
                        db_cursor.close()
 
if __name__ == '__main__':
        logger = Logger()
        writer(get_links(logger), logger)

С дураками и сектантами не спорю, истину не ищу.
Ели кому-то правда не нравится, то заранее извиняюсь.

Отредактировано Rodegast (Апрель 9, 2019 10:47:26)

zlodiak · Апрель 9, 2019 12:23:24

FishHook

как оказалось, паттерны изучать намного проще чем применять) даже такой простой парсер мне пока сложно написать. я решил упростить задачу: временно отказаться от логгера и блоков try/except. посмотрите пожалуйста в правильном ли направлении я двигаюсь.

в частности смущает, что после сокращения кода инициализации до такого:

 if __name__ == '__main__': 
    Main(config, credentials, Parser, DB_connection, Sql_requester)

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

 import config
import credentials
from classes.parser import Parser
from classes.db_connection import DB_connection
from classes.sql_requester import Sql_requester
class Main:
    def __init__(self):

ссылка на репозиторий

Отредактировано zlodiak (Апрель 9, 2019 12:24:42)

FishHook · Апрель 9, 2019 12:47:29

zlodiak
которые можно было просто импортировать, например так:

А как вы будете мокать коннекшены и пр. для тестов? Вы же не хотите, чтобы ваши тесты работали на продакшен-базе данных. Старайтесь не импортировать объекты, а создавать их по месту необходимости. Синглтон - это не такой уж хороший паттерн.

zlodiak · Апрель 9, 2019 18:53:38

FishHook
Старайтесь не импортировать объекты, а создавать их по месту необходимости

Но как в этом случае быть с большим количеством передаваемых аргументов, которые, как вы говорили ранее, являются признаком плохой архитектуры?

например вот такой код выглядит уже странно

 if __name__ == '__main__': 
    Main(config, credentials, Parser, DB_connection, Sql_requester, classes.exceptions, Logger)

можно конечно все аргументы собрать в объект и передавать только его, но сути это не изменит

Отредактировано zlodiak (Апрель 9, 2019 18:56:24)

FishHook · Апрель 10, 2019 06:06:36

zlodiak
Но как в этом случае быть с большим количеством передаваемых аргументов

А откуда вы их собрались передавать? Main - это ваша точка входа, она кроме аргументов командной строки вообще ничего принимать не должна.

 if __name__ == '__main__': 
    config = Config("production_settings.ini") 
    parser = Parser(file=sys.argv[1], root="root")
    db = DbConnection("db_settings.ini")
    app = App()
    app.setConfig(config)
    app.setParser(parser)
    app.setDb(db)
    app.run()

test.py

    
import unittest
 
class TestAppCase(unittest.TestCase):
    def setUp(self):
        config = Config("test_settings.ini") 
        parser = Parser(file="test_data.html", root="root")
        db = DbConnection("test_db_settings.ini")
        app = App()
        app.setConfig(config)
        app.setParser(parser)
        app.setDb(db)
        self.app = app        
    def test_result(self):
        self.app.run()
        self.assertEqual(self.app.result, {"count": 100},
                         'incorrect result')

Я вам не просто так рекомендую написать модульные тесты. Как только вы попробуете это сделать, принцип единичной ответственности войдет в ваш код автоматически. Нужно протестировать каждый публичный метод по возможности используя не более одного assert на тест.

zlodiak · Апрель 10, 2019 23:32:33

FishHook
Я вам не просто так рекомендую написать модульные тесты. Как только вы попробуете это сделать, принцип единичной ответственности войдет в ваш код автоматически. Нужно протестировать каждый публичный метод по возможности используя не более одного assert на тест.

я добавил юниттесты и теперь вижу неправильные вещи. в частности вижу, что класс Main() не соответствует принципу единичной ответственности потому что теперь явно видно, что он:
1. получает результат парсинга (это я протестировал)
2. записывает результат парсинга (это пока не протестировал)

то есть я не могу протестировать результат работы Main() конкретным единичным тестом. про эту пользу от юниттестов вы говорили выше?

вот мой класс Main:

 class Main:
    def __init__(self, exceptions):
        self.exceptions = exceptions
        self.content = []
    def set_db_connection(self, db_connection):
        self.db_connection = db_connection
    def set_parser(self, parser):
        self.parser = parser
    def set_logger(self, logger):
        self.logger = logger
    def set_Sql_requester(self, Sql_requester):
        self.Sql_requester = Sql_requester
    def run(self):
        try:
            self.get_content()
            self.save_content()
        except (self.exceptions.ErrorGetContent, 
                self.exceptions.ErrorConnectDB, 
                self.exceptions.ErrorClearTable, 
                self.exceptions.ErrorSaveContent) as e:
            msg = e
            self.logger.write_to_log(msg)
            print('E::', msg)   # debug info        
    def get_content(self):
        self.content = self.parser.get_content()           
    def save_content(self):
        with self.db_connection as db_connection:
            db_cursor = db_connection.cursor() 
            try:
                sql_requester = self.Sql_requester(db_cursor, self.exceptions)
                sql_requester.clear_table()
                sql_requester.fill_table(self.content)
            except (self.exceptions.ErrorClearTable,
                    self.exceptions.ErrorTableInsert) as e:
                msg = 'Save content is failed. ' + str(e)
                self.logger.write_to_log(msg)
                raise self.exceptions.ErrorSaveContent(msg)

Вот тест к нему:

 import unittest
import config
import credentials
import classes.exceptions
from classes.parser import Parser
from classes.db_connection import DB_connection
from classes.sql_requester import Sql_requester
from classes.logger import *
from classes.main import Main
 
class TestMainCase(unittest.TestCase):
    def setUp(self):
        logger = Logger(HumanFormatStrategy)
        db_connection = DB_connection(credentials, classes.exceptions, logger)
        parser = Parser(config, classes.exceptions, logger)
        main = Main(classes.exceptions)
        main.set_db_connection(db_connection)
        main.set_parser(parser)
        main.set_logger(logger)
        main.set_Sql_requester(Sql_requester)
        self.main = main     
    def test_content(self):
        self.main.run()
        self.assertNotEqual(len(self.main.content), 0)
if __name__ == '__main__':
    unittest.main()

вот репозиторий

Python-сообщество

Уведомления

#1 Апрель 7, 2019 17:53:58

всегда ли нужно соблюдать принцип единой ответственности?

#2 Апрель 7, 2019 19:29:14

всегда ли нужно соблюдать принцип единой ответственности?

#3 Апрель 7, 2019 20:40:09

всегда ли нужно соблюдать принцип единой ответственности?

#4 Апрель 8, 2019 06:15:47

всегда ли нужно соблюдать принцип единой ответственности?

#5 Апрель 8, 2019 11:04:13

всегда ли нужно соблюдать принцип единой ответственности?

#6 Апрель 9, 2019 12:23:24

всегда ли нужно соблюдать принцип единой ответственности?

#7 Апрель 9, 2019 12:47:29

всегда ли нужно соблюдать принцип единой ответственности?

#8 Апрель 9, 2019 18:53:38

всегда ли нужно соблюдать принцип единой ответственности?

#9 Апрель 10, 2019 06:06:36

всегда ли нужно соблюдать принцип единой ответственности?

#10 Апрель 10, 2019 23:32:33

всегда ли нужно соблюдать принцип единой ответственности?

Board footer