LINUX.ORG.RU

Рефакторинг кода

 ,


0

1

Добрый день. Нужна помощь опытных “питонистов” :) Я создал метод, код ниже, который скорее всего можно хорошенько сократить и сделать его более “питонистым”, но я ещё в этом деле неопытен. Могли бы вы глянуть своим опытным взглядом и дать пару подсказок? И ещё, если не затруднит, поправить одно условие, оно закомментировано в коде, 2 дня уже ломаю голову над этим условием, не получается. вот код:

def play(self):
        """Start the WarGame."""
        new_players = self.players[:]   # make list copy
        while len(new_players) != 1:    # start loop until one object remains in list
            self.deck.deal(new_players, per_hand=1)  # hand over to each player one card
            m = new_players[0].total
            n = 0
            for player in new_players:
                print(player)
            """Need this condition but it's not work
            for player in new_players:
            # if the all objects in the list are equal, start the loop over
                if new_players.count(player.total) >= len(new_players):
                    self.play()"""
            # find those who need to kill
            for player in new_players:
                if player.total < m:
                    m = player.total
                    n = new_players.index(player)
            # kill'em all
            for player in new_players:
                if m == player.total:
                    n == new_players.index(player)
                    print("R.I.P.:", player.name)
                    new_players.remove(player)
            # clear copy list
            for player in new_players:
                player.clear()
        print("Win!:", player.name)
        # clear list
        for player in self.players:
            player.clear()
Спасибо.

m = new_players[0].total
n = 0


ты уверен что для этих переменных нельзя найти более осмысленные имена?

MrClon ★★★★★
()
Ответ на: комментарий от MrClon

Та, да. Я тут проморгал этот нюанс. Стыдно, каюсь, но уже не исправить ( m - start_compaire n - index_player

Guguzjaka
() автор топика

Не «питонист», но сразу в глаза бросились бесполезные комментарии.

theNamelessOne ★★★★★
()
Ответ на: комментарий от Guguzjaka

Это если не важен первоначальный порядок игроков. Но, проще заново всё переписать по задаче, чем рефакторить.

vvn_black ★★★★★
()
Ответ на: комментарий от vvn_black

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

Guguzjaka
() автор топика

Это вообще что значило?

n == new_players.index(player)

Как я понял, большая часть цикла занимается тем что ищет игрока с наименьшим количестввом очков и убивает всех у кого не больше этого количества. Тогда два for’а в середине заменяются на что-то типа этого:

min_score = min((player.total for player in players))

for dead_player in (player for player in players if player.total == min_score):
    print("RIP {}".format(dead_player.name))

players = [player for player in players if player.total > min_score]

По вкусу можно использовать filter вместо list comprehension, но в питоне это получается ужасно неуклюже.

slovazap ★★★★★
()
Ответ на: комментарий от slovazap

Это да) мне ещё учиться и учиться. Я только 2 недели как начал, просто хотел посмотреть как бы это выглядело в правильном варианте. Но мне уже пару красивых кусочков показали, примерно то, что я и хотел увидеть. Спасибо за уделённое мне время. Пора ехать, с работы, домой и учить Python ))

Guguzjaka
() автор топика
Ответ на: комментарий от slovazap

Фильтр с неудобной лямбдой не нужен.

Фильтр с лямбдой в питоне сам по себе нинужно.

Цикл в стиле 80x не нужен.

Цикл в стиле 80х в разы лучше двух циклов на одной строке.

Если особо хипстер, то разбей хотя бы на две.

dead_players = (player for player in players if player.total == min_score)
anonymous
()
Ответ на: комментарий от anonymous

Цикл в стиле 80х в разы лучше двух циклов на одной строке.

Только для недоросших до функционального подхода.

То разбей хотя бы на две

Здесь это значительно ухудшит читаемость кода из-за тройного повторения. dead_players = (), for dead_player in dead_players.

slovazap ★★★★★
()
Ответ на: комментарий от slovazap

