Перейти к содержимому






Фотография
* * * * * 1 голосов

7 способов улучшить код

Написано eu_tomat , 27 Март 2016 · 1 671 просмотров

Здравствуй, брат автоматизатор!

 

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

 

Небольшое отступление:
Полгода назад я задумал написать серию статей о программировании для новичков, где я писал код и объяснял, почему он должен писаться так, а не иначе. Но такое объяснение оказалось для меня непосильным занятием, т. к. мне приходилось спорить с самим собой. Легко было только один раз, когда я обнаружил серьезный дефект, наверное, во всех копалках на проекте. Но потом я получил стрелу в колено...
Вернувшись на проект, я неожиданно для себя понял, что демонстрировать эффективность удачного кода значительно проще, противопоставляя его коду неудачному.

 

В прошлый раз я рассказал о том, как математика помогает писать более простые, понятные и при этом более эффективные программы.
qwertyMAN заметил, что использование взятия остатка от деления – это не математическая проблема, и я с ним отчасти согласился. Решение о том, использовать или нет операцию %, мало влияет на общий алгоритм, но сильно влияет на то, как будет выглядеть готовый код. Но всё-таки об этой операции следует помнить уже на этапе проектирования алгоритма, а не только на этапе кодинга.
А вот, о тонкостях написания кода я хочу рассказать сегодня, не особо вдаваясь в общие алгоритмы. Моим подопытным будет уже знакомый код из охранной системы турелей.
Вот сам код:

Спойлер

 

Первое, на что я обратил внимание – это использование pcall. Именно оно и побудило меня к разбору кода, но прежде я предлагаю обсудить другой нюанс.

 

1. Избегай повторения уже выполненных вычислений
Имеются два фрагмента кода:

if target and radar.getPlayerByName(target) then
  target=radar.getPlayerByName(target).all()
...
local mob
if radar.getMobData(scan[i]).basic() then
  mob = radar.getMobData(scan[i]).basic()
  target = mob
И в обоих фрагментах происходит дублирование вызовов. Первый раз вроде как для проверки. Но такой код всё равно не снимает проблему, т. к. при первом вызове функция может вернуть одно значение, а при втором вернет другое. Правильным решением будет сохранить результат в переменную. И если ее значение удовлетворяет условиям, использовать ее в дальнейшем. Во втором фрагменте следует заодно избавиться от переменной mob, которая больше нигде не используется.
if target then
  target = radar.getPlayerByName(target)
  if target then
    target=target.all()
...
target = radar.getMobData(scan[i])
if target then
  target = target.basic()
Почему я проверяю результаты getPlayerByName и getMobData, а не других функций вроде all() или basic()? Потому что именно они могут вызвать ошибку. Ошибка возникает от того, что игрок или моб может покинуть зону действия сенсора, пока выполняется обработка внутри программы. К сожалению, указанной проверки недостаточно, т. к. мод OpenPeripheral вместо того чтобы вернуть nil или иным образом указать на проблему, тупо генерирует ошибку, не оставляя иного варианта кроме использования pcall.

 

2. Используй защищенный режим только там, где это необходимо
Код программы выглядит примерно так:

local function func()
  -- почти весь код программы помещен в эту функцию
  -- включая инициализацию переменных и определение функций
  ...
  -- этот бесконечный цикл прерывается из-за отсутствующей обработки ошибок
  while true do
    ...
    local target = false
    local scan=radar.getPlayers()
    ...
    target = ...
    if target and radar.getPlayerByName(target) then
      target=radar.getPlayerByName(target).all()
      ...
    end
    ...
  end
end
-- мегакостыль (комментарий самого автора)
while true do
	local oop = pcall(func)
	print(oop)
