Уведомления

Группа в Telegram: @pythonsu

#1 Май 18, 2018 07:31:30

Vladimirv
Зарегистрирован: 2013-03-22
Сообщения: 108
Репутация: +  7  -
Профиль   Отправить e-mail  

Говнокод. Научите изяществу

irgit
 URL = {}
URL["vk_getCities"] = "https://api.vk.com/method/database.getCities.json?"
URL["country_id"] = "country_id=1"
URL["region_id"] = "region_id=1157049"
URL["count"] = "count=1000"
URL["token"] = "access_token=blabla"
URL["version_vkapi"]="v=5.52"
заменить на
 URL = {
    "vk_getCities"  : "https://api.vk.com/method/database.getCities.json?",
    "country_id"    : "country_id=1",
    "region_id"     : "region_id=1157049",
    "count"         : "count=1000",
    "token"         : "access_token=blabla",
    "version_vkapi" : "v=5.52",
}

Отредактировано Vladimirv (Май 18, 2018 07:32:01)

Офлайн

#2 Май 18, 2018 07:40:54

irgit
Зарегистрирован: 2018-02-04
Сообщения: 30
Репутация: +  0  -
Профиль   Отправить e-mail  

Говнокод. Научите изяществу

Vladimirv
URL = { “vk_getCities” : "https://api.vk.com/method/database.getCities.json?“, ”country_id“ : ”country_id=1“, ”region_id“ : ”region_id=1157049“, ”count“ : ”count=1000“, ”token“ : ”access_token=blabla“, ”version_vkapi“ : ”v=5.52", }

Елки-палки - это круто! Спасибо большое!

Офлайн

#3 Май 18, 2018 07:44:15

Vladimirv
Зарегистрирован: 2013-03-22
Сообщения: 108
Репутация: +  7  -
Профиль   Отправить e-mail  

Говнокод. Научите изяществу

и тут

irgit
 for sgroup in datagroup:
            idg = (sgroup["id"])
            nmg = (sgroup["name"])
            icg = (sgroup["is_closed"])
            tyg = (sgroup["type"])
            phg = (sgroup["photo_200"])
            cig = (sgroup["city"])
            flg = (sgroup["follower"])
            urg = (sgroup["url"])
            writer.writerow({'id': idg, 'name': nmg, 'is_closed': icg, 'type': tyg, 'photo_200': phg, 'city': cig, 'follower': flg, 'url': urg} )
на
 for sgroup in datagroup:
    writer.writerow({
        'id'        : sgroup["id"], 
        'name'      : sgroup["name"], 
        'is_closed' : sgroup["is_closed"], 
        'type'      : sgroup["type"], 
        'photo_200' : sgroup["photo_200"], 
        'city'      : sgroup["city"], 
        'follower'  : sgroup["follower"], 
        'url'       : sgroup["url"],
    })

Офлайн

#4 Июнь 10, 2019 07:32:04

irgit
Зарегистрирован: 2018-02-04
Сообщения: 30
Репутация: +  0  -
Профиль   Отправить e-mail  

Говнокод. Научите изяществу

Уважаемые эксперты, оцените пожалуйста ‘изящество’ кода. Как бы Вы написали его фрагменты? Какие очевидные ошибки присутствуют?

 import requests
from bs4 import BeautifulSoup as bs
from datetime import date
from dateutil.rrule import rrule, DAILY
import re
from multiprocessing.dummy import Pool as ThreadPool
def clear(text):
    return re.sub( r'\s+', ' ', str(text))
def split(arr, count):
    '''Делим ссылки на части для мультипотока'''
    return [arr[i::count] for i in range(count)]
def all_url():
    '''генерируем ссылки за указанный период
    Поменяйте в ссылке регион'''
    a = date(2013, 1, 1) #Начало периода
    b = date(2019, 4, 10) #конец периода
    url_all = []
    for dt in rrule(DAILY, dtstart=a, until=b):
        date_select = (dt.strftime("%d.%m.%Y"))
        url = 'http://www.zakupki.gov.ru/epz/order/extendedsearch/results.html?morphology=on&openMode=USE_DEFAULT_PARAMS&pageNumber=1&sortDirection=false&recordsPerPage=_100&showLotsInfoHidden=false&fz44=on&fz223=on&ppRf615=on&fz94=on&af=on&ca=on&pc=on&pa=on&currencyIdGeneral=-1&publishDateFrom='+date_select+'&publishDateTo='+date_select+'&region_regions_5277386=region_regions_5277386&regions=5277386&regionDeleted=false&sortBy=UPDATE_DATE'
        url_all.append(url)
    return url_all