Только для недоросших до функционального подхода.

Нафига ты пихаешь свой микроскоп функциональный подход в процедуру?

Здесь это значительно ухудшит читаемость кода из-за тройного повторения. dead_players = (), for dead_player in dead_players.

Ты запихал два цикла в одну строку. И даже не в один list comprehension. Тут некуда ухудшать читаемость.

Во вторых напиши for player in dead_players и не мучай людям мозги лишними префиксами.

anonymous
()
Ответ на: комментарий от anonymous

Нафига ты пихаешь свой микроскоп функциональный подход в процедуру?

Бессмысленный вопрос. Во-первых, он не мой. Во-вторых, куда его «пихать» совершенно не важно, он применим везде.

Ты запихал два цикла в одну строку

А сколько машинных инструкций я «запихал» в одну строку, это же подумать страшно! Как-бы весь смысл высокоуровнего программирования - не считать циклы, переменные и инструкции, а описывать поток и преобразования данных, что тут и сделано. Развернуть его в два цикла - это путь назад к недокоду из первого сообщения.

Во вторых напиши for player in dead_players и не мучай людям мозги лишними префиксами

Это потеря контекста в теле цикла, так что нет, не напишу.

slovazap ★★★★★
()
Ответ на: комментарий от slovazap
for dead_player in (player for player in players if player.total == min_score):

...

players = [player for player in players if player.total > min_score]

рефAкторнул на отличненько :-D

P.S. Для питона это типичный код. Дай только осмысленные имена и наверни ООП побольше.

Deleted
()
Ответ на: комментарий от slovazap

Ты запихал два цикла в одну строку

А сколько машинных инструкций я «запихал» в одну строку, это же подумать страшно!

Страшно, но к теме не относится. Твой код тупо отвратителен как жабоскрипт в .min.js.

Это потеря контекста в теле цикла, так что нет, не напишу.

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

anonymous
()
Ответ на: комментарий от anonymous

Страшно, но к теме не относится. Твой код тупо отвратителен как жабоскрипт в .min.js.

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

То есть то, что ты находишься в теле этого самого цикла тебе не на что не намекает?

Не намекает. Я ещё и в main() нахожусь, когда смотрю код на 20-м уровне стека вызовов. Контекст должен быть понятен сразу, без беганья глазами по строкам и тем более файлам. Странно что тебя этому не учили.

slovazap ★★★★★
()
Ответ на: комментарий от slovazap

Господа! Я польщён вашим пристальным вниманием, к моему скромному коду. Я и подумать не мог, что мой код, 2-х недельного самоучки, вызовет столь бурные прения. В любом случае, спасибо вам за внимание и отзывчивость, а также за вдохновение работать в этом направлении и дальше. Если мне кто ещё и объяснит почему не работает это:

for player in new_players:
   if new_players.count(player.total) >= len(new_players):
      self.play()
то вам вообще цены не будет :)

Guguzjaka
() автор топика
Ответ на: комментарий от Guguzjaka

И как же это, по-твоему, должно работать? Почему ты среди игроков ищешь число?

slovazap ★★★★★
()
Последнее исправление: slovazap (всего исправлений: 1)

Свойство total может быть одинаковым для нескольких игроков? Если да, то есть вероятность что все игроки умрут...

while len(new_players)>1: 

maxim2
()
Ответ на: комментарий от maxim2

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

Guguzjaka
() автор топика
Ответ на: комментарий от slovazap

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

Нет.

Не намекает. Я ещё и в main() нахожусь, когда смотрю код на 20-м уровне стека вызовов.

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

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

anonymous
()
Ответ на: комментарий от Guguzjaka

Извините, но интерпретатор считает по другому.

Объясни ему либо мне что один из нас не прав.

anonymous
()
Ответ на: комментарий от maxim2
if len(set([p.total for p in new_players])) == 1:
     continue

Работает супер! Спасибо.