end
Автор понимает, что использует костыль, но правильное решение использовать не хочет. Подобные городушки провоцируют появление новых: уже сейчас на ровном месте появилась дополнительная функция и вложение двух бесконечных циклов. Но главная проблема этого кода в том, что pcall скрывает любые ошибки, и дальнейшая отладка программы становится затруднительной. Даже печать результата, возвращаемого pcall, реализована неверно – ничего кроме false выведено не будет.
Для решения проблемы следует использовать pcall точечно, только там, где это необходимо, а именно в вызове getPlayerByName и getMobData. А если есть возможность вообще обойтись без pcall, то в готовой программе без него следует обойтись. В нашем случае обойтись без pcall, похоже, нельзя. Поэтому убираем наш костыль и функцию func, оставив лишь ее содержимое, и дорабатываем проблемные участки таким образом:
if target then
  local flag,res = pcall(radar.getPlayerByName,target)
  if flag then
    target=res.all()
...
local flag,res = pcall(radar.getMobData,scan[i])
if flag then
  target = res.basic()
Пришлось ввести дополнительные переменные, зато код избавился от костыля со всей его обвязкой, а ошибка, генерируемая модом OpenPeripheral, локализована, и новых проблем не создаёт. Программа работает стабильно и ее отладка не нарушена. На этом можно было бы и закончить, но я уже вошёл во вкус. Поэтому продолжу давать советы, иллюстрируя их фрагментами кода:

 

3. Выноси за цикл всё, что возможно
Это правило подобно первому, но пренебрежение им не так бросается в глаза начинающего программиста, т.к. сам код не дублируется, зато дублируется его исполнение.
Восстановим общий алгоритм работы программы, чуть подробнее разобрав этот кусок:

while true do
  local target = false
  ...
  local scan=radar.getPlayers()
  if firePleayers and #scan>0 then
    if #White_Player_List>0 then
      добавление списка авторов в в белый список
      target = первый игрок на радаре вне белого списка
    elseif #Black_Player_List>0 then
      удаление авторов из черного списка
      target = первый игрок на радаре в черном списке
    else
      if #autors>0 then
        перенос авторов в белый список
      else
        target = первый игрок на радаре
  вычисление координат цели и выполнение выстрела
  сканирование мобов, вычисление их координат с последующим расстрелом.
Как ты уже догадался по заголовку, в бесконечном цикле выполняется что-то явно лишнее. А именно, работа со списком авторов. Всё это могло бы прекрасно работать и до основного цикла, создавая при этом нужный эффект. Более того, вынос этого кода за цикл позволит исправить серьезнейшую ошибку: на каждой итерации бесконечного цикла происходит добавление авторов в белый список, при этом их наличие в белом списке никак не проверяется, но каждый раз происходит добавление новых. Даже с двумя планками памяти 3.5 уровня через сотню-другую тысяч итераций программа завершится с ошибкой «not enough memory».
Вынося лишние действия за цикл, ты избавишь программу как от неконтролируемого расхода памяти, так и от лишних действий, замедляющих ее работу. Думаю, демонстрировать корректный код здесь излишне. Достаточно лишь вынести эти участки кода из цикла.

 

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

