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

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

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

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

local function func()	local com					= require("component")	local event					= require("event")	local gpu					= com.gpu	local turret				= com.os_energyturret	local radar					= com.openperipheral_sensor	local autors				= {"qwertyMAN"}	local version				= "0.9.1"	-- Настройки	local firePleayers			= true	local fireMobs				= true	local Black_Player_List		= {}	local White_Player_List		= {}	-- относительные координаты пушки от сканера	local correct = {		x = 0,		y = 4,		z = 0	}	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								while true do		os.sleep(0)		local target = false		local fire = true		local scan=radar.getPlayers()		if firePleayers and #scan>0 then			if #White_Player_List>0 then				for i=1, #autors do					White_Player_List[#White_Player_List+1] = autors[i]				end				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						end					end					if swich then						target = scan[i].name						break					end				end			elseif #Black_Player_List>0 then				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 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						end					end					if swich then						target = scan[i].name						break					end				end			else				if #autors>0 then					for i=1, #autors do						White_Player_List[#White_Player_List+1] = autors[i]					end				else					target = scan[1].name				end			end			if target and radar.getPlayerByName(target) then				target=radar.getPlayerByName(target).all()				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 vx,vy = pointer(x,y,z)				turret.moveTo(vx,vy)				if turret.isOnTarget() then					turret.fire()				end				fire = false			end		end		target = false		if fireMobs and fire then			local scan=radar.getMobIds()			if #scan>0 then				for i=1, #scan do					local mob					if radar.getMobData(scan[i]).basic() then						mob = radar.getMobData(scan[i]).basic()						target = mob					end				end				if target then					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 vx,vy = pointer(x,y,z)					turret.moveTo(vx,vy)					if turret.isOnTarget() then						turret.fire()					end				end			end		end	endend-- мегакостыльwhile true do	local oop = pcall(func)	print(oop)end

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

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

if target and radar.getPlayerByName(target) then  target=radar.getPlayerByName(target).all()...local mobif 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    ...  endend-- мегакостыль (комментарий самого автора)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 = truefor j=1, #White_Player_List do  if scan[i].name==White_Player_List[j] or not scan[i].name then    swich = false  endendif swich then...local swich = falsefor j=1, #Black_Player_List do  if scan[i].name==Black_Player_List[j] then    swich = true  endendif 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  endend...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  endend

Что выполняют оба фрагмента? Ищут подходящую цель по белому и черному спискам игроков.
Где хранится цель? В переменной 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  endend...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  endend

Код стал немного короче и быстрее, но и это еще не предел.

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

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

PlayerList={"Ded", "Baba", "KurochkaRyaba"}for k,v in pairs(PlayerList)do print(k,v)endfor i=1,#PlayerList do print(PlayerList[i])endPlayerList={[1]="Ded", [2]="Baba", [3]="KurochkaRyaba"}for k,v in pairs(PlayerList)do print(k,v)endfor i=1,#PlayerList do print(PlayerList[i])endPlayerList={["Ded"]=1, ["Baba"]=2, ["KurochkaRyaba"]=3}for k,v in pairs(PlayerList)do print(k,v)endfor i=1,#PlayerList do print(PlayerList[i])endprint(#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]] = 1end

Код немного укоротился, а главное – теперь переполнение памяти не грозит даже при работе в бесконечном цикле, т. к. элемент создается один раз, а в последующие – лишь изменяется его значение.

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

-- было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  endend-- сталоfor j=1, #autors do  Black_Player_List[autors[i]] = nilend

Код заметно укоротился.
Сомневаешься, действительно ли элемент таблицы удаляется? Запусти этот код, и все станет понятным:

PlayerList={["Ded"]=1, ["Baba"]=2, ["KurochkaRyaba"]=3}for k,v in pairs(PlayerList)do print(k,v)endPlayerList["Ded"]=nilfor 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  endend...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  endend-- сталоfor i=1, #scan do  if not White_Player_List[scan[i].name] then     target = scan[i].name     break  endend...for i=1, #scan do  if Black_Player_List[scan[i].name] then      target = scan[i].name      break    endend

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

local x,y,z = target.position.x-correct.x, target.position.y+1.3-correct.y, target.position.z-correct.zlocal 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.zlocal 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,outYend

Учитывая то, как я переписал эту функцию в прошлый раз, а также избавляясь от лишних переменных и перенося часть вычислений внутрь функции, перепишу код таким образом:

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, elevationend...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'а вполне адекватен.

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