Jump to content




Avoiding common bad practices in your code


9 replies to this topic

#1 Lignum

  • Members
  • 558 posts

Posted 14 May 2017 - 03:03 PM

ComputerCraft is a mod that is very easy to pick up, being well-documented and having lots of fan-made tutorials. Sadly, a lot of these tutorials contain misinformation, having lead to bad practices becoming commonplace in a lot of ComputerCraft programs. These practices lead to various problems, such as unnecessary server load and security flaws.

The purpose of this tutorial is to list some of these common bad practices and to provide better alternatives to them.

1. Globals
A global variable is a variable that is not declared local, like this:
x = 3
They are more complex than that, of course, but if you don't know what a global is, then don't worry. It's possible to write any program you desire without ever using globals, without any inconveniences. If you think you need a global, think harder, because you most definitely don't. Globals are stored in the global table, called _G, which is not bound to your program's execution. This means that any global variables you create will continue to exist after your program ends (and can be accessed from the Lua prompt, that's no good for storing passwords or similarly confidential information!), in addition to there being a performance penalty due to every global variable access being a table index (into _G).

One notable exception is when you're writing APIs. Any functions that you want people to be able to use must be made global, or they will not be accessible. (Although I personally prefer a combination of returning a table from my libraries and loading them with dofile to avoid this.)

So make that:
local x = 3
x = 8 --# Note that reassigning a local variable looks like a global assignment, but isn't one.

Additionally, keep in mind that functions can be global too. This:
function f()
  -- ...
end
should be this:
local function f()
  -- ...
end

2. shell.run
There are perfectly valid reasons to use shell.run, however, too many people use shell.run to implement funcitonality in their program, such as shell.run("clear") or shell.run("pastebin", "get", "xxxxxx"). This is bad practice because it introduces the overhead of having to load an entirely separate program for petty things like clearing the screen. Additionally, OSes may not even provide these programs, leaving your program in the dark.

Instead of using shell.run("clear"), use term.clear(), instead of shell.run("bg worm") use shell.openTab("worm"). In general, find the function that does what you want in the ComputerCraft API instead of relying on other programs. If you can't find it, you can always look at the source code of these programs, since none of the CraftOS programs are magic; They can do just as much as you can.

3. sleep(0)
If you're using sleep(0), you're probably doing something wrong. People often add this line when they get a "Too long without yielding" error, when they should be using the event system. A common example is checking for changes in redstone state. Here's a flawed example program that outputs redstone to the right when there is redstone input on the left (like a repeater):
while true do
  rs.setOutput("right", rs.getInput("left"))
  sleep(0)
end
The infamous "Too long without yielding" error is created when a program has run too long without pulling an event (no other computer can run while yours does, until you pull an event). Adding sleep(0) works because it creates a 0.05 second timer, and pulls a timer event immediately after. This means we are checking the redstone input every 0.05s, which is completely unnecessary, since there is a redstone event that is triggered when the computer's redstone input changes, which is all we need:
while true do
  rs.setOutput("right", rs.getInput("left"))
  os.pullEvent("redstone")
end
Before you add sleep(0), check if there is an event for what you need first. You will in all likelihood find something that fits your needs. Using sleep(0) will make your computer execute as often as possible, putting extra load on the server, so make sure you avoid it as much as you can.

4. Using the parallel API
A common question seen in chats and in Ask a Pro is "How do I pull multiple events at the same time?" or "How do I pull events while receiving redstone messages?". Similarly, a common reply is "Use parallel.waitForAll!". This is bad advice, however, since parallel is an ugly and overkill workaround for doing this sort of thing. Infact, just like with globals, if you think you need parallel, you're probably wrong. There are very few cases in which you need parallel, and in those cases, you'll probably end up writing your own coroutine manager anyway.

Here's a flawed example that prints mouse click coordinates, key presses and incoming rednet messages at the same time:
parallel.waitForAll(function()
  while true do
	local e, btn, x, y = os.pullEvent("mouse_click")
	print(string.format("Clicked %d at %d, %d", btn, x, y))
  end
end, function()
  while true do
	local id, msg, protocol = rednet.receive()
	print(string.format("Got message \"%s\" from %d", msg, id))
  end
end, function()
  while true do
	local e, char = os.pullEvent("char")
	print(string.format("Pressed %s", char))
  end
end)