-- этот код находится за циклом
local correct = { x = 0, y = 4, z = 0 }
...
-- а этот внутри
local x,y,z = target.position.x-0.5-correct.x, target.position.y+0.3-correct.y, target.position.z-0.5-correct.z
...
local x,y,z = target.position.x-0.5-correct.x, target.position.y-0.5-correct.y, target.position.z-0.5-correct.z
Логичным будет переписать код таким образом:
-- этот код за циклом
local correct = { x = 0+0.5, y = 4+1, z = 0+0.5 }
...
-- а этот внутри
local x,y,z = target.position.x-correct.x, target.position.y+1.3-correct.y, target.position.z-correct.z
...
local x,y,z = target.position.x-correct.x, target.position.y+0.5-correct.y, target.position.z-correct.z
Так мы избавимся от лишних сложений и вычитаний в цикле. Лишние сложения при инициализации таблицы correct не сильно помещают, т. к. они выполняются всего один раз, а нужны они для наглядности кода. Поясню, что здесь происходит.
Во-первых, турель и сенсор находятся в разных блоках, поэтому координаты игрока следует скорректировать на эту разницу. Координаты турели относительно сенсора хранятся в таблице correct.
Во-вторых, сенсор определяет координаты игроков и мобов относительно своего северо-западного угла, а турель вращается вокруг центра блока. Поэтому приходится корректировать координаты (x,z) на половину блока по горизонтали.
В-третьих, сама турель стреляет на один блок выше уровня ног игрока или моба. Автор корректирует высоту цели в зависимости от цели: для игрока прицел приподнимается на 0.3 блока, чтобы игрок не мог уходить от выстрела прыжком или прятаться за блоки, а для мобов прицел опускается на полблока, чтобы попадать, например, в кур или свиней. Но тут тоже не всё просто. Чтобы попадать в цыплят, прицел следует опустить еще ниже, но тогда турель промахивается мимо взрослых кур, стреляя им куда-то под ноги. Для эффективной стрельбы по любым мобам нужен алгоритм, определяющий вид моба, его возраст, и находящий по таблице его рост. Причем, нужен не только рост. Например, нет смысла целиться в нижнюю часть слизня, т. к. тот постоянно прыгает. В общем, это отдельная тема для поиска оптимального алгоритма, сейчас же я хочу продолжить рассказ о кодинге.
В итоговом коде не только вынесены лишние вычисления из цикла. Кроме этого он стал более логичным: числа 1.3 и 0.5 по сути означают высоту цели относительно ног игрока или моба.

 

4. Прерывай циклы, когда они уже выполнили свою задачу
Вот два подобных друг другу фрагмента кода:

local swich = true
for j=1, #White_Player_List do
  if scan[i].name==White_Player_List[j] or not scan[i].name then
    swich = false
  end
end
if swich then
...
local swich = false
for j=1, #Black_Player_List do
  if scan[i].name==Black_Player_List[j] then
    swich = true
  end
end
if swich then
Задача циклов в том, чтобы при подходящем случае изменить значение переменной switch. Но как только оно изменилось, зачем продолжать работу? Далай break. Иначе выполнение твоей программы замедляется.
Пока я писал этот текст, то напрягался при каждом наборе названия переменной swich. Нет такого слова, зато есть слово switch, и не глядя я набирал именно его. Поэтому буду писать switch. Кроме того, название переменной все равно не отражает ее сути. С тем же успехом можно было использовать однобуквенное название переменной. А лучше бы и вовсе избавиться от нее.

 

5. Избавляйся от лишних переменных
Вот те же фрагменты в немного дополненном составе, внутри других циклов и с оператором break, как же теперь без него:

local target = false
...
for i=1, #scan do
  local swich = true
  for j=1, #White_Player_List do
    if scan[i].name==White_Player_List[j] or not scan[i].name then
      swich = false
      break
    end
  end
  if swich then
    target = scan[i].name
    break
  end
end
...
for i=1, #scan do
  local swich = false
  for j=1, #Black_Player_List do
    if scan[i].name==Black_Player_List[j] then
      swich = true
      break
    end
  end
  if swich then
    target = scan[i].name
    break
  end
end
Что выполняют оба фрагмента? Ищут подходящую цель по белому и черному спискам игроков.
Где хранится цель? В переменной target.
Что хранится в переменной switch? Флаг того, что переменная target должна быть изменена.
А зачем нам этот флаг? Что мешает сразу изменить переменную?
local target = false
...
for i=1, #scan do
  target = scan[i].name
  for j=1, #White_Player_List do
    if scan[i].name==White_Player_List[j] or not scan[i].name then
      target = false
      break
    end
  end
  if target then
    break
  end
end
...
for i=1, #scan do
  for j=1, #Black_Player_List do
    if scan[i].name==Black_Player_List[j] then
      target = scan[i].name
      break
    end
  end
  if target then
    break
  end