def get_page(uri):
    '''Парсинг страницы'''
    headers = {'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36'}
    response = requests.get(uri, headers=headers ).text
    soup = bs(response, 'html.parser')
    tag = soup.findAll( "div", {"class": "registerBox"} )
    zakup_all = []
    for i in tag:
        try:
            one_columne = i.find('td')
            text_type = one_columne.find('strong').text
            text_fz = one_columne.find('span', {"class": "orange"}).text
            text_rub = (one_columne.find( 'span', {"class": "fractionNumber"}).parent).text
            text_rub = re.sub(r' ', '', clear(text_rub))
            two_columne = i.find('td', {"class": "descriptTenderTd"})
            text_num = two_columne.find('dt').text
            text_num = (clear(text_num))
            text_num_url = two_columne.find('dt')
            text_num_url = 'http://www.zakupki.gov.ru' + text_num_url.find("a").get('href')
            text_org = two_columne.find('li').text
            text_org_url = 'http://www.zakupki.gov.ru'+(two_columne.find('li').find("a").get('href'))
            text_main = two_columne.findAll( 'dd' )[1].text
            tree_columne = i.find('td', {"class": "amountTenderTd"})
            text_date = (tree_columne.findAll('li')[0].text)[11:]
            text_update = (tree_columne.findAll( 'li' )[1].text)[11:]
            text_type = (clear(text_type))
            text_org = (clear(text_org))
            text_main = (clear(text_main))
            zakup = (text_type, text_fz,text_rub, text_num, text_org,
                     text_main, text_date, text_update, text_num_url)
            zakup_all.append(zakup)
        except:
            z=0
    return zakup_all
'''Парсим мультипотоком и сохраняем'''
url_count = len(all_url())
pagi = round(url_count / 5)
pool = ThreadPool(5)
page_count = []
with open( 'zakup.csv', 'a') as f:
    for part in (split(all_url(), pagi)): #458
        page_count.append(len(part))
        print ('Страниц обработано: ' + str(sum(page_count)) + ' из ' + str(url_count))
        try:
            r = pool.map(get_page, part)
            for i in r:
                if bool(i) == True:
                    for item in i:
                        stroka = item[0] + '|' + item[1] + '|' + item[2] + '|' \
                                 + item[3] + '|' + item[4] + '|' + item[5] + '|'\
                                 + item[6] + '|' + item[7] + '|' + item[8] + '|'
                        f.write(str(stroka) + '\n')
        except:
            print( "Ошибка" )
print ('ок')

Офлайн

#5 Июнь 11, 2019 00:05:54

rami
Зарегистрирован: 2018-01-08
Сообщения: 280
Репутация: +  71  -
Профиль   Отправить e-mail  

Говнокод. Научите изяществу

Очевидных ошибок вроде не заметно, код работает, но говнокода навалом.

Предлагать на форуме скачать аукционы за шесть с лишним лет это мягко говоря беспредел:

 def all_url():
    '''генерируем ссылки за указанный период
    Поменяйте в ссылке регион'''
    a = date(2013, 1, 1) #Начало периода
    b = date(2019, 4, 10) #конец периода
достаточно установить даты с 1 по 11 июня текущего года или даже меньше.


Год назад в этой теме я предлагал вам использовать форматирование строк, а vic57 предлагал использовать метод .join(), но вы по прежнему пишете безобразие:
                         stroka = item[0] + '|' + item[1] + '|' + item[2] + '|' \
                                 + item[3] + '|' + item[4] + '|' + item[5] + '|'\
                                 + item[6] + '|' + item[7] + '|' + item[8] + '|'
вместо простого:
 stroka = '|'.join(item)

Функция str() нигде не нужна, если использовать форматирование строк, а не сложение.

Много ещё чего по мелочам.

Офлайн

#6 Июнь 11, 2019 06:40:12

irgit
Зарегистрирован: 2018-02-04
Сообщения: 30
Репутация: +  0  -
Профиль   Отправить e-mail  

Говнокод. Научите изяществу