Guguzjaka
() автор топика
Ответ на: комментарий от Guguzjaka
def play(self):
        """Start the WarGame."""
        new_players = self.players[:]   # make list copy
        while len(new_players) != 1:    # start loop until one object remains in list
            self.deck.deal(new_players, per_hand=1)  # hand over to each player one card

            for player in new_players:
                print(player)

            min_score = min(player.total for player in new_players)

            dead_players = [player for player in new_players if player.total <= min_score]
            living_players = [player for player in new_players if player.total > min_score]

            if not living_players:
                # everybody died, play again
                continue

            # kill'em all
            for player in dead_players:
                print("R.I.P.:", player.name)

            new_players = living_players

            # clear copy list
            for player in new_players:
                player.clear()
        print("Win!:", player.name)
        # clear list
        for player in self.players:
            player.clear()
anonymous
()
Ответ на: комментарий от anonymous

Ого! Как всё просто! Премного благодарен!

Guguzjaka
() автор топика
def play(self):
    least = min(map(lambda x: x.total, self.players))
    
    losers = *filter(lambda x: x.total == least, self.players),
    winners = *filter(lambda x: x.total > least, self.players),

    self.players = len(losers) == len(self.players) and losers or winners

    if len(self.players) == 1:
        print(f'Champion: {self.players[0].name}: {self.players[0].total}')
        return

    self.deck.deal(self.players, per_hand=1)
    self.play()
vvn_black ★★★★★
()
Ответ на: комментарий от vvn_black

Или даже так, больше «функциональщины»:

    ...
    def hands(self, players):
        # "сдаём" в players
        players = self.deck.deal(players, per_hand=1)

        least = min(map(lambda x: x.total, players))

        losers = *filter(lambda x: x.total == least, players),
        winners = *filter(lambda x: x.total > least, players),

        players = len(losers) == len(players) and losers or winners

        return len(players) == 1 and players or self.hands(players)

    def play(self):
        w = self.hands(self.players)
        print(f'Champion: {w[0].name} with {w[0].total} points')
vvn_black ★★★★★
()
Последнее исправление: vvn_black (всего исправлений: 2)
Ответ на: комментарий от Guguzjaka

Я понимаю, что непонятно в плане того, как принято писать на питоне, зато алгоритм читается явно в hands:

  1. сдали всем игрокам по карте;
  2. посчитали наименьшую сумму карт;
  3. получили список проигравших (у кого сумма карт равна наименьшей);
  4. получили список выигрывших (сумма карт больше наименьшей);
  5. если так получилось, что у всех за столом равное количество очков (у всех сумма карт равна наименьшей), то всех оставляем, иначе только победителей;
  6. если за столом остался один игрок, то возвращаем его и печатаем «это чемпион», если нет то продолжаем новой сдачей карт тем, кто остался - см. п. 1.

Это если я правильно понял суть игры.

vvn_black ★★★★★
()
Ответ на: комментарий от vvn_black

А

Аж глаза разбегаются...

Это не про сам код, а про обилие разных решений)

Guguzjaka
() автор топика
Ответ на: комментарий от Guguzjaka

Если все разобрались, что и как реализовано, значит нормально. Остальное вкусовщина.

vvn_black ★★★★★
()
Ответ на: комментарий от anonymous

Нет.

Принимаю капитуляцию.

То у него дофига инструкций машинных не важны, а теперь размера стека вызовов испугался. Нету у тебя 20 уровней стека вызовов, даже одного нету. Потому что это простой цикл, стек вызовов от него не растёт.

Ты даже не осилил распарсить что тебе написали. Я доношу до тебя что много в каких телах нахожусь, не важно функций, циклов или if’ов, но знать обо всём этом не обязан вообще ничего. Контекст - это здесь и сейчас, на этой самой строчке, и из него должно быть понятно с чем мы работаем. Вот так, 5 символов разницы отличают код который можно понять сходу от кода для понятия которого нужно лазить по всему проекту, и, соответственно, написавшего его профессионала от безграмотного профнепригодного шлака, имеющего наглость иметь собственное мнение о правильном стиле.

