DjangoBB LoFi version

Начало » Python для новичков » Оцените код новичка

1 2 3 4

CHAOS

Ноя. 19, 2011 18:59:29

Бодрого времени всем! Появилось в наличии страшное желание освоить Python. Пара дней работы (знакомства) и появилось немного кода. Хотелось бы услышать мнение более маститых товарищей. Код в студию:

from optparse import OptionParser
from re import search
from urllib import urlopen
from urlparse import urlparse
from subprocess import call
from os import devnull


def get_options():
    """Get command-line options"""
    parser = OptionParser()
    parser.add_option('-l', '--links')
    parser.add_option('-d', '--dir', default='/media/sda5/tmp/images')
    (options, args) = parser.parse_args()
    return options


def main():
    """The main function

    This function uses a string of HTML tags (<a>text</a> <a><img /></a>)
    as --links option. aria2 is a default downloader. And don't forget to
    change the download directory (--dir).

    """

    options = get_options()

    if options.links:
        links = options.links.split('</a> <a')
        total = len(links)
        dnull = open(devnull, 'w')
        strings = [
            '\n{0} from {1} ({2}) \nGetting the link:',
           'OK ({0})\nDownloading the image:',
           '-d {0}',
           'OK\n',
           'ERROR\n',
           'http://{0}/{1}',
            'Error opening the web page, try again? (y or n)',
        ]

        if total == 0:
            print 'total == 0'
            return 0

        for (key, link) in enumerate(links):
            matches = search('href="(.+?)"', link)
            uri = matches.group(1)
            print strings[0].format(key + 1, total, uri),
            uopen = urlopen(uri)
            content = ''
            uri_parsed = urlparse(uri)
            try_again = True

            while content == '' and try_again:
                content = uopen.read()

                if content == '':
                    key = raw_input(strings[6])

                    if key == 'y':
                        try_again = True
                    else:
                        try_again = False

            if 'imageshack' in uri_parsed.netloc:
                matches = search('<link rel="image_src" href="(.+?)"', content)
                uri = matches.group(1)
            elif 'imagevenue' in uri_parsed.netloc:
                matches = search('SRC="(.+?)"', content)
                uri = strings[5].format(uri_parsed.netloc, matches.group(1))

            print strings[1].format(uri),
            return_code = call(['aria2c', uri, strings[2].format(options.dir)],
                               stdout=dnull)

            if return_code == 0:
                print strings[3],
            else:
                print strings[4],

    return 0


if __name__ == '__main__':
    main()

Ноя. 19, 2011 21:03:37

А чего оно делает?

Ноя. 19, 2011 21:23:12

В общем я усилием мозга таки осилил назначение этого.
Начнемс:
1. Уберите strings. Из-за наличия этого списка код нечитаем.
2. Комментарий к main тоже непонятен. Напишите что оно делает.
3. Вместо try_again и key используйте одну переменную try_again с начальным значением ‘y’. Цикл будет выглядеть как while content == ‘' and try_again == ’y', а выкрутасов с проверкой ввода не будет.
4. if return_code == 0 не pythonic. Используйте просто if return_code.
5. Для импортируемых сущностей, которые используются в коде один раз я бы в начале импортировал только модуль, а в коде указывал модуль.сущность. По-моему так читается проще.
6. Если aria2c просто даунлоадит, то зачем он вам нужен? У вас же есть urllib.

CHAOS

Ноя. 20, 2011 12:02:16

Спасибо, Ed, за комментарии, за наводки. Сейчас пойду чинить, но сперва хотел спросить. Пункт 5. Я импортировал сущности, а не модули только для экономии ресурсов, хотя читабельность от этого упала, это верно. Вопрос лишь в том, есть ли в такой экономии вообще смысл?

Ноя. 20, 2011 13:16:31

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

С точки зрения читабельности я бы предпочел re.search просто search-у, поскольку оно места больше не занимает, но гораздо понятнее читается. subprocess.call тоже не завредит. os.devnull из той же оперы. А вот OptionParser, urlopen и urlparse я бы оставил как есть, поскольку они менее generic, а urlparse.urlparse выглядит длиннее, но читабельности не прибавляет, даже скорее раздражает, смотрится как масло масляное.

Если хотите разобраться как работают импорты в Python, то могу порекомендовать статьи Андрея Светлова на эту тему: http://asvetlov.blogspot.com/2010/05/blog-post.html