rami
Очевидных ошибок вроде не заметно, код работает, но говнокода навалом.Предлагать на форуме скачать аукционы за шесть с лишним лет это мягко говоря беспредел:
Очень прошу указать на мелочи. Самому мне их сложно заметить из-за отсутствия необходимого опыта)

Офлайн

#7 Июнь 11, 2019 09:53:38

PEHDOM
Зарегистрирован: 2016-11-28
Сообщения: 2196
Репутация: +  294  -
Профиль   Отправить e-mail  

Говнокод. Научите изяществу

irgit
Очень прошу указать на мелочи. Самому мне их сложно заметить из-за отсутствия необходимого опыта)
ну смотрите конкатенация строк это некошерно, пользуйтесь форматом:
 # Bad:
print ('Страниц обработано: ' + str(sum(page_count)) + ' из ' + str(url_count))
# Good :
print ('Страниц обработано: {} из {}'.format(sum(page_count), url_count))
лишнее переобразование это плохо
 # Пустой список и так вернет False, непустой True
#Плохо
if bool(i) == True:
    ....
# Хорошо:
if i :
   ....


подавлять все ошибки крайне не рекомендуется
 # Плохо:
try:
   .....
except: print( "Ошибка" )
что за ошибка, откуда ошибка? что я должен делать чтобы ошибка исчезла?
 # Хорошо:
try:
   .....
except IOError as e: 
    print( "Ошибка записи в файл " )
    print(e) 

Имена файлов лучше заводить в виде переменных или получать извне
 # Плохо:
with open( 'zakup.csv', 'a') as f:
    ....
# Хорошо:
output_file= 'zakup.csv'
....
with open(output_file, 'a') as f:
    ....
тогда если вам (или комуто еще) вдруг понадобиться изменить имя выходного файла, ему не придется искать и заменять во всем коде, а достаточно будет изменить одну строчку.

опять же вот ваша конструкция:
 try:
            r = pool.map(get_page, part)
            for i in r:
                if bool(i) == True:
                    for item in i:
                        stroka = item[0] + '|' + item[1] + '|' + item[2] + '|' \
                                 + item[3] + '|' + item[4] + '|' + item[5] + '|'\
                                 + item[6] + '|' + item[7] + '|' + item[8] + '|'
                        f.write(str(stroka) + '\n')
        except:
            print( "Ошибка" )
1.я честно говоря плохо понимаю что тут может вызвать исключение, кроме записи в файл. Рекомендуется заключать в try-except только тот код который может вызвать исключение, а не все подряд.
2. неинформативность переменных. что такое r? что такое i?




==============================
Помещайте код в теги:
[code python][/code]
Бериегите свое и чужое время.

Отредактировано PEHDOM (Июнь 11, 2019 09:57:01)

Офлайн

#8 Июнь 11, 2019 12:06:55

irgit
Зарегистрирован: 2018-02-04
Сообщения: 30
Репутация: +  0  -
Профиль   Отправить e-mail  

Говнокод. Научите изяществу

PEHDOM
ну смотрите конкатенация строк это некошерно, пользуйтесь форматом:
огромное спасибо!!!! Такой формат разъяснений мне очень помог заметить и понять ошибки!!!! СУПЕР! Спасибки!!!!!!

Офлайн

#9 Июнь 11, 2019 12:24:15

irgit
Зарегистрирован: 2018-02-04
Сообщения: 30
Репутация: +  0  -
Профиль   Отправить e-mail  

Говнокод. Научите изяществу

Спасибо ребята!!! Теперь уже не совсем говнокод, а что-то отдалёно похожее на ‘приличное’. Ошибки ещё есть, теперь я их вижу. Благодарствую))) код

 import requests
from bs4 import BeautifulSoup as bs
from datetime import date
from dateutil.rrule import rrule, DAILY
import re
from multiprocessing.dummy import Pool as ThreadPool
def clear(text):
    return re.sub( r'\s+', ' ', text)
def split(arr, count):
    '''Делим ссылки на части для мультипотока'''
    return [arr[i::count] for i in range(count)]
