Кто-то плохо думает о моем парсере журнала сервера

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

import re
import numpy as np
from collections import Counter
import sys

def parse_server_log(path_to_file):

    #First check whether it's a legal file name
    try:
        f = open(path_to_file)
    except:
        print "\nInvalid file name and/or path. Please correct!\n"
        return
    # end of sanity check on file

    # The following regexes would extract the concerned values from each line
    # of the file.
    method_regx = re.compile("(method=)+[A-Z]+[\s]+")
    path_regx = re.compile("(path=)+[/\w.\"]+[\s]+")
    dyno_regx = re.compile("(dyno=)+[\w.]+[\s]+")
    connect_regx = re.compile("(connect=)+[\w.]+[\s]+")
    service_regx = re.compile("(service=)+[\w.]+[\s]+")

    # Target values for each urls
    url1 = [0, [], []]
    url2 = [0, [], []]
    url3 = [0, [], []]
    url4 = [0, [], []]
    url5 = [0, [], []]
    url6 = [0, [], []]

    # url matcher regex for each url type
    url1_regex = re.compile("(#)+(/api/users/)+[\d]+(/count_pending_messages)+(#)+")
    url2_regex = re.compile("(#)+(/api/users/)+[\d]+(/get_messages)+(#)+")
    url3_regex = re.compile("(#)+(/api/users/)+[\d]+(/get_friends_progress)+(#)+")
    url4_regex = re.compile("(#)+(/api/users/)+[\d]+(/get_friends_score)+(#)+")
    url5_6_regex = re.compile("(#)+(/api/users/)+[\d]+(#)+")


    with open(path_to_file) as lines:
        for my_data in lines:

            # Now lets separate out the method, path, dyno, connect and service times
            k = method_regx.search(my_data)
            try:            
                line_method = k.group(0).split("=")[1].strip()
            except:
                line_method = ""

            k = path_regx.search(my_data)
            try:
                # The hashes are added at the start and end to make sure the path
                # is not a part of a string rather the entire string!
                line_path = "#" + k.group(0).split("=")[1].strip()  + "#"            
            except:
                line_path = ""

            k = dyno_regx.search(my_data)
            try:            
                line_dyno = k.group(0).split("=")[1].strip()          
            except:
                line_dyno = ""

            k = connect_regx.search(my_data)
            try:            
                line_connect_time = float(k.group(0).split("=")[1].split("ms")[0])            
            except:
                line_connect_time = 0

            k = service_regx.search(my_data)
            try:            
                line_service_time = float(k.group(0).split("=")[1].split("ms")[0])             
            except:
                line_service_time = 0

            # End of getting all the data

            # Now match up the URL and do this under sanity checker
            if(len(line_method) > 0 and len(line_path) > 0):
                url_denoter = 0
                if url1_regex.match(line_path) is not None:
                    url_denoter = 1
                elif url2_regex.match(line_path) is not None:
                    url_denoter = 2
                elif url3_regex.match(line_path) is not None:
                    url_denoter = 3
                elif url4_regex.match(line_path) is not None:
                    url_denoter = 4
                elif url5_6_regex.match(line_path) is not None:
                    url_denoter = 5            



                # OK so now we have the url to which the current url matched
                if(url_denoter==1 and line_method=="GET"):
                    """
                    for GET /api/users/{user_id}/count_pending_messages
                   """
                    url1[0] += 1
                    url1[1].append(line_dyno)
                    url1[2].append(line_connect_time + line_service_time)

                elif(url_denoter==2 and line_method=="GET"):
                    """
                    for GET /api/users/{user_id}/get_messages
                    """
                    url2[0] += 1
                    url2[1].append(line_dyno)
                    url2[2].append(line_connect_time + line_service_time)


    """
    Now print the results!

    """

    # for GET /api/users/{user_id}/count_pending_messages
    print "\n------GET /api/users/{user_id}/count_pending_messages----\n"
    print "Number of times the url is called: ", url1[0]
    if(url1[0]>0):
        my_num_list = url1[2]
        print "Average response time: ", round(np.average(my_num_list), 2), " in ms."
        print "Median response time: ", round(np.median(my_num_list), 2), " in ms."
        print "Mode of response time: ", round(Counter(my_num_list).most_common(1)[0][0], 2), " in ms."
        counts = [(x, url1[1].count(x)) for x in set(url1[1])]
        swap = 0
        tar_dyno = ""
        for count in counts:
            if(count[1]> swap):
                swap = count[1]
                tar_dyno = count[0]

        print "Dyno that responded the most: ", tar_dyno   
    else:
        print "Sorry no parameters can be calculated as the url has not been accessed!"


    print "\n------GET /api/users/{user_id}/get_messages----\n"
    print "Number of times the url is called: ", url2[0]
    if(url2[0]>0):
        my_num_list = url2[2]
        print "Average response time: ", round(np.average(my_num_list), 2), " in ms."
        print "Median response time: ", round(np.median(my_num_list), 2), " in ms."
        print "Mode of response time: ", round(Counter(my_num_list).most_common(1)[0][0], 2), " in ms."
        counts = [(x, url2[1].count(x)) for x in set(url2[1])]
        swap = 0
        tar_dyno = ""
        for count in counts:
            if(count[1]> swap):
                swap = count[1]
                tar_dyno = count[0]

        print "Dyno that responded the most: ", tar_dyno   
    else:
        print "Sorry no parameters can be calculated as the url has not beenaccessed!"


    print "\n------GET /api/users/{user_id}/get_friends_progress----\n"
    print "Number of times the url is called: ", url3[0]


if(__name__=="__main__"):
    parse_server_log(sys.argv[1])
37 голосов | спросил user3001408 4 22014vEurope/Moscow11bEurope/MoscowTue, 04 Nov 2014 14:30:57 +0300 2014, 14:30:57

