Help with complete recoding of quarry program

So, I recently asked (a couple months ago) help for a quarry program that excavated a certain (restricted) area. So, to take advantage of the new frames block in redpower, I am completely rewriting the code to allow for the quarry to dig a completely user specified area. The problem? It is not easy (for me). So I just finished writing what I'm hoping will be the first of very few drafts of this program. What I want to know is:

1. Will the code actually work?
2. Is there any I can improve it/make it smaller?

Don't worry about the GUI or how the user selects the area, I already know how to do that. Also, I'm not worried really about the timing between the frame engines moving, I can tweak that once I build the machine. I just want to know if the logic will work. Any construction criticism is appreciated. Thanks even if this seems like too much!

function bundledOn(side, color)
  c = colors.combine(c, colors[color])
  rs.setBundledOutput(side, c)
end
function bundledOff(side, color)
  c = colors.subtract(c, colors[color])
  rs.setBundledOutput(side, c)
end
function bundledPulse(side, color, interval)
  c = colors.combine(c, colors[color])
  rs.setBundledOutput(side, c)
  sleep(interval)
  c = colors.subtract(c, colors[color])
  rs.setBundledOutput(side, c)
end
function printCentered(height, value)
  local xpos = w/2 - string.len(value)/2
  term.setCursorPos(xpos, height)
  term.write(value)
end
function clearA()
  term.clear()
  term.setCursorPos(1,1)
end
w, h=term.getSize()

function configCheck()
  if fs.exists("quarry/config") then
	config=fs.open("quarry/config", "r")
	tmpSide=config.readLine()
	config.close()
   if tmpSide ~= "top" and tmpSide ~= "bottom" and tmpSide ~= "left" and tmpSide ~= "right" and tmpSide ~= "front" and tmpSide ~= "back" then
	 shell.run("pages/config")
   end
  else
	shell.run("pages/config")
  end
  return tmpSide
end
side=configCheck()
up="white"
down="orange"
west="magenta"
east="lightBlue"
north="yellow"
south="lime"
dig="black"
bundledPulse(side, up, 2)
print(side)
y=65 -- Up/Down
x=14 -- East/West
z=14 -- North/South
zReverse=false
xreverse=false
function breakBlocks()
bundledPulse(side, dig, 0.2)
end
function xMovE()
	rX=x%7
	if xReverse==false then
  if rX==0 then
   for cX=1, x/7 do
	breakBlocks()
	for eM=1, 7 do
	 bundledPulse(side, east, 0.8)
	 sleep(0.2)
	 eM=eM+1
	end
	zMovE()
	cX=cX+1
   end
  else
   for cX=1, x/7 do
	breakBlocks()
	for eM=1, 7 do
	 bundledPulse(side, east, 0.8)
	 sleep(0.2)
	 eM=eM+1
	end
	cX=cX+1
   end
   for cX=1, rX do
	breakBlocks()
	bundledPulse(side, east, 0.8)
	cX=cX+1
	zMovE()
   end
  end
elseif xReverse==true then
  if rX==0 then
   for cX=1, x/7 do
	breakBlocks()
	for wM=1, 7 do
	 bundledPulse(side, west, 0.8)
	 sleep(0.2)
	 wM=wM+1
	end
	zMovE()
	cX=cX+1
   end
  else
   for cX=1, x/7 do
	breakBlocks()
	for wM=1, 7 do
	 bundledPulse(side, west, 0.8)
	 sleep(0.2)
	 wM=wM+1
	end
	cX=cX+1
   end
   for cX=1, rX do
	breakBlocks()
	bundledPulse(side, west, 0.8)
	cX=cX+1
	zMovE()
   end
  end
else error("An unknown error has occurred")
end
if xReverse==false then
  xReverse=true
else
  xReverse=false
end
end
function zMovE()
	rZ=z%7
	if zReverse==false then
  if rZ==0 then
   for cZ=1, z/7 do
	breakBlocks()
	for nM=1, 7 do
	 bundledPulse(side, north, 0.8)
	 sleep(0.2)
	 nM=nM+1
	end
	cZ=cZ+1
   end
  else
   for cZ=1, z/7 do
	breakBlocks()
	for nM=1, 7 do
	 bundledPulse(side, north, 0.8)
	 sleep(0.2)
	 nM=nM+1
	end
	cZ=cZ+1
   end
   for cZ=1, rZ do
	breakBlocks()
	bundledPulse(side, north, 0.8)
	cZ=cZ+1
   end
  end
elseif zReverse==true then
  if rZ==0 then
   for cZ=1, z/7 do
	breakBlocks()
	for nM=1, 7 do
	 bundledPulse(side, south, 0.8)
	 sleep(0.2)
	 nM=nM+1
	end
	cZ=cZ+1
   end
  else
   for cZ=1, z/7 do
	breakBlocks()
	for nM=1, 7 do
	 bundledPulse(side, south, 0.8)
	 sleep(0.2)
	 nM=nM+1
	end
	cZ=cZ+1
   end
   for cZ=1, rZ do
	breakBlocks()
	bundledPulse(side, south, 0.8)
	cZ=cZ+1
   end
  end