def all_url():
    '''генерируем ссылки за указанный период
    Поменяйте в ссылке регион'''
    a = date(2019, 1, 1) #Начало периода
    b = date(2019, 4, 10) #конец периода
    url_all = []
    for dt in rrule(DAILY, dtstart=a, until=b):
        date_select = (dt.strftime("%d.%m.%Y"))
        url = 'http://www.zakupki.gov.ru/epz/order/extendedsearch/results.html?' \
              'morphology=on&openMode=USE_DEFAULT_PARAMS&pageNumber=1&' \
              'sortDirection=false&recordsPerPage=_100&showLotsInfoHidden=' \
              'false&fz44=on&fz223=on&ppRf615=on&fz94=on&af=on&ca=on&pc=on&pa=' \
              'on&currencyIdGeneral=-1&publishDateFrom={}&publishDateTo={}&region_regions_5277386=' \
              'region_regions_5277386&regions=5277386&regionDeleted=' \
              'false&sortBy=UPDATE_DATE'.format(date_select,date_select)
        url_all.append(url)
    return url_all
def get_page(uri):
    '''Парсинг страницы'''
    headers = {'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) '
                             'AppleWebKit/537.36 (KHTML, like Gecko) '
                             'Chrome/71.0.3578.98 Safari/537.36'}
    response = requests.get(uri, headers=headers ).text
    soup = bs(response, 'html.parser')
    tag = soup.findAll( "div", {"class": "registerBox"} )
    zakup_all = []
    for i in tag:
        try:
            one_columne = i.find('td')
            text_type = one_columne.find('strong').text
            text_fz = one_columne.find('span', {"class": "orange"}).text
            text_rub = (one_columne.find( 'span', {"class": "fractionNumber"}).parent).text
            text_rub = re.sub(r' ', '', clear(text_rub))
            two_columne = i.find('td', {"class": "descriptTenderTd"})
            text_num = two_columne.find('dt').text
            text_num = (clear(text_num))
            text_num_url = two_columne.find('dt')
            text_num_url = 'http://www.zakupki.gov.ru' + text_num_url.find("a").get('href')
            text_org = two_columne.find('li').text
            text_org_url = 'http://www.zakupki.gov.ru'+(two_columne.find('li').find("a").get('href'))
            text_main = two_columne.findAll( 'dd' )[1].text
            tree_columne = i.find('td', {"class": "amountTenderTd"})
            text_date = (tree_columne.findAll('li')[0].text)[11:]
            text_update = (tree_columne.findAll( 'li' )[1].text)[11:]
            text_type = (clear(text_type))
            text_org = (clear(text_org))
            text_main = (clear(text_main))
            zakup = (text_type, text_fz,text_rub, text_num, text_org,
                     text_main, text_date, text_update, text_num_url)
            zakup_all.append(zakup)
        except:
            print ('Ошибка извлечения данных')
    return zakup_all
'''Парсим мультипотоком и сохраняем'''
url_count = len(all_url())
pagi = round(url_count / 5)
pool = ThreadPool(5)
page_count = []
record_file = 'zakup.csv'
with open(record_file, 'a') as f:
    for part in (split(all_url(), pagi)): #458
        page_count.append(len(part))
        print( 'Страниц обработано: {} из {}'.format(sum( page_count ), url_count))
        try:
            date_array = pool.map(get_page, part)
            for sep in date_array:
                if sep == True:
                    for item in sep:
                        stroka = '|'.join(item)
                        f.write(stroka + '\n')
        except IOError as e:
            print("Ошибка записи в файл")
            print(e)
print ('Парсинг данных завершён успешно.')

Отредактировано irgit (Июнь 11, 2019 12:30:32)

Офлайн

#10 Июнь 11, 2019 20:16:32

rami
Зарегистрирован: 2018-01-08
Сообщения: 280
Репутация: +  71  -
Профиль   Отправить e-mail  

Говнокод. Научите изяществу

irgit
Очень прошу указать на мелочи.
Мелочей много, даже не знал с чего начать, спасибо, PEHDOM объяснил многое. Но это ещё не всё.

PEHDOM
лишнее переобразование это плохо
# Пустой список и так вернет False, непустой True
#Плохо
if bool(i) == True:
….
# Хорошо:
if i :
В общем случае это правильно, но в данном случае нет надобности проверять содержимое списка, если он пустой, то for ничего с ним не сделает и даже не заметит.


Функция all_url() совсем не нужна, она вызывается у вас два раза, второй раз не понятно зачем. Эту функцию можно заменить одной строкой кода. Начало и конец периода, а так же URL сделать константами и переместить в начало модуля в блок констант под блоком импорта.

Функция split() также не нужна, она вызывается аж целый один раз.

