Ребята, помогите оптимизировать код


(Nikita) #1

Была задача: написать метод для … размена монет :))
входные данные: 2$ (200 центов)
выходные: хеш с количеством монет на нужную сумму, исходя из количества монет в автомате для размена - { 50=>3,25=>1,10=>2,5=>1 }
Написал такой вот код, понимаю что “говнокод”. Подскажите как оптимизировать, или просто критику по коду?

class Store
attr_accessor :bank

def initialize(a:10,b:10,c:10,d:5,e:3,f:3)
self.bank = {50=>f,25=>e,10=>d,5=>c,2=>b,1=>a}
end

def pockets(vol)

@result = {}
@weigth = [50,25,10,5,2,1]
@count = 1
total = $account.total_exchange
no_money = "No coins for this amount."
m = vol/50
e = vol%50
if vol > total
  @result = "No coins for this amount. Max exchange #{total}"
else
  if m > @bank[50]
    left = (m - @bank[50])*50 + e
    @result[50] = @bank[50]
    begin
      check(left, @bank[25])
    rescue
      @result = no_money
    end
  elsif m <= @bank[50] && e == 0
    @result[50] = m
  elsif m <= @bank[50] && e != 0
    @result[50] = m
    begin
      check(e, @bank[25])
    rescue
      @result = no_money
    end
  end
  transaction(@result) if @result.is_a?(Hash)
end

@result

end

def total_exchange
sum = 0

@bank.each do |k,v|
  sum += k*v
end
sum

end

private

def check(cash,bank)

value = @weigth[@count]
m = cash/value
e = cash%value
@count += 1
if m > @bank[value]
  left = (m - @bank[value])*value + e
  @result[value] = @bank[value]
  check(left,@weigth[@count])
elsif m <= @bank[value] && e == 0
  @result[value] = m
elsif m <= @bank[value] && e != 0
  @result[value] = m
  check(e,@weigth[@count])
end

end
end


(Nikita) #3

Хорошая критика, давайте еще!
Можно ли переписать такой код через &block, lямбду?


(Kvokka) #4

побуду руикритиком, хотя, проще поставь его и повинуйся ему

имена переменных - не информативно
дилнна метода более 7 строк- пованивает, разбить
глобальные переменные- это реально надо?
слишком много условий в 1 методе
использовать явный выход, а не городить кучу из if/else
магические цифры в коде: 50,25
rescue нужно, когда нечто, что ты не можешь контролировать делает raise. а тут это просто от беспомощности.
@count - не адекватное имя.
No coins for this amount это из гугл транслейта что ли? There is no coins for this amount.

в целом- это говнокод и не важно работает это или нет. по стилю говно. только после того, как стиль в порядке можно читать, а пока что obey rubycritic && rubocop.

Хочешь критику- устройся на работу, или плати ментору, или дорасти до более интересных вопросов.
А то будешь получать ответы лишь, когда кому-то будет ну совсем охота работать :wink:


(Nikita) #5

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


(Kvokka) #6

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


(Nikita) #7

Мне нужна конструктивная критика, за сарказмом я могу и к соседу по этажу обратится.


(Kvokka) #8

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

мне тебя жаль


(Nikita) #9

Да я хочу бесплатное код ревью, это же форум где более продвинутые пользователи помогают менее продвинутым, я правильно понимаю?
В заголовке темы написано - помогите оптимизировать код. Я не понимаю в данный момент как по другому написать такую задачу, если кто либо может показать как - будет круто. А у тебя видимо шляпа упала, когда ты ответил такому недоросшему говнокодеру как я. Я не раздаю паршивые “спасибо”.
Если у тебя кроме 9 блестящих замечаний, сарказма и жалости больше ничего нет, то тебе осталось только пожалеть меня.


(Сергей) #10

Ну эта задача легко гуглится. И она решается с помощью рекурсии. На выходе Вы должны получить что-то типа хеша хешей. Например, у Вас есть все монеты в количестве 10шт. каждая. Вы можете розменять 200 центов взяв токо монеты 50 номинал или 25 номинал. Способов розмена будет несколько. Ещё бы эти вычисления перенести в модель, выделить роуты и контроллер. И не смешивать это все в одном файле. Rest функционала там нету.


(Nikita) #11

Для этого обязательно подымать рельсу? или можно на синатре оставить?
Можно поподробней про отсутствие rest?
Разве возможность отправки запросов GET/POST в api приложения, для отправки и получения инфы в джейсоне, не есть rest функционал?


(Lavandosovich) #12

ты кнепсек то гуглил ,не? метод покет и чек ДОСТАВЛЯЮТ. можешь начать с СИКПа. Там наткнешься на многое, в том числе на размен монет. только с рекурсией не заигрывайся, память ведь, тыры-пыры.


(Evgeniy) #13

С таким-то отношением много помощи дождешься


(Nikita) #14

Спасибо за knapsack и сикп. Нашел в закромах “Алгоритмы: построение и анализ”, попробую пожевать.


(Kvokka) #15

очень надеюсь, что для любителей рекурсии ради рекурсии в аду есть отдельный рекурсивный котел

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


(Vadim) #16

да и что бы поднять рельсу, не забудь сначала поднять рельсу