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

Всем привет. :) Извиняюсь за длинное изложение, но очень важно передать суть проблемы.

<Лев Толстой mode on>
Мне была поставлена задача, написать приложение, которое вычисляет некоторый хэш с помощью некоторой функции от 100к соли и делает распределение полученного результата (т.е. полученные 100к хэшей) по заданному файлу (те делается конкатенация строки из файла и хэша строка::хэш). Приложение должно эффективно использовать системные ресурсы.

Я написал следующий код (во вложении) и в результате получил примерно следующий комментарий “в решении есть логические недочеты и на большой платформе это выйдет боком, тебе необходимо менять мышление.”

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

Приму любую помощь тут или по скайпу “bel.mih”.
</Лев Толстой mode off>

Отредактировано zloymih (Фев. 17, 2016 20:28:36)

Прикреплённый файлы:
masterfoo.py (3,9 KБ)

1)
Вот у тебя везде там sys.exit() по коду разбросаны. Представь, что этот код импортируется в другой код и становится там лишь мелкой частью какой-то. Зачем эти sys.exit()'ы? Или ты думаешь “я потом переделаю”? Не переделаешь, потом надо будет делать другие дела. В главном потоке должен обрабатываться выход, чтобы можно было по-другому реагировать, если поток не сработал.

2)
Там, когда файл читаешь, весь его загоняешь в память. (Ну, это легко исправить.)
Используй генераторы.

zloymih

lines = list(s for s in map(string.strip, my_file))

Что это вообще за хрень? Во втором питоне map() и так возвращает список.
Может, конечно, файл закрыться, когда генератор весь не исчерпан, но в память загонять - это неправильно. А если там несколько гигабайт?

3)
В потоки надо подавать не имена файлов, а очереди, которые уже заполняются в это время строками из файлов. Потому что строки сейчас могут идти из файлов, а завтра будут приходить по сети.

zloymih
написать приложение, которое вычисляет некоторый хэш с помощью некоторой функции от 100к соли и делает распределение полученного результата (т.е. полученные 100к хэшей) по заданному файлу (те делается конкатенация строки из файла и хэша строка::хэш).

Спроектируй сначала это всё, опиши, что должен делать каждый поток и зачем он нужен.
Например, вычисления хеша могут быть достаточно сложными, поэтому хорошо, если это будет делать отдельный поток всё время, принимая и выдавая данные.
Чтение файла тоже занимает какое-то время, поэтому пусть читает отдельный поток, выдавая по строке.
Запись в файл тоже занимает какое-то время, поэтому пусть пишет отдельный поток, записывая по строке.
И дальше ты их просто соединяешь вместе через очереди обмена.

Большое спасибо за хороший анализ.
Появились еще вопросы:
А то, что я все оформил в виде одного класса это правильно? Пишут так или нужно разбивать на отдельные классы?
Могут ли возникать проблемы, когда методы одного класса вызываются в разных процессах.
Как правильно (общеприянто) обрабатывать ошибки в подпроцессе и корректно его завершать?
Легко ли поддерживать такой код, как у меня?

zloymih
А то, что я все оформил в виде одного класса это правильно?

Класс нужно делать, когда у тебя есть одни данные, с которыми работают несколько функций.
Например, в питоне есть список - list. Это класс, который состоит из данных и методов для работы с ними. Методы +, append(), extend(), pop(), len и другие. Данные ты не видишь, вся работа с ними происходит через методы.
Когда у тебя есть этот класс, ты можешь создать несколько списков, в каждом из которых свои данные.
Это называется информационно-прочным модулем в классификации Майерса. При этом каждый метод является функционально-прочным модулем, то есть не делает ничего, кроме своей задачи.
Можно ещё наследование рассмотреть, но наследование не такое важное, так как при наследовании точно так же можно получать спагетти.

zloymih
Пишут так или нужно разбивать на отдельные классы?

Если делаешь новый класс, должен задаваться вопросом “что будет делать этот класс, для чего он?”. Вот потренируйся на списке list, как будто его нет и ты должен его сделать.

zloymih
Могут ли возникать проблемы, когда методы одного класса вызываются в разных процессах.

Методы могут быть чистыми, а могут быть с побочными эффектами. Но даже чистые методы могут работать с одними данными, одновременный доступ к которым может приводить к ошибкам. Это тонкая тема, нужно хорошо понимать, что делают процессы и чего не делают.

zloymih
Как правильно (общеприянто) обрабатывать ошибки в подпроцессе и корректно его завершать?

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

zloymih
Легко ли поддерживать такой код, как у меня?

Ты сам откроешь его через год или два и поймёшь. Поймёшь, что написана бурда, которую можно только заново переписать. А времени на это не будет. Вот так же он выглядит для незнакомого человека.
Главная проблема там, если приводить аналогию с машиной, в том, что ты колёса связываешь с рулём, а двигатель - с бампером. Не надо их связывать, это должны быть независимые друг от друга вещи.
Вот наскоряк замени файл на диске на сетевой сокет…

Отредактировано py.user.next (Фев. 18, 2016 12:54:02)

Благодарю за столь подробный ответ. Постараюсь переписать код с учетом вышесказанного.

В процессе написания возникли затруднения в синхронизации потоков и способами обмена информацией между ними.

Предполагаю, что для решения первой проблемы нужно использовать threading.Event().
А для решения второй проблемы ничего кроме Queue() не придумал. Есть сомнения насчет передачи одной строки из файла через очередь. Мне кажется это накладно. Загружать все строки в очередь - еще хуже. Как правильно в таком случае: использовать глобальную переменную или другой способ? Мой код с учетом замечаний во вложении.

Буду рад замечаниям и критике. Стало лучше или вообще лапша получилась?

Прикреплённый файлы:
masterfoo_v4.py (4,5 KБ)

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

zloymih
В процессе написания возникли затруднения в синхронизации потоков

Когда делаешь потоки, нужно представлять, что запускаешь их в порядке 1 2 3 4 5, а работать они будут в порядке 5 4 3 2 1. Блокировками управляют очереди. Если в очереди есть элемент, то он может читаться. Если в очереди нет элемента, то он ожидается. Если все элементы переданы, то очередь закрывается.

zloymih
использовать глобальную переменную или другой способ?

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

zloymih
Стало лучше или вообще лапша получилась?

Лапша стала лучше. Приличнее всего выглядит функция main(). Но в ней должны открываться файлы.
Ты должен один раз написать и забыть. А ты по 500 раз переписываешь.

py.user.next
Ты должен один раз написать и забыть. А ты по 500 раз переписываешь.

Я учусь. Если бы я умел, то вопросов бы не задавал, а помогал бы другим, как вы. То, что для вас кажется очевидным, для меня кажется запутанным. Спасибо вам за помощь.

py.user.next
Ты сначала напиши, чтобы это хотя бы работало, а оптимизировать будешь потом.

Сделал все через очереди. Программа работает. Но получился монстр. Поддерживать такое сложно. Можно код улучшить?

Прикреплённый файлы:
masterfoo_v5.py (5,7 KБ)

zloymih
Сделал все через очереди. Программа работает.

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

У записывальщика три очереди. Зачем? Он должен получать только строки, уже готовые для записи. Из-за лишней очереди хешей ты его усложнил. А надо упростить. Он должен быть не сложнее читальщика.

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

Отредактировано py.user.next (Фев. 21, 2016 06:36:01)