This example uses parallel.waitForAll to run three event loops at once, when infact we can do this exact same thing with just one loop, which is much simpler and more efficient. Note that rednet.receive() is just os.pullEvent("rednet_message") with a timeout, meaning we can just pull that event. If we don't pass anything into os.pullEvent, it will pull any event it can find, meaning we just have to check which one it is by comparing its first return value (commonly called "e") with our desired event.

while true do
  local e, p1, p2, p3, p4, p5 = os.pullEvent()
  if e == "mouse_click" then
	--# We're essentially renaming our variables here to fit the event better. Not necessary, it's just nicer code.
	local btn, x, y = p1, p2, p3
	print(string.format("Clicked %d at %d, %d", btn, x, y))
  elseif e == "rednet_message" then
	local id, msg, protocol = p1, p2, p3
	print(string.format("Got message \"%s\" from %d", msg, id))
  elseif e == "char" then
	local char = p1
	print(string.format("Pressed %s", char))
  end
end

Edited by Lignum, 15 May 2017 - 06:25 AM.


#2 CLNinja

  • Members
  • 191 posts

Posted 14 May 2017 - 03:12 PM

This is actually super useful. I've been told on many occasions that proper yielding when you don't need to do anything is to sleep(0). Hadn't even though to look for an event that corresponds to what I'm doing. Great work!

#3 Lignum

  • Members
  • 558 posts

Posted 14 May 2017 - 03:16 PM

 CLNinja, on 14 May 2017 - 03:12 PM, said:

This is actually super useful. I've been told on many occasions that proper yielding when you don't need to do anything is to sleep(0). Hadn't even though to look for an event that corresponds to what I'm doing. Great work!

Thanks, glad I could help! It's important to keep in mind that events aren't just a convenience feature, but how ComputerCraft fundamentally works.

#4 Lupus590

  • Members
  • 2,029 posts
  • LocationUK

Posted 14 May 2017 - 04:10 PM

May I suggest that you mention at writing APIs requires using global variables.

Also, you may want to link to BB's Coroutine Guide when you talk about using the parallel API.

#5 Lignum

  • Members
  • 558 posts

Posted 14 May 2017 - 04:47 PM

 Lupus590, on 14 May 2017 - 04:10 PM, said:

May I suggest that you mention at writing APIs requires using global variables.
Sure, will do.

 Lupus590, on 14 May 2017 - 04:10 PM, said:

Also, you may want to link to BB's Coroutine Guide when you talk about using the parallel API.
Not sure why that should be mentioned... using parallel is fine on its own, I talk about finding a reason to use it here.

#6 Bomb Bloke

    Hobbyist Coder

  • Moderators
  • 7,099 posts
  • LocationTasmania (AU)

Posted 15 May 2017 - 12:07 AM

I think there's a bit of a problem here in that anyone who can understand this doesn't need this, and anyone who could stand to benefit may not have a clue what you're talking about.

 Lignum, on 14 May 2017 - 03:03 PM, said:

If you think you need a global, think harder, because you most definitely don't.

Is it really useful to tell coders to "think harder" if you yourself are not prepared to explain scope here? Without the relevant background knowledge, what are they supposed to be thinking about, exactly? :|