end
Код стал немного короче и быстрее, но и это еще не предел.

 

Еще непонятно, что делает, or not scan[i].name в условии. Повлиять это выражение может в том случае, если scan[i].name будет равно false, или nil. А разве такое возможно? Даже не знаю, как классифицировать этот недочет. Видимо, от старых экспериментов осталось. Посоветовать можно только одно: вычищать код перед публикацией.

 

6. Используй ассоциативные возможности таблиц Lua
Таблицы в Lua – это больше чем массивы. Это и объекты, и списки, и словари. А может, и что-то еще, о чем я забыл или даже не знал.
Таблицы в Lua – это наборы пар ключ-значение. В качестве ключей и значений таблицы может быть что угодно кроме nil.
Пара ключ-значение в таблицах Lua присутствует даже если мы не используем ключи явным образом.
Попробуй запустить такой код:

PlayerList={"Ded", "Baba", "KurochkaRyaba"}
for k,v in pairs(PlayerList)do print(k,v)end
for i=1,#PlayerList do print(PlayerList[i])end

PlayerList={[1]="Ded", [2]="Baba", [3]="KurochkaRyaba"}
for k,v in pairs(PlayerList)do print(k,v)end
for i=1,#PlayerList do print(PlayerList[i])end

PlayerList={["Ded"]=1, ["Baba"]=2, ["KurochkaRyaba"]=3}
for k,v in pairs(PlayerList)do print(k,v)end
for i=1,#PlayerList do print(PlayerList[i])end
print(#PlayerList)
print(PlayerList["Ded"])
print(PlayerList["Baba"])
print(PlayerList["RedHatBaby"])
Прикрепленное изображение
В первом случае мы не указываем ключи явно, но они создаются. Мы свободно перебираем как пары ключ-значение, так и значения по их индексу, который совпадает с ключом.
Во втором случае мы явно указали ключи. Перебор пар ключ-значение показывает, что элементы таблицы хранятся в неведомом нам порядке, скорее всего, в соответствии с неким хешем, но на перебор по индексу это никак не влияет, порядок не нарушен, а это самое главное.
В третьем случае мы поменяли ключи и значения местами. Естественно, ни о каком переборе по индексу теперь не идет и речи. Более того, определить длину таблицы теперь тоже не так просто. Зато появилась возможность, не выполняя перебор всех значений в цикле, одной командой определить, присутствует ли игрок в списке. Для игроков Дед и Баба есть значение, а игрок КраснаяШапочка в список не внесен, и имеет значение nil.

 

Как это соотносится с нашим кодом? Попробуем заполнять списки игроков таким образом:
local Black_Player_List = { ["qwertyMAN"]=1 }
local White_Player_List = { ["Ded"]=1, ["Baba"]=1, ["KurochkaRyaba"]=1 }
В качестве значения я использовал 1, и оно может быть любым, но 1 - записывается кратко. Главное, чтобы не nil. И не false, чтобы проще было выполнять проверку элемента.
То, что qwertyMAN оказался в черном списке, ему никак не повредит, он же автор.

 

Теперь все фрагменты кода изменятся таким образом:

 

Добавление списка авторов в в белый список:

-- было
for i=1, #autors do
  White_Player_List[#White_Player_List+1] = autors[i]
end
-- стало
for i=1, #autors do
  White_Player_List[autors[i]] = 1
end
Код немного укоротился, а главное – теперь переполнение памяти не грозит даже при работе в бесконечном цикле, т. к. элемент создается один раз, а в последующие – лишь изменяется его значение.

 

Удаление авторов из черного списка:

-- было
for i=#Black_Player_List, 1, -1 do
  for j=1, #autors do
    if Black_Player_List[i] == autors[j] then
      table.remove(Black_Player_List,i)
    end
  end
end
-- стало
for j=1, #autors do
  Black_Player_List[autors[i]] = nil
end
Код заметно укоротился.
Сомневаешься, действительно ли элемент таблицы удаляется? Запусти этот код, и все станет понятным:
PlayerList={["Ded"]=1, ["Baba"]=2, ["KurochkaRyaba"]=3}
for k,v in pairs(PlayerList)do print(k,v)end
PlayerList["Ded"]=nil
for k,v in pairs(PlayerList)do print(k,v)end
И напоследок два уже разобранных перед этим фрагмента, которые еще более упростились:
-- было
for i=1, #scan do
  target = scan[i].name
  for j=1, #White_Player_List do
    if scan[i].name==White_Player_List[j] then
      target = false
      break
    end
  end
  if target then
    break
  end
end
...
for i=1, #scan do
  for j=1, #Black_Player_List do
    if scan[i].name==Black_Player_List[j] then
      target = scan[i].name
      break
    end
  end
  if target then
    break
  end
end
-- стало
for i=1, #scan do
  if not White_Player_List[scan[i].name] then
     target = scan[i].name
     break
  end
end
...
for i=1, #scan do
  if Black_Player_List[scan[i].name] then
      target = scan[i].name
      break
    end
end
7. Минимизируй идентичные участки кода
При добавлении сходного функционала в программу можно скопировать рабочий участок кода и немного изменить его. Иногда до неузнаваемости. Но если фрагмент достаточно велик, а изменения малы, то правильнее будет вынести этот фрагмент в отдельную функцию. Во-первых, итоговой код будет более компактным. Во-вторых, при необходимости будет проще вносить изменения, не правя все участки кода.
В программе имеется два таких похожих участка:
local x,y,z = target.position.x-correct.x, target.position.y+1.3-correct.y, target.position.z-correct.z
local vx,vy = pointer(x,y,z)
turret.moveTo(vx,vy)
if turret.isOnTarget() then
  turret.fire()
end
...
local x,y,z = target.position.x-correct.x, target.position.y+0.5-correct.y, target.position.z-correct.z
local vx,vy = pointer(x,y,z)
turret.moveTo(vx,vy)
if turret.isOnTarget() then
  turret.fire()
end
Ранее мы уже выяснили, что единственная изменяемая величина здесь – это коррекция высоты цели. Сейчас основной вопрос: что из этого следует вынести в отдельную функцию. С точки зрения минимизации кода следует выносить почти всё. Но чтобы сделать код более логичным, не следует всё мешать в одну кучу. Лучшим решением мне кажется вынос всего, что связано с вычислениями, в уже имеющуюся функцию pointer. Вот она:
local function pointer(x,y,z)
  local distXY = math.sqrt(x^2+z^2)
  local distDY = math.sqrt(y^2+distXY^2)
  local outX = math.deg(math.acos(x/distXY))+90
  local outY = 90-math.deg(math.acos(y/distDY))
  if z<0 then
    outX = (180-outX)%360
  end
  return outX,outY
end
Учитывая то, как я переписал эту функцию в прошлый раз, а также избавляясь от лишних переменных и перенося часть вычислений внутрь функции, перепишу код таким образом:
local function pointer(pos,h)
  local x,y,z = pos.x-correct.x, pos.y-correct.y+h, pos.z-correct.z
  local azimuth=math.deg(math.atan2(x,-z))%360
  local elevation=math.deg(math.atan2(y,math.sqrt(x*x+z*z)))
  return azimuth, elevation
end
...
turret.moveTo(pointer(target.position,1.3))
if turret.isOnTarget() then
  turret.fire()
end
...
turret.moveTo(pointer(target.position,0.5))
if turret.isOnTarget() then
  turret.fire()
end
Конечно, этот код можно ужать еще плотнее, но я увижу в этом смысл, если дальнейшая доработка программы вынудит меня продублировать эти блоки кода.

 

На этом закончу. Возможно, я дал бы тебе еще пару советов, но за исключением описанных выше моментов код qwertyMAN'а вполне адекватен.

 

Программируй красиво, брат!



  • Zer0Galaxy, Krutoy, Fingercomp и 5 другим это нравится



TL:DR
Но в роде норм. Так что лайк.

Забыл про правило 0: не говнокодить и не лагодромить.

Я хотел акцентировать внимание не на том, от чего следует уйти, а на том, куда желательно прийти. Все мы говнокодим в начале пути: сначала пелёнки пачкаем, потом ломаем инструменты на уроках труда, залагиваем сервер майнкрафта, заплёвываем какахами своих оппонентов по форуму, и даже превращаем в брак целые партии продукции на заводе. Почти все понимают, что это плохо, но не у каждого получается этого избежать. Кто-то не знаком с теорией, у кого-то не достаточна практика, а кому-то нужен практический совет и живой пример. Последнему и посвящена запись моего блога. Или у тебя возникло чувство, что мой текст недостаточно длинный?
    • Fingercomp и qwertyMAN это нравится

Вот тебе не влом было копаться в чужом коде...)

Вот тебе не влом было копаться в чужом коде...)