CHAOS

Ноя. 21, 2011 12:16:03

Воспользовался Вашими советами, Ed.

strings переименовал в i18n, чтобы было понятнее, и убрал из основной функции. Лучше всего, я думаю, строки убрать в отдельный файл, но в рамках данного скрипта это бессмысленно.

Добавил комментарии, куда только смог, хотя мой английский - это хромая лошадь.

Убрал aria2c. Действительно, зачем она там была нужна?

Спасибо еще раз, Ed, за конструктивную критику. Благодаря доработкам код стал проще для понимания, меньше, уже, избавился от лишнего веса:) Обновленный код:

import re
import os
from urllib import urlopen, urlretrieve
from optparse import OptionParser
from urlparse import urlparse

i18n = [
    '\n{0} from {1} ({2})\nGetting the page...',
    'Downloading an image...',
    'OK',
    'ERROR. Try again? (y or n)',
    'http://{0}/{1}',
    'URI is oblivious',
    'ERROR',
]


def get_options():
    """Get command-line options."""
    parser = OptionParser()
    parser.add_option('-l', '--links')
    parser.add_option('-d', '--dir', default='/media/sda5/tmp/images')
    (options, args) = parser.parse_args()
    return options


def main():
    """This function gets URIs from href attributes of the --links option,
    which must be a basic HTML code. Loading pages by URIs.
    Finds links to images and downloading them.

    """

    options = get_options()

    if options.links is None:
        return 1

    matches = re.findall('href="(.+?)"', options.links)
    total = len(matches)

    if total == 0:
        return 1

    if options.dir[-1] != '/':
        options.dir = ''.join((options.dir, '/'))

    for (key, uri) in enumerate(matches):
        print i18n[0].format(key + 1, total, uri)
        uri_parsed = urlparse(uri)
        try_again = 'y'

        try:
            uopen = urlopen(uri)
        except IOError:
            print i18n[5]
            continue

        while try_again == 'y':
            uopen = urlopen(uri)
            content = uopen.read()
            uopen.close()

            if content == '':
                try_again = raw_input(i18n[3])
            else:
                break

        if 'imageshack' in uri_parsed.netloc:
            matches = re.search('<link rel="image_src" href="(.+?)"',
                                content)
            uri = matches.group(1)
        elif 'imagevenue' in uri_parsed.netloc:
            matches = re.search('SRC="(.+?)"', content)
            uri = i18n[4].format(uri_parsed.netloc, matches.group(1))

        print i18n[1].format(uri)
        uri_parsed = urlparse(uri)
        file_name = uri_parsed.path.split('/')
        (down_name, headers) = urlretrieve(uri,
            ''.join((options.dir, file_name[-1])))

        if os.path.isfile(down_name):
            print i18n[2]
        else:
            print i18n[6]

    return 0


if __name__ == '__main__':
    main()

Ноя. 21, 2011 20:22:58

Продолжим.
1. Я имел в виду не переименование strings, а убирание этого списка, как класса.
Как по-вашему что лучше читается:
print i18n.format(key + 1, total, uri) или print “\n{0} from {1} ({2})\nGetting the page…”.format(key + 1, total, uri) ?

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

2. Если args вам не нужен, то можно делать так: (options, _args) = parser.parse_args()
Тогда pylint не будет ругаться на неиспользованную переменную.

3. default для -d какой-то странный. Я бы сделал просто текущий каталог. Это обычный ожидаемый эффект для скриптов такого типа. Для того же, чтобы найти куда ваш скрипт положил результат нужно будет смотреть в сорцы, что не гуд.

4. Вместо -l я бы попользовался args. Ваша строка - это не опция, а аргумент.

5. Зачем передавать строку в виде <a href=“bla-bla”>, а потом регекспами выдирать это bla-bla? проще просто передать туда “bla-bla”, тогда и пользоваться будет проще и регекспы не нужны.

6.

    total = len(matches)

    if total == 0:
        return 1

замените на

if not matches:
    return 1

Это не Си, а Python.

7. try_again = ‘y’ поставьте прямо перед циклом, где он используется.

8.

if content == '':
    try_again = raw_input(i18n[3])
else:
    break

замените на

if not content:
    try_again = raw_input("ERROR. Try again? (y or n)")

