@Pompeius_Magnus
Студент

Что не так с кодом?

Всем привет!
Написал на работе парсилку для логов.
Указываете, на каких серверах, где логи, что в них искать, оно по sftp выкачивает логи, и ищет.
Для работы нужен paramiko.
Вопрос в том, нормальный ли код? или говно полное
https://github.com/aikrasnov/simple_parser
  • Вопрос задан
  • 400 просмотров
Решения вопроса 4
@CobaltTheTerrible
Копипастю код на Python
Ниже замечания, вопросы возникшие после того, как я пробежал за пять минут глазами выложенное по ссылке.

Не про код:
1. Не пользуетесь .gitignore. У вас в репозитарии есть README.md~
2. В самом README.md стоит пользоваться разметкой markdown
3. Что за коммит с мессаджем all?

Про код:
4. Как уже отмечали: большая простыня без разбиения на функции, которую зачем-то запихнули в функцию main.
5. Код не следует PEP8 (поставьте себе какой-нибудь чекер и проверяйте код). В частности очень много длинных строк, которые читать очень тяжело, лучше бы код не в main засовывали, добавляя лишние 4 пробела на каждую строку, а обошлись вообще без функций.
6. Комментариев про логику обработки файлов нет. Возможно, конечно, логика простая, но мне вникать, разбирая код, стало лениво.
7. Код ниже может выкинуть эксепшен, если кто-то неправильно сконфигурирует скрипт. Почему не обрабатываете этот экспшен, хотя исключения paramiko ловите?
list_with_path_to_file = DICT_WITH_SERVER[ip]
Проверяйте что наконфигурировали в вашем скрипте. Гарантированно же при таком количестве настроек будут опечатки, забытые запятые и кавычки.
8. Почему в одном случае вызов getcwd обёрнут в str, а вдругом нет? Это стоит пояснить комментарием, если str действительно нужен (в чём я сомневаюсь :))
path_local = str(os.getcwd()) + '/logs/' + name_logfiles
path_to_logs_parser = os.getcwd() + '/logs/' + name_parser_logfiles

9. Раз уж импортировали модуль os, то почему бы не пользоваться os.posixpath.basename вместо
name_logfiles = file_path.split('/')
name_logfiles = str(name_logfiles[-1:])...

10. Аналогичное замечание про os.posixpath.join
11. Используйте string.format. Вместо
print ('\nAll done! I will sleep next '+ str(TIME_FOR_SLEEP) +'  seconds' + '\n')

print ('\nAll done! I will sleep next {} seconds\n'.format(TIME_FOR_SLEEP))

12. Если так хочется if-ы запихнуть в одну-две строки, то пользуйтесь conditional expressions.
if i - context < 0: j = 0
else: j = i - context

пребразуется в
j = i - context if i - context >= 0 else j = 0
# а то и вовсе пишите
j = max(i-context, 0)

13. Зачем вы сравниваете с False постоянно ваши переменные found_error и first_iter? Что мешает написать сразу if no(first_iter)? Зачем вы пишете if found_error == False: print '1' else: print '2' вместо более простого if found_error: print '2' else: print '1' ?
14. Пользуйтесь контекстными менеджерами, list comprehensions. В куске кода ниже комментарий не нужен. Вы просто дублируете код.
logfile = open(path_local, 'r')  # open file for read
for line in logfile:
    logfile_list_old.append(line)
 logfile.close()

Сравните с
with open(path_local, 'r') as f:
    logfile_list_old.extend(f)


Дальше уже стало лень писать. Если вкратце, то код очень плохой. Он отвратительно структурирован и никак не документирован. В довершение ко всему, он еще и не совсем pythonic.
Ответ написан
@lega
1) Разбейте на функции
2) Выбросте py 2.x, используйте py 3.4+
3) Используйте авторизацию по ключам, а не по паролю (это удобней и безопасней).
4) Не знаю на сколько удобен paramiko, но это все можно просто сделать через subprocess если у вас linux.

Так же, не знаю что вы ищите там в логах, но (скорее всего) это можно сделать "парой" bash команд (как минимум они могут помочь).
Например этот код выдает мне все строки со словом "error" в лог файле удаленного сервера без запроса пароля, при этом не гоняя файл по сети:
$ ssh server "cat /var/log/nginx/alight.access.log.1 | grep error"
189.217.26.81 - - [24/Sep/2015:18:53:25 -0400] "GET /*~1*/foca.aspx?aspxerrorpath=/ HTTP/1.1" 404 168 "-" "FOCA"
189.217.26.81 - - [24/Sep/2015:18:53:25 -0400] "GET /*F0C4~1*/foca.aspx?aspxerrorpath=/ HTTP/1.1" 404 168 "-" "FOCA"
Ответ написан
@dmtrrr
Backend developer
Код плохой. Идея выкачивать логи тоже. Посмотрите на fabric.
Ответ написан
@VovanZ
Код не очень, на мой взгляд. Почти всё - одной простынёй в main, очень тяжело читать.

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

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

Войдите, чтобы написать ответ

Войти через центр авторизации
Похожие вопросы