else error("An unknown error has occurred")
end
if zReverse==false then
  zReverse=true
else
  zReverse=false
end
end
function excavateLayer()
  zMovE()
  breakBlocks()
  bundledPulse(side, down, 0.8)
end
function excavate()
for i=1, y do
  excavateLayer()
  i=i+1
end
end
excavate()

Well, at first glance

local xpos = w/2 - string.len(value)/2
  term.setCursorPos(xpos, height)
  term.write(value)
--should be
  term.setCursorPos((w/2 - string.len(value)/2), height)
  term.write(value)
--to cut out the variable assignment

You also might want to replace all those 0.8s and 7s with a variable so you can tweak them easier.

Otherwise, I say just go ahead and run it to see if it works.

Pharap, on 06 February 2013 - 06:17 PM said:

Well, at first glance

local xpos = w/2 - string.len(value)/2
  term.setCursorPos(xpos, height)
  term.write(value)
--should be
  term.setCursorPos((w/2 - string.len(value)/2), height)
  term.write(value)
--to cut out the variable assignment

Or to streamline even more it could become this

term.setCursorPos(w/2-#value/2, height)
term.write(value)

Now me at first glance I see this

if zReverse==false then
  zReverse=true
else
  zReverse=false
end

and think "ahhhhh!!!!" make it

zReverse = not zReverse

also where you say

if someVariable == true then
[code]
or
[code]
if someVariable == false then
[code]
they could just become
[code]
if someVariable then -- if true
if not someVariable then -- if false

EDIT: Also in this function

function bundledPulse(side, color, interval)

you are using a variable c, I cant see that declared anywhere, can I suggest you do this

function bundledPulse(side, color, interval)
  local oldOutput = rs.getBundledInput()
  rs.setBundledOutput(side, colors.combine(oldOutput, colors[color]))
  sleep(interval)
  rs.setBundledOutput(side, oldOutput)
end

EDIT 2: Oh its the same with

function bundledOff(side, color)

and

function bundledOn(side, color)

EDIT 3: More stuffs:

• Large lack of defining local variables and functions.
• the up, down, north, east, south, west and dig variables would probably be easier if they were the values from the colors api. for example colors.white, colors.orange, etc. If this change is done you will need to change the functions bundledPulse, bundledOff and bundledOn so that instead of doing a color lookup they just add in the color.
• the sleep value in the function breakBlocks call to bundledPulse may as well be 0.4 since that is the quickest refresh time of an entity so any faster is kinda pointless.
• in the function xMovE where you have

  if xReverse == false then

  and then

  elseif xReverse == true then else end

  the elseif is not needed, you could have

  if not xReverse then -- if its false else -- if its true end

  and obviously if xReverse is a boolean the else is pointless as it will either be true OR false, will never be anything else. You should only consider doing this if you know that you will be using different data types in the one variable. lastly the sleep(0.2) after each bundledPulse call is not needed at all, you are already sleeping for 0.8 seconds in the pulse, why would you need to sleep again?
• all the above with zMovE
• in xMovE where you call zMovE() to return back is bad. use return instead, every time you call a function it goes on a stack, the maximum stack size is 256 so that means the program would crash if it was left running for long enough
• REMOVE FROM ALL FOR LOOPS where you increment the loops number, for example in excavate you have

  i=i+1

  this is bad, the for loop automatically does this for you. so remove those from all loops
• Lastly and its something small, the side validation in configCheck could easily become this

for _,v in pairs(rs.getSides()) do
  if tmpSide == v then
    shell.run("pages/config")
  end
end

TheOriginalBIT, on 06 February 2013 - 06:25 PM said:

Pharap, on 06 February 2013 - 06:17 PM said:

Well, at first glance

local xpos = w/2 - string.len(value)/2
  term.setCursorPos(xpos, height)
  term.write(value)
--should be
  term.setCursorPos((w/2 - string.len(value)/2), height)
  term.write(value)
--to cut out the variable assignment

Or to streamline even more it could become this

term.setCursorPos(w/2-#value/2, height)
term.write(value)

I often forget about that operator, mostly because it's not used in any language beyond lua as far as I know.
Good spotting.

Yeah, I guess you're right about just going for it. I just wanted to see if anyone could detect some potentially fatal flaws before I spent all the time designing the thing. I'm gonna add a progress bar into there. I did run the program and was shocked that no error message popped up. At least my code is running without format errors. Still have no idea if it's doing what I want it to though.

Pharap, on 06 February 2013 - 06:27 PM said:

TheOriginalBIT, on 06 February 2013 - 06:25 PM said:

Pharap, on 06 February 2013 - 06:17 PM said:

Well, at first glance

local xpos = w/2 - string.len(value)/2
  term.setCursorPos(xpos, height)
  term.write(value)
--should be
  term.setCursorPos((w/2 - string.len(value)/2), height)
  term.write(value)
--to cut out the variable assignment

Or to streamline even more it could become this

term.setCursorPos(w/2-#value/2, height)
term.write(value)

I often forget about that operator, mostly because it's not used in any language beyond lua as far as I know.
Good spotting.

Thanks. I'll see what that does. I didn't write that code, I just borrowed it from someone else and it worked, so I didn't really bother with it for fear of breaking it. Everything else is my original work though.

Himself12794, on 06 February 2013 - 06:43 PM said:

Yeah, I guess you're right about just going for it. I just wanted to see if anyone could detect some potentially fatal flaws before I spent all the time designing the thing. I'm gonna add a progress bar into there. I did run the program and was shocked that no error message popped up. At least my code is running without format errors. Still have no idea if it's doing what I want it to though.

Check my reply again, I edited it with all the issues I found.

Also please, please, please do not add a fake progress bar. Most users on this community will not like that one bit.

If you actually have some loading to do, consider using a real progress bar *cough* I made one, its in the link in my signature </end shameless plug> *cough*

TheOriginalBIT, on 06 February 2013 - 06:25 PM said:

Or to streamline even more it could become this

term.setCursorPos(w/2-#value/2, height)
term.write(value)

Now me at first glance I see this

if zReverse==false then
  zReverse=true
else
  zReverse=false
end

and think "ahhhhh!!!!" make it

zReverse = not zReverse

also where you say

if someVariable == true then
[code]
or
[code]
if someVariable == false then
[code]
they could just become
[code]
if someVariable then -- if true
if not someVariable then -- if false

EDIT: Also in this function

function bundledPulse(side, color, interval)

you are using a variable c, I cant see that declared anywhere, can I suggest you do this

function bundledPulse(side, color, interval)
  local oldOutput = rs.getBundledInput()
  rs.setBundledOutput(side, colors.combine(oldOutput, colors[color]))
  sleep(interval)
  rs.setBundledOutput(side, oldOutput)
end

EDIT 2: Oh its the same with

function bundledOff(side, color)

and

function bundledOn(side, color)

EDIT 3: More stuffs:

• Large lack of defining local variables and functions.
• the up, down, north, east, south, west and dig variables would probably be easier if they were the values from the colors api. for example colors.white, colors.orange, etc. If this change is done you will need to change the functions bundledPulse, bundledOff and bundledOn so that instead of doing a color lookup they just add in the color.
• the sleep value in the function breakBlocks call to bundledPulse may as well be 0.4 since that is the quickest refresh time of an entity so any faster is kinda pointless.
• in the function xMovE where you have

  if xReverse == false then

  and then

  elseif xReverse == true then else end

  the elseif is not needed, you could have

  if not xReverse then -- if its false else -- if its true end

  and obviously if xReverse is a boolean the else is pointless as it will either be true OR false, will never be anything else. You should only consider doing this if you know that you will be using different data types in the one variable. lastly the sleep(0.2) after each bundledPulse call is not needed at all, you are already sleeping for 0.8 seconds in the pulse, why would you need to sleep again?
• all the above with zMovE
• in xMovE where you call zMovE() to return back is bad. use return instead, every time you call a function it goes on a stack, the maximum stack size is 256 so that means the program would crash if it was left running for long enough
• REMOVE FROM ALL FOR LOOPS where you increment the loops number, for example in excavate you have

  i=i+1

  this is bad, the for loop automatically does this for you. so remove those from all loops
• Lastly and its something small, the side validation in configCheck could easily become this

for _,v in pairs(rs.getSides()) do
  if tmpSide == v then
	shell.run("pages/config")
  end
end

I understand all that your saying except for the part where about:

TheOriginalBIT said:

in xMovE where you call zMovE() to return back is bad. use return instead, every time you call a function it goes on a stack, the maximum stack size is 256 so that means the program would crash if it was left running for long enough

could you elaborate a bit and perhaps give an example? I'm just a tad confused on what you mean exactly.

Himself12794, on 07 February 2013 - 04:15 AM said:

I understand all that your saying except for the part where about:

TheOriginalBIT said:

in xMovE where you call zMovE() to return back is bad. use return instead, every time you call a function it goes on a stack, the maximum stack size is 256 so that means the program would crash if it was left running for long enough

could you elaborate a bit and perhaps give an example? I'm just a tad confused on what you mean exactly.

Ok so a shortened (pseudo-ish code) example of what you are doing

local function xMovE()
  if <some condition> then
    -- do stuff
  elseif <some condition> then
    zMovE() -- you want to end here
  else
    -- do stuff
  end
end

local function zMovE()
  -- do some stuff
  xMovE()
  -- do more stuff
end

zMovE()

This method of performing tasks is bad. As stated the computer has a program stack… Every time you call a function it gets pushed onto this stack and every time a function finishes running it is popped off the stack and the program continues running the previous function from where it was up to. This stack has a maximum size of references it can hold, in the case of ComputerCraft this is 256. So when this stack fills up and you try to push another item to it you would get a 'stack overflow' error, or sometimes a 'java arrayIndexOutOfBounds:256' (depends on which one Lua wants to throw at you).

In the code you have written it would do the following (assuming this is how we would visualise the stack

zMovE
zMovE —> xMovE
zMovE —> xMovE —> zMovE
zMovE —> xMovE —> zMovE —> xMovE
...

now hopefully you can see a pattern there of what is happening… Now obviously with the code it wont always call zMovE but instead the function would go down another if statement, finish and return, so you may get a little more than 256 function calls in there, but it is still an issue.

Now as outlined in the PIL here (http://www.lua.org/pil/4.4.html) we can see that we can leave a block early by using return and break.
Break is intended for use inside loops to exit the loop before it normally would, or to exit an infinite loop.
Return will leave the current function (or program if the block is the body of the program, not a function). Using a return we can leave out of a block earlier than intended AND we can also pass back some values if we wish too. Example:

local function isAllowed(something)
  if fs.exists(someFile) then
    print("Exists!")
    return true
  else
    return false
  end
end

in lua we can also return multiple values by using a , between each return value. When attempting to capture these returns we also need to use multiple values (an example of this is term.getSize() where it returns the width and height of the terminal)

So for your program you need to do something like this (again pseudo-ish code)

local function xMovE()
  if <some condition> then
    -- do stuff
  elseif <some condition> then
    return -- you want to end here
  else
    -- do stuff
  end
end

local function zMovE()
  -- do some stuff
  xMovE()
  -- do more stuff
end

zMovE()

Hope this now makes sense :)/>

TheOriginalBIT, on 07 February 2013 - 04:34 AM said:

Himself12794, on 07 February 2013 - 04:15 AM said:

I understand all that your saying except for the part where about:

TheOriginalBIT said:

in xMovE where you call zMovE() to return back is bad. use return instead, every time you call a function it goes on a stack, the maximum stack size is 256 so that means the program would crash if it was left running for long enough

could you elaborate a bit and perhaps give an example? I'm just a tad confused on what you mean exactly.

Ok so a shortened (pseudo-ish code) example of what you are doing

local function xMovE()
  if <some condition> then
	-- do stuff
  elseif <some condition> then
	zMovE() -- you want to end here
  else
	-- do stuff
  end
end

local function zMovE()
  -- do some stuff
  xMovE()
  -- do more stuff
end

zMovE()

This method of performing tasks is bad. As stated the computer has a program stack… Every time you call a function it gets pushed onto this stack and every time a function finishes running it is popped off the stack and the program continues running the previous function from where it was up to. This stack has a maximum size of references it can hold, in the case of ComputerCraft this is 256. So when this stack fills up and you try to push another item to it you would get a 'stack overflow' error, or sometimes a 'java arrayIndexOutOfBounds:256' (depends on which one Lua wants to throw at you).

In the code you have written it would do the following (assuming this is how we would visualise the stack

zMovE
zMovE —> xMovE
zMovE —> xMovE —> zMovE
zMovE —> xMovE —> zMovE —> xMovE
...

now hopefully you can see a pattern there of what is happening… Now obviously with the code it wont always call zMovE but instead the function would go down another if statement, finish and return, so you may get a little more than 256 function calls in there, but it is still an issue.

Now as outlined in the PIL here (http://www.lua.org/pil/4.4.html) we can see that we can leave a block early by using return and break.
Break is intended for use inside loops to exit the loop before it normally would, or to exit an infinite loop.
Return will leave the current function (or program if the block is the body of the program, not a function). Using a return we can leave out of a block earlier than intended AND we can also pass back some values if we wish too. Example:

local function isAllowed(something)
  if fs.exists(someFile) then
	print("Exists!")
	return true
  else
	return false
  end
end

in lua we can also return multiple values by using a , between each return value. When attempting to capture these returns we also need to use multiple values (an example of this is term.getSize() where it returns the width and height of the terminal)

So for your program you need to do something like this (again pseudo-ish code)

local function xMovE()
  if <some condition> then
	-- do stuff
  elseif <some condition> then
	return -- you want to end here
  else
	-- do stuff
  end
end

local function zMovE()
  -- do some stuff
  xMovE()
  -- do more stuff
end

zMovE()

Hope this now makes sense :)/>

Thank you! I see what you're saying there. I will try it out.