Вот, тебе же не влом писать свою операционку. Вряд ли ты ее напишешь, не копаясь в чужом коде.
А этот код оформлен более-менее достойно, и разобрать его не сложно – в этом его большой плюс.
В конце концов, это самое позитивное, что я мог сделать с этим кодом в тот момент. Тебе же понравился результат?

Обратные ссылки на эту запись [ URL обратной ссылки ]

Обратных ссылок на эту запись нет

Август 2018

В П В С Ч П С
   1234
567891011
12131415161718
192021 22 232425
262728293031 

Новые записи

Новые комментарии

Последние посетители

  • Фотография
    Asior
    17 авг 2018 - 13:20
  • Фотография
    Kartze
    22 июл 2018 - 21:28
  • Фотография
    Alex
    19 июл 2018 - 16:42
  • Фотография
    Sploom
    19 июл 2018 - 15:41
  • Фотография
    Fingercomp
    19 июл 2018 - 14:17
  • Фотография
    bb_slipers
    19 июл 2018 - 13:10
  • Фотография
    Aubrey
    19 июл 2018 - 12:42
  • Фотография
    BrightYC
    19 июл 2018 - 12:16
  • Фотография
    davial
    19 июл 2018 - 09:54
  • Фотография
    NEO
    19 июл 2018 - 09:33
  • Фотография
    mrlobaker
    11 май 2018 - 07:54
  • Фотография
    krovyaka
    08 май 2018 - 20:02
  • Фотография
    Romanok2805
    08 май 2018 - 09:52
  • Фотография
    anar66
    27 апр 2018 - 01:40
  • Фотография
    Jakowlew
    14 мар 2018 - 21:10
  • Фотография
    AtomicScience
    03 мар 2018 - 14:00
  • Фотография
    Mirotworex
    14 янв 2018 - 23:56
  • Фотография
    Megageorgio
    09 янв 2018 - 21:57
  • Фотография
    MrSnake20_15
    08 янв 2018 - 17:17
  • Фотография
    monkey
    26 дек 2017 - 14:39
  • Фотография
    serafim
    21 ноя 2017 - 15:07
  • Фотография
    nikit356
    22 окт 2017 - 12:37
  • Фотография
    ivan52945
    19 окт 2017 - 01:00
  • Фотография
    vx13
    08 авг 2017 - 19:13
  • Фотография
    Marsi233
    07 авг 2017 - 22:48
  • Фотография
    titanium123
    01 авг 2017 - 12:56
  • Фотография
    FeodorKekovich
    25 июн 2017 - 13:20
  • 26 апр 2017 - 06:26
  • Фотография
    electronic_steve
    19 апр 2017 - 21:47
  • Фотография
    Dark
    21 мар 2017 - 16:09