Even telling coders to "use the local keyword the first time you assign to a variable" (a process you haven't exactly made clear) wouldn't suffice as a workaround here, because those who currently use globals everywhere likely aren't first referring to all of them in places where they could be defined to work as upvalues.

 Lignum, on 14 May 2017 - 03:03 PM, said:

Globals are stored in the global table, called _G, which is not bound to your program's execution.

If ComputerCraft didn't assign the shell a custom environment table your globals would go in _G, but it does and so they don't.

 Lignum, on 14 May 2017 - 03:03 PM, said:

Additionally, keep in mind that functions can be global too.

Not... exactly. Function pointers can be assigned to globals, but in Lua, the term "global" only applies to variables.

 Lignum, on 14 May 2017 - 03:03 PM, said:

instead of shell.run("bg worm") use multishell.launch({}, "worm")

multishell.launch({}, "worm") is the equivalent of shell.openTab("worm"), only since you're making a new, empty environment table, you're losing shell / multishell API functionality within that new tab.

Generally users should avoid os.run() / multishell.launch() unless they not only understand how scope works but also how environment tables work.

 Lignum, on 14 May 2017 - 03:03 PM, said:

a common reply is "Use parallel.waitForAll!". This is bad advice, however, since parallel is an ugly and overkill workaround for doing this sort of thing.

Worth noting that some functions which pull events can't be broken down into a multi-event pulling loop. Examples are pretty much everything in the turtle API, and functions from many third-party peripherals.

A coder who knows how to correctly build their own coroutine manager to handle such a case may indeed be better off doing so, but they don't need us to tell them that. A coder who doesn't know how is generally much better off sticking with the parallel API rather than trying to "re-invent the wheel" using parts they don't understand.

#7 Lignum

  • Members
  • 558 posts

Posted 15 May 2017 - 06:24 AM

 Bomb Bloke, on 15 May 2017 - 12:07 AM, said:

Is it really useful to tell coders to "think harder" if you yourself are not prepared to explain scope here? Without the relevant background knowledge, what are they supposed to be thinking about, exactly? :|

Even telling coders to "use the local keyword the first time you assign to a variable" (a process you haven't exactly made clear) wouldn't suffice as a workaround here, because those who currently use globals everywhere likely aren't first referring to all of them in places where they could be defined to work as upvalues.
Alright, fair point, I'll try and simplify my explanation when I get around to it.

 Bomb Bloke, on 15 May 2017 - 12:07 AM, said:

If ComputerCraft didn't assign the shell a custom environment table your globals would go in _G, but it does and so they don't.
I wasn't aware of that, but I'll get rid of the specifics anyway, so it doesn't matter.

 Bomb Bloke, on 15 May 2017 - 12:07 AM, said:

Not... exactly. Function pointers can be assigned to globals, but in Lua, the term "global" only applies to variables.
And incidentally, functions are first-class citizens in Lua, meaning they can be assigned to variables. As you've stated, these are actually closures, but that's behind the scenes.

 Bomb Bloke, on 15 May 2017 - 12:07 AM, said:

multishell.launch({}, "worm") is the equivalent of shell.openTab("worm"), only since you're making a new, empty environment table, you're losing shell / multishell API functionality within that new tab.

Generally users should avoid os.run() / multishell.launch() unless they not only understand how scope works but also how environment tables work.
Ah, I figured since multishell had "shell" in its name, it would pass on the shell. I'll change the post in a second.

 Bomb Bloke, on 15 May 2017 - 12:07 AM, said:

Worth noting that some functions which pull events can't be broken down into a multi-event pulling loop. Examples are pretty much everything in the turtle API, and functions from many third-party peripherals.
If I recall correctly, you can break down the turtle functions into an event loop by using the functions in turtle.native().
I've never seen one of those peripherals in my life, but if, as you say, they do exist, I suppose that is then one of those rare cases in which you need the parallel API.

 Bomb Bloke, on 15 May 2017 - 12:07 AM, said:

A coder who knows how to correctly build their own coroutine manager to handle such a case may indeed be better off doing so, but they don't need us to tell them that. A coder who doesn't know how is generally much better off sticking with the parallel API rather than trying to "re-invent the wheel" using parts they don't understand.
Since I wasn't aware of aforementioned peripherals, the only use-case I could think of for parallel was a multitasking system, which is indeed better off with a custom coroutine manager.

#8 Lupus590

  • Members
  • 2,029 posts
  • LocationUK

Posted 15 May 2017 - 09:42 AM

 Lignum, on 15 May 2017 - 06:24 AM, said:

If I recall correctly, you can break down the turtle functions into an event loop by using the functions in turtle.native().

I belive that turtle.native was removed a while ago.

Edited by Lupus590, 15 May 2017 - 09:42 AM.


#9 Bomb Bloke

    Hobbyist Coder

  • Moderators
  • 7,099 posts
  • LocationTasmania (AU)

Posted 18 May 2017 - 11:51 PM

 Lignum, on 15 May 2017 - 06:24 AM, said:

And incidentally, functions are first-class citizens in Lua, meaning they can be assigned to variables.

Pointers yes, functions no. Same deal as with tables and coroutines - this means that if you attempt to copy the content of a variable containing such a pointer, you end up with two variables holding a pointer leading to the same actual object.

Not sure what you meant about closures. Closures are functions that make use of upvalues, to my understanding.

#10 cntkillme

  • Members
  • 24 posts

Posted 19 May 2017 - 07:48 PM

When talking about Lua you're not supposed to think of functions, tables, and closures as pointers even though they really are, you're supposed to think of them as values. The only difference being they're not cloned when you assign to another variable (unlike numbers and booleans).
And all functions in Lua are closures regardless if they have any upvalues. All a closure is, though, is a pointer to a function prototype (contains static info. like constants, instructions, and nested prototypes), its upvalues, and its environment.

Edited by cntkillme, 19 May 2017 - 07:55 PM.






1 user(s) are reading this topic

0 members, 1 guests, 0 anonymous users