В двух строках этого цикла ты контекст не потеряешь.

Кто тебе сказал что завтра их не будет двадцать и что они не переедут в выделенную процедуру?

slovazap ★★★★★
()
Ответ на: комментарий от slovazap

Ты даже не осилил распарсить что тебе написали.

Я осилил распарсить, что ты натягиваешь сову на глобус, пытаясь отстоять высказанную тобой глупость. И продолжаешь натягивать — теперь откуда-то взялась необходимость лазить по всему проекту. У тебя две строки, не проект, не функции, а две строки перед глазами. Здесь некуда лазить. В твою голову после беглого взгляда на код легко должно помещаться полторы дюжины строк «контекста». Здесь их две. Каждое лишние прилагательное - это вредный шум, который мешает читать код.

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

Кто тебе сказал что завтра их не будет двадцать и что они не переедут в выделенную процедуру?

Кто тебе сказал, что код, который не выделен в отдельную процедуру надо писать, будто он в неё выделен? Это идиотское захламление. Будут выделять — переделают.

anonymous
()
Ответ на: комментарий от anonymous

У тебя две строки, не проект, не функции, а две строки перед глазами. Здесь некуда лазить.

Ты видно не писал ничего сложнее «hello, world», ибо в упор не понимаешь как развиваются реальные проекты, что такое поддержка кода, технический долг, рефакторинг и хотя бы ввод нового человека в команду.

Будут — переделают

Собственно вот она и есть, чистая как слеза собственноручная роспись в говнокодерстве.

slovazap ★★★★★
()
Ответ на: комментарий от slovazap

Ты видно не писал ничего сложнее «hello, world», ибо в упор не понимаешь как развиваются реальные проекты, что такое поддержка кода, технический долг, рефакторинг и хотя бы ввод нового человека в команду.

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

Собственно вот она и есть, чистая как слеза собственноручная роспись в говнокодерстве.

Хорошо, ты победил.

anonymous
()
Ответ на: комментарий от anonymous

Поди на жабе пишешь?

Или на пыхе.

anonymous
()
Ответ на: комментарий от anonymous

Да нормально у него всё с кодом, списковые включения во все поля. Ещё async/await присыпать и ещё лучше будет. Обмазаться генераторами с корутинами и вот у тебя эффективный код готов, иначе зачем нужен питон? Разве что переносов сток натыкать — длинные конструкции дольше воспринимаются.

anonymous
()

немного оффтопик, но вот это:

but it's not work

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

seiken ★★★★★
()
Ответ на: комментарий от anonymous

Я понимаю, что ты привык писать переусложнённый говнокод

5 дополнительных букв в названии переменной это не переусложнённый говнокод.

Оттуда и проблемы с техническим долгом, рефакторингом и новыми людьми в команде

Проблем нет, и я считаю что именно благодаря культуре кодинга.

Поди на жабе пишешь?

Боже упаси. Плюсы и питон.

slovazap ★★★★★
()
Ответ на: комментарий от seiken

немного оффтопик, но вот это:

как по мне, так совсем оффтопик, но что я понимать. Ведь я писать как хотеть, а ты читать как мочь... или наоборот... я ходить спросить, но гугиль не подсказать( А если по существу, то автор, вы, совсем не в кассу, притом сознательно. Просто пришли дохнуть яду, чисто ради self-profit (гугл подсказал;Р). Не об того тешетесь.

Я ещё и 2 переменные обозвал, как «m» и «n», так может мне и код не писать или, всё-таки вы допускаете, что человек может совершить банальную ошибку, в силу каких-либо факторов вам не известных? А ещё может я так тонко намекаю, что вам пора перестать дышать, дабы не источать яд на окружающих...(гугл снова бессилен, может посоветуете медиума посильнее?)

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

Guguzjaka
() автор топика
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.