9. Вместо ''.join((options.dir, file_name))) используйте os.path.join, оно правильно делает пути в зависимости от OS, на которой запускается.

Пока достаточно. Исправите - приходите еще :) И не забудьте проверять pylint-ом.

CHAOS

Ноя. 26, 2011 18:25:12

Доброго времени, Ed!

По поводу первого пункта. Строки вынесены в массив для интернационализации. Если же их вставить непосредственно в код, то как тогда быть с переводом?

И восьмой пункт. Если мы уберем else, то получим бесконечный цикл. Правильно ли я все понял?

CHAOS

Дек. 6, 2011 18:28:49

Здравствуйте, все!

С интернационализацией все на самом деле просто - надо использовать gettext. По поводу пятого пункта. Эта программа должна работать в паре с Linkshop - онлайновым старшим братом, который на выходе дает HTML-строку. Остальные претензии вроде тоже учтены. Код:

# -*- coding: utf-8 -*-

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

Также с ее помощью можно загружать материалы с imageshack.us и по прямым
ссылкам. Пример:

linkshop.py -l '<a href="imagevenue.com"></a><a href...' -d /ваш-каталог

"""

import re
import os
from urllib import urlopen, urlretrieve
from optparse import OptionParser
from urlparse import urlparse


def get_options():
    """Получает опции и аргументы командной строки."""
    parser = OptionParser()
    parser.add_option('-d', '--dir', default='')
    return parser.parse_args()


def main():
    """Функция получает адреса страниц из href-атрибутов первого аргумента,
    который должен быть обычным HTML-кодом. Используя адреса,
    загружает страницы, находит там картинки и их тоже загружает.

    """

    (options, args) = get_options()

    if not args:
        return 1

    matches = re.findall('href="(.+?)"', args[0])
    total = len(matches)

    if not matches:
        return 1

    if options.dir and options.dir[-1] != '/':
        options.dir = os.path.join(options.dir, '/')

    for (key, url) in enumerate(matches):
        string = '\n{0} из {1} ({2})\nПолучение страницы...'
        print string.format(key + 1, total, url)
        url_parsed = urlparse(url)

        try:
            url_open = urlopen(url)
        except IOError:
            print 'Неправильная ссылка'
            continue

        try_again = 'y'

        while try_again == 'y':
            url_open = urlopen(url)
            content = url_open.read()
            url_open.close()

            if not content:
                try_again = raw_input('ERROR. Попробовать еще раз? (y or n)')
            else:
                break

        if 'imageshack' in url_parsed.netloc:
            matches = re.search('<link rel="image_src" href="(.+?)"',
                                content)
            url = matches.group(1)
        elif 'imagevenue' in url_parsed.netloc:
            matches = re.search('SRC="(.+?)"', content)
            url = 'http://{0}/{1}'.format(url_parsed.netloc, matches.group(1))

        print 'Загрузка картинки...'
        url_parsed = urlparse(url)
        file_name = url_parsed.path.split('/')
        (down_name, _headers) = urlretrieve(url,
            os.path.join(options.dir, file_name[-1]))

        if os.path.isfile(down_name):
            print 'OK'
        else:
            print 'ERROR'

    return 0


if __name__ == '__main__':
    main()

Дек. 10, 2011 16:27:21

Вы как-то надолго исчезаете. Я думал уже все, кончилась ваша терпелка. Ан нет :)

1. Тихий выход в случае отсутствия аргументов или совпадений - это не очень юзер-френдли. У OptionParser-а есть вроде usage, error, help и т.д. Вполне уместно будет их попользовать.
2. total = len(matches) не нужен. достаточно будет print string.format(key + 1, len(matches), url)
3. Непонятно назначение этого:

   if options.dir and options.dir[-1] != '/':
        options.dir = os.path.join(options.dir, '/')

по-моему и без него все отлично работает.
4. file_name = url_parsed.path.split('/') по-моему читается лучше, чем у вас в коде.
5. (down_name, _headers) - лучше без скобок.
6. Много длинных строк. PEP8 рекомендует ограничиться 79ю символами и это правильно.
7. Ошибки лучше выводить на stderr, а не на stdout.
8. url_parsed = urlparse(url) стоит далеко от использования, а url_parsed.netloc юзается три раза. Я бы сделал netloc = urlparse(url).netloc.
Исправите - приходите еще.