Функция clear() реально нужна всего лишь два раза, к тому же она оставляет пробелы в начале и в конце предложения, которые нужно убирать методом .strip().

Функцию get_page(uri) нужно привести в порядок. Смотрите мой код и если что не ясно, спрашивайте.

Строка кода:
 text_org_url = 'http://www.zakupki.gov.ru'+(two_columne.find('li').find("a").get('href'))
не используется, она нужна?

Нужно использовать контекстный менеджер with:
 with Pool(PROCESSES) as pool:
а не ждать пока “городские власти закроют сезон фонтанов”.

Документация модуля multiprocessing говорит: …необходимо защищать «точку входа» программы следующим образом: if __name__ == ‘__main__’

Сообщение о том, сколько страниц обработано нужно давать не до обработки, а после.

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


Вот мой вариант:
 import requests
from bs4 import BeautifulSoup as bs
from datetime import date
from dateutil.rrule import rrule, DAILY
from multiprocessing import Pool
import re
 
 
#все константы здесь:
PROCESSES = 4    #число процессов
START_DATE = date(2019, 6, 7)  #Начало периода
END_DATE = date(2019, 6, 11)   #конец периода
OUTPUT_FILE = 'zakup.csv'
URL = 'http://www.zakupki.gov.ru/epz/order/extendedsearch/results.html?morphology=on&openMode=USE_DEFAULT_PARAMS&pageNumber=1&sortDirection=false&recordsPerPage=_100&showLotsInfoHidden=false&fz44=on&fz223=on&ppRf615=on&fz94=on&af=on&ca=on&pc=on&pa=on&currencyIdGeneral=-1&publishDateFrom={0}&publishDateTo={0}&region_regions_5277386=region_regions_5277386&regions=5277386&regionDeleted=false&sortBy=UPDATE_DATE'
 
def get_page(uri):
    '''Парсинг страницы'''
    headers = {'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36'}
    response = requests.get(uri, headers=headers).text
    soup = bs(response, 'html.parser')
    tag = soup.findAll("div", {"class": "registerBox"})
    zakup_all = []
    for i in tag:
        try:
            one_columne = i.find('td')
            text_type = one_columne.find('strong').text.strip()
            text_fz = one_columne.find('span', {"class": "orange"}).text
            text_rub = (one_columne.find('span', {"class": "fractionNumber"}).parent).text
            text_rub = re.sub('\s+', '', text_rub)
            two_columne = i.find('td', {"class": "descriptTenderTd"})
            text_num = two_columne.find('dt').text.strip()
            text_num_url = f'http://www.zakupki.gov.ru{two_columne.find("dt").find("a").get("href")}'
            text_org = two_columne.find('li').text.strip()
            text_org_url = f'http://www.zakupki.gov.ru{two_columne.find("li").find("a").get("href")}'  #не используется?
            text_main = two_columne.findAll('dd')[1].text
            text_main = re.sub('\s+', ' ', text_main).strip()
            tree_columne = i.find('td', {"class": "amountTenderTd"})
            text_date = (tree_columne.findAll('li')[0].text)[11:]
            text_update = (tree_columne.findAll('li')[1].text)[11:]
            zakup_all.append((text_type, text_fz, text_rub, text_num, text_org,
                             text_main, text_date, text_update, text_num_url))
        except Exception as e:
            print(e)
    return zakup_all
 
 
'''Парсим мультипотоком и сохраняем'''
if __name__ == '__main__':
    all_url = [URL.format(dt.strftime("%d.%m.%Y")) for dt in rrule(DAILY, dtstart=START_DATE, until=END_DATE)]
    page_count = []
    all_rows = []
 
    with open(OUTPUT_FILE, 'w') as f:
        print('Начали. Ждите завершение...')
        n = len(all_url) // PROCESSES
        for part in [all_url[i::n] for i in range(n)]:
            page_count.append(len(part))
            with Pool(PROCESSES) as pool:
                for row in pool.map(get_page, part):
                    for item in row:
                        all_rows.append('|'.join(item))
            print(f'Страниц обработано: {sum(page_count)} из {len(all_url)}')
        f.write('\n'.join(all_rows))  #Запись в файл одним массивом если данных не очень много
    print('Завершили обработку')

Офлайн

Board footer

Модераторировать

Powered by DjangoBB

Lo-Fi Version