[Answered]for lua experts : code optimization ? autoequip project

[whismerhill]

hi I'm looking at taking over the autoequip project, author has stated anyone can step in & take over, I'm still trying to reach him personally to be extra sure.

I started this as fixing it for my own usage, but now I'm looking at making general improvements before publishing (if ever lol )
Anyways my understanding is that this code is executed all the ******* time (since it replaces all click actions pretty much) so it needs to stay fast, I already happened to add print functions (see comment :p) and it was literally spamming the console with hundreds of lines per second. The way stuff is coded right now is like this :

Spoiler

 

local function GetAction(inst, target, action)
	local tool = FindTool(inst.components.inventory, action)
	--if action == ACTIONS.MOWDOWN then print("getaction - tool") end
	if not tool then return end
	if action == ACTIONS.NET and (target.prefab ~="fireflies" and not target.components.health or target.components.health:IsDead()) then return end
	if not target.components.workable and not target.components.hackable then return end
	
	if not target.components.hackable and target.components.workable.action ~= action and not (action == ACTIONS.MOWDOWN and ( target.prefab == "grass" or target.prefab == "reeds" or target.prefab == "sapling" ) ) then return end
	if( target.components.hackable and ( ( action == ACTIONS.HACK and not target.components.hackable:IsActionValid(action,false) ) or ( action ~= ACTIONS.HACK and target.components.workable.action ~= action ) ) ) then return end
	local action = BufferedAction(inst, target, action, tool)
	action.autoequip = true
	return action
end

 

I already added new parts but as you can see the original author used the scheme : "if not xxxx then return end" which basically terminates the function right there & then. Is this more efficient & faster than say ... something like this :

Spoiler

 

local function GetAction(inst, target, action)
	local tool = FindTool(inst.components.inventory, action)
	--if action == ACTIONS.MOWDOWN then print("getaction - tool") end
	if  tool then
      if action == ACTIONS.NET and (target.prefab ~="fireflies" and not target.components.health or target.components.health:IsDead()) then return end
      if not target.components.workable and not target.components.hackable then return end

      if not target.components.hackable and target.components.workable.action ~= action and not (action == ACTIONS.MOWDOWN and ( target.prefab == "grass" or target.prefab == "reeds" or target.prefab == "sapling" ) ) then return end
      if( target.components.hackable and ( ( action == ACTIONS.HACK and not target.components.hackable:IsActionValid(action,false) ) or ( action ~= ACTIONS.HACK and target.components.workable.action ~= action ) ) ) then return end
      local action = BufferedAction(inst, target, action, tool)
      action.autoequip = true
      return action
	end
end

 

The indentation is imperfect it seems lol xD but you should get the idea, I just changed the "if not tool" to a "if tool" that encloses the rest of the code.

In theory it should be the same buuuut I'm really not a LUA guru. Reason I ask this is that there is some checks which are repeated several times like target.components.hackable, I believe if I start enclosing conditions I should be able to reduce the number of repeated checks, but is it beneficial or is the "return end" with negative conditions the best way?

 

Thanks for any answer, I'm a real noob at LUA but I'm trying to improve ^^

[CarlZalph]

When LUA is ran it first gets compiled into its own bytecode for its virtual machine.

The conditionals will short circuit and a (branch if not) is equal to a (branch if equal to) on pretty much any CPU.

The thing you would see differences in is if the CPU has a cache setup and a branch lookahead- it would be best to keep the branches in the case where the CPU will guess it to be on so cache misses are lowered.  If I recall the most used branch lookahead will be for a not equal to due to the high amount of loop usage in programming wherein the branch will execute only if the conditional fails (to escape the loop).

You shouldn't have to worry about this because the compiler should be handling this, though with the virtual machine you're already going to have the overhead and costs associated with it so thinking about a small optimization as this is tiny in the grand scheme.

 

The reason why the author did the check and returned early is to stop making programming pyramids.

It adds ease of code readability, structure, and maintenance for later.

Lest you make the great Pyramid that resembles this (Please don't do this):

Pyramid's big, URL instead.

 

Might I suggest looking over Linus Torvald's requirements for programming a Linux Kernel?

Some of it may seem silly, but the code is some of the cleanest looking I've seen.

[bizziboi]

I suspect it's indeed to prevent nesting ifs and thus increasing indentation, causing your code to run out of the page. Of course, if you don't properly indent that indeed won't be an issue, but it'll make your code really hard to read/debug.

In defense of not doing the early out though - if your function ever needs to add some exit clean-up you need to find every exit - the less exits, the less chances of missing one (and this introducing an error)

[whismerhill]

yeah ok thanks guyz you were a big help

I already read a debate about the coding style, I tend to like indentation (and I prefer 1 tab over a single space that I've seen used by some programmers. Single spaces are too hard on the eyes  ) Though I'm not against some early returns.

What I do not like however are overly long lines that do not fit on my screen . For this very reason the pyramid would probably not fit

 

Anyways question answered, I'm gonna try & remove a few redundant checks