4 ответа


44

Во-первых, у Python есть руководство по стилю и (если вы не используете " вам дается другое руководство, и в этом случае укажите ссылку на него), вы должны следовать ему. Ваш код обычно следует за ним, но обратите внимание, что import s находятся в неправильном порядке, это должно быть:

from collections import Counter
import re
import sys

import numpy as np

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


Повторение - большая проблема при написании хорошего кода, и это был немедленный красный флаг:

url1 = [0, [], []]
url2 = [0, [], []]
url3 = [0, [], []]
url4 = [0, [], []]
url5 = [0, [], []]
url6 = [0, [], []]

Почему бы не составить список, urls, содержащий эти структуры? Вы можете разрезать это на одну строку, поэтому, если вы измените структуру позже, вы будете делать это только один раз:

urls = [[0, [], []] for _ in range(6)]

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


parse_server_log длиннее много . Вы должны разделить его на более мелкие, автономные функции с разумными параметрами и значениями return, что сделает ваш код намного легче читать, понимать и поддерживать. Каждый должен иметь docstring , объясняющий, что именно он делает. Это также поможет уменьшить повторение.


Bare except - плохая идея - в наименее , вы должны использовать except Exception, но гораздо лучше понять, что ошибки могли , и обрабатывать их явно. См. "Зло из except " .


Вы должны использовать форматирование строк, например.

print "Average response time: ", round(np.average(my_num_list), 2), " in ms."

должен быть

print "Average response time: {0:.2f} in ms.".format(np.average(my_num_list))

Использовать упаковку и распаковку кортежа, например.

for count in counts:
    if(count[1]> swap):
        swap = count[1]
        tar_dyno = count[0]

может быть:

for dyno_, swap_ in counts:
    if swap_ > swap:
        swap, tar_dyno = swap_, dyno_

Проверка файла

try:
    f = open(path_to_file)
except:
    print "\nInvalid file name and/or path. Please correct!\n"
    return

никогда не закрывает файл , который остается открытым через всю функцию. Вы можете проверить, существует ли файл в Python с помощью os ; сделайте это вместо этого. Кроме того, посмотрите этот ответ SO .

ответил jonrsharpe 4 22014vEurope/Moscow11bEurope/MoscowTue, 04 Nov 2014 14:48:08 +0300 2014, 14:48:08
10

+ в regex означает Once or more. Я думаю, что в вашем случае вы ошибаетесь в этом для оператора конкатенации.

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

method_regx = re.compile("(method=)+[A-Z]+[\s]+")
# ...
k = method_regx.search(my_data)
try:
    line_method = k.group(0).split("=")[1].strip()
except:

Может быть переписано

method_regx = re.compile("method=([A-Z]+)[\s]+")
# ...
k = method_regx.search(my_data)
line_method = k.group(1) if k else ''

Так как k будет None, если поиск не работает.

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

line_connect_time = float(k.group(0).split("=")[1].split("ms")[0]) 
ответил njzk2 5 32014vEurope/Moscow11bEurope/MoscowWed, 05 Nov 2014 01:07:30 +0300 2014, 01:07:30
6

Отличные ответы уже, но я хотел бы перейти к еще одному вопросу о обработке файлов, что на самом деле отражает то же самое, что parse_server_log слишком длинный. Рассмотрим первый бит вашего метода:

def parse_server_log(path_to_file):
    : : :
    try:
        f = open(path_to_file)
    except:
        print "\nInvalid file name and/or path. Please correct!\n"
        return
    : : :

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

  • чтение или запись глобальных данных
  • читать или записывать переданные ему аргументы
  • возвращает один объект
  • выполнить исключение

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

Но как потребитель этого кода, как я могу сказать, что произошло? За исключением хаков, таких как замена объекта StringIO для sys.stdout, а затем его разбор, я не могу. Все, что доступно, как пользователь, вызывающий скрипт, я могу прочитать вывод. Это очень ограничивает. Если parse_server_log делает все, что вы хотите, это нормально. Это качественный одноразовый скрипт. И он может быть изменен для обработки новых потребностей, которые возникают.

Но может быть более полезно отделить свои проблемы. parse_server_log похоже, что он должен вернуть структуру данных, которая представляет содержимое журнала. Другая функция может обрабатывать его, а третий может суммировать его с помощью операторов печати. Если бы он был структурирован таким образом, вам нужно было бы знать, удалось ли выполнить parse_server_log или не удалось. Это может быть связано с изменением того, что он возвращает, или может быть путем исключения исключения.

Все это очень длинный изложенный способ сказать это: подумайте о том, чтобы позволить open(path_to_file) выбросить исключение, не поймав его. Затем клиент parse_server_log может видеть это исключение и выбирать, как его обрабатывать. В этом случае Python просто выгрузит эту информацию на экран, но даже это может сказать вам больше, чем сообщение, которое вы выбрали для печати на своем месте.

ответил Michael Urman 5 32014vEurope/Moscow11bEurope/MoscowWed, 05 Nov 2014 16:24:33 +0300 2014, 16:24:33
4

Так же, как небольшая (но слишком большая для комментария) сноска к другим ответам, такая же удобная, как и numpy, это может быть своего рода излишество только для вычисления средней и медианы списка. Вместо этого вы можете легко сделать это сами:

from math import fsum
a = sorted(my_num_list)
n = len(a)
average = fsum(a) / n
median = a[n/2]

или, если вы хотите получить фантазию для списков четной длины (где медиана не всегда однозначно определена):

median = float(a[(n-1)/2] + a[n/2]) / 2

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

ответил Ilmari Karonen 5 32014vEurope/Moscow11bEurope/MoscowWed, 05 Nov 2014 22:50:56 +0300 2014, 22:50:56

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132