Asking For Code Review (By Pros)

I'm sure this is the right section since I am asking for Pros.

Here we go, I have made a program and It is bug free as far as im concerned. http://pastebin.com/4qk1wReT
It's primary purpose is to run through the inventory and refuel. The difference between 'refuel all' and this program is that it is designed to refuel to a minimum fuel level before it finishs.

So my question is if there is anything wrong with the code that you can spot, or any places where I could improve code for execution time, cohesion, basically software metrics.

--Charge Program - p3pt3RQM
--Developed by UltraNoobian
local version = "0.04d"

See my comments in the code, prefixed with –>>

Also cleaned up some code indentation.

For the most part this is laid out fairly well. The code is pretty easy to read. There is a bit of redundant code you can remove, and at least one function that is never called, but all in all it isn't too bad. :)/>

Spoiler

--Charge Program - p3pt3RQM
--Developed by UltraNoobian
local version = "0.04d"

--API_LOAD
--END API_LOAD


--VAR_LOAD
local args = {...}
local targetCharge = 0
--END VAR_LOAD

--FUNC_DEFINE
function Intro()
	print ("--------")
	print ("Charging Program - Developed by UltraNoobian")
	print ("Using version: " .. version)
	print ("--------")
end
function termReset()
	term.clear()
	term.setCursorPos(1,1)
end
-->> termReset() is never called.  Either remove completely or integrate int Intro()

function getVariables()
	if args and #args == 1 then
	-->> Because args is defined as {...} it will always exist as a table.  Remove "args and" as it is redundant.
		if tonumber(args[1]) ~= nil and tonumber(args[1]) >= 1 then
			targetCharge = tonumber(args[1])
			print("Valid Charge: " .. args[1])
		elseif tonumber(args[1]) ~= nil and tonumber(args[1]) < 1 then
			print("That is not a positive charge. Try again.")
			error()
		elseif tonumber(args[1]) == nil then
			print("Not a number, Please enter a valid number next time you run this program~!")
			error()
		end
		-->> Move the handler for tonumber(args[1]) == nil to the top, then you do not need to check for it for the other 2 cases.
	elseif args and #args > 1 then
		print("Invalid number of arguements, We only need the charge level!")
		error()
		-->> use this:  error("Invalid number of arguments.", 0)
		-->> The 2nd parameter, 0, tells Lua to exit with an error, but not provide a line number.
		-->> or do what most Lua programs do; simply ignore the extra parameters.
	else
		print("")
		print("What Charge level would you like to achieve?")
		testCharge = read()
		-->> You never use the string form of testCharge.  Go ahead and use:
		-->> testCharge = tonumber(read())
		-->> This cleans up later code a lot.

		if tonumber(testCharge) ~= nil and tonumber(testCharge) >= 1 then
        -->> Replace with:  if testCharge and testCharge >= 1 then
			targetCharge = tonumber(testCharge)
			print("Valid Charge: " .. targetCharge)
		elseif tonumber(testCharge) ~= nil and tonumber(testCharge) < 1 then
			print("That is not a positive charge. Try again.")
			error()
		elseif tonumber(testCharge) == nil then
			print("Not a number, Please enter a valid number next time you run this program~!")
			error()
		end
		-->> Same concept as above.
	end
end
function printVariables()
	print("")
	print("---NOTICE---")
	print("Your current charge level is " .. turtle.getFuelLevel())
	print("You are charging to " .. targetCharge .. ".")
	print("Press any key to continue...")
	read()
end
function chargeFull()
	if turtle.getFuelLevel() > targetCharge then
		return true
	else
		return false
	end
end
-->> if then else is redundant.  Comparisons return a boolean already.  Use:
-->> return turtle.getFuelLeve() > targetCharge

function tryBurn(slotID)
	while turtle.getItemCount(slotID) > 0 do
		if turtle.refuel(1) == false or chargeFull() then
			break
		end
	end
end
function inventoryBurn()
	print("Inventory burn commencing...")
	print("")

	for slotID = 1,16 do
		turtle.select(slotID)
		lineUpdate("Currently attempting to burn slot " .. slotID .. ".")
		tryBurn(slotID)

		if chargeFull() then
			print("")
			print("Your turtle has reached it's target charge level!!")
			read()
			break
		end
	end
end
function burnCoolDown()
	print("")
	print("Begining idle phase, Now is the time to put in fuel...")
	print("")
	for count = 30,0,-1 do
		lineUpdate("Next Inventory Burn in " .. count .. " seconds")
		sleep(1.0)
	end
	print("")
end
-->> burnCoolDown() calls lineUpdate() before it is declared.  You need to move the lineUpdate() function above burnCoolDown() so it exists when burnCoolDown() is compiled.

function lineUpdate(string)
	local currX,currY = term.getCursorPos()
	term.clearLine()
	term.setCursorPos(1,currY)
	write(string)
end
--END FUNC_DEFINE

--MAIN

Intro()
getVariables()
printVariables()

while not chargeFull() do
	inventoryBurn()
	if not chargeFull() then
		burnCoolDown()
	end
end

print("")
print("This program will now terminate.... Thank you for using UltraNoobian's Programs")

What is the point of making a function and then calling it once? Right, for me a function is something you use to shorten your code. So if you have a code snippet which you use repeatedly, or it's a loong snippet and you call it 2 or 3 times, then you make a function out of it.

Also I want to mention that there is almost always space for improvent. But it really depends on coding styles, and nobody has the same habit as the other.

And finally, you can ask yourself a few questions to figure out if it's good enough:
Does this what it's supposed to do?
Any small improvements for optimization?
Is all code purely for my goal that I want to reach?
And so on.

Engineer, on 27 September 2013 - 07:38 PM said:

What is the point of making a function and then calling it once?

One common use for functions is to separate bits of code for readability. Take for instance the "main" function in many applications. The entirety of the main loop is usually contained within that function, and it is only called once, from the bottom of the app. A "setup" function is used the same way. I assume that is what the OP was doing here.

I disagree so f**king much with you. Functions don't make it readable, it makes you scroll up to see what that function does. So it does not add to readability at all!

And again, you shouldn't use functions to call it once, because it's pointless. If you only use functions when you use something 2 or more times, then a function is okay. Otherwise, it would make you scrolling up to see what that function does, and again, it doesn't add ANYTHING to readability!
You have hit a nerve.

Engineer, on 28 September 2013 - 05:05 AM said:

I disagree so f**king much with you. Functions don't make it readable, it makes you scroll up to see what that function does. So it does not add to readability at all!

And again, you shouldn't use functions to call it once, because it's pointless. If you only use functions when you use something 2 or more times, then a function is okay. Otherwise, it would make you scrolling up to see what that function does, and again, it doesn't add ANYTHING to readability!
You have hit a nerve.

Then you're naming your functions wrong. You should not have to scroll to a function to see what it does, the name of the function should be concise yet descriptive enough that you know exactly what it is.

I whole-heartedly agree with what BigTwisty said, functions can made for a lot more readable code, large slabs of code are far less readable than several function calls to adequately named functions. Also one of the easiest ways to develop something is by applying functional decomposition to a problem. Functional decomposition essentially ends up having only 1 task per function, if there is more than approx 15-30 lines to a function you should consider breaking it up further until you no longer can.

Your point of "you shouldn't use functions to call it once, because it's pointless" is a bit of a (to put it nicely) silly comment, how do you know what code you're only going to ever use once? You don't, a future version or expansion upon your program could very well use that function in another place. It is much better to break individual features or tasks into their own function just incase you want to use it again later rather than having to try reorganise your code at a later stage. If you haven't already noticed, quite a lot of my programs and APIs I actually need to type very little of. why? because I have everything broken into functions, so if I need to write code to perform a certain task, instead of writing it again, I just go to a previous project and copy/paste the function.

A function is not designed to shorten code. A function allows us to think of our program as a bunch of sub-steps, each sub-step can be its own function and when any program or sub-step seems too hard, just break it down further into sub-steps! (this is functional decomposition!). Functions allow us to reuse code instead of rewriting it. Functions are very good at allowing us to test small parts of our program in isolation from the rest of the program. And of course functions allow us to keep our variable namespace clean (since local variables only "live" as long as the function does).

If you avoid using functions simply so you don't have to scrolling your IDE/Text Editor then you're doing something wrong, and you should also consider investigating a feature in any editor that does code called "code folding"

theoriginalbit, on 28 September 2013 - 09:40 AM said:

Then you're naming your functions wrong. You should not have to scroll to a function to see what it does, the name of the function should be concise yet descriptive enough that you know exactly what it is.

BigTwisty was talking about a function called main. What does main do then?

theoriginalbit, on 28 September 2013 - 09:40 AM said:

I whole-heartedly agree with what BigTwisty said, functions can made for a lot more readable code, large slabs of code are far less readable than several function calls to adequately named functions. Also one of the easiest ways to develop something is by applying functional decomposition to a problem. Functional decomposition essentially ends up having only 1 task per function, if there is more than approx 15-30 lines to a function you should consider breaking it up further until you no longer can.

But then, why if you have a function, and you call it once. No, that does not at readability at all, if you are done with that section of the code, feel free to have blank line under it and then continue to code. I do agree though that one long code should be broken up, by whitelines, not functions.

theoriginalbit, on 28 September 2013 - 09:40 AM said:

Your point of "you shouldn't use functions to call it once, because it's pointless" is a bit of a (to put it nicely) silly comment, how do you know what code you're only going to ever use once? You don't, a future version or expansion upon your program could very well use that function in another place. It is much better to break individual features or tasks into their own function just incase you want to use it again later rather than having to try reorganise your code at a later stage. If you haven't already noticed, quite a lot of my programs and APIs I actually need to type very little of. why? because I have everything broken into functions, so if I need to write code to perform a certain task, instead of writing it again, I just go to a previous project and copy/paste the function.

Well if you have like in the OP's code, an intro(duction) function, then you are going to call it once, because it is a introduction. So there is no need for that function because you show it once in the program!

Well, if you have such an API, it is modular. I mean, you make wrappers around other functions that does stuff for you, it is good to do that though, dont get me wrong there. But I really, really hate to see just a function and called once in the whole code. If you are going to add features, do it smart, make that modular as well. Make some sort of API to interact with the main program, but not every function be declared and only called once, there is no point in that. Like with a project of mine, I have a checking tree:

local tree = {
	["folder"] = {
	   "screenUtils"; "otherCrap"; --etc.
	["NotAFolder"] = true;
}

I made it so you only have to extend that one table, and it will do all the work for you. That is because the checking isnt hardcoded, this tree is hardcoded. I really prefer that kind of style of programming, but thats me I guess.

theoriginalbit, on 28 September 2013 - 09:40 AM said:

A function is not designed to shorten code. A function allows us to think of our program as a bunch of sub-steps, each sub-step can be its own function and when any program or sub-step seems too hard, just break it down further into sub-steps! (this is functional decomposition!). Functions allow us to reuse code instead of rewriting it. Functions are very good at allowing us to test small parts of our program in isolation from the rest of the program. And of course functions allow us to keep our variable namespace clean (since local variables only "live" as long as the function does).

But then you are using a function just like goto, which I have heard is bad. But my preference, is to have as less functions as possible. Otherwise every program out there will look like:

-- All function declared
while true do
   main()
end

And I believe that is silly. Why don't you put the main function just into the loop anyway…

theoriginalbit, on 28 September 2013 - 09:40 AM said:

If you avoid using functions simply so you don't have to scrolling your IDE/Text Editor then you're doing something wrong, and you should also consider investigating a feature in any editor that does code called "code folding"

I have to admit that reason was bit silly :P/>

Hello everyone,

First off, I would like to thank everyone that replied to this thread and contributed their criticism and advice regarding my style of coding.
I am really grateful since I am relatively new to Lua. There seems to be issues about my particular style of coding that doesn't seem to be as well accepted by everyone but that's okay, everyone has their view on the 'correct' way of programming.

So lets first address some facts initially pointed out by BigTwisty.

		-->> Because args is defined as {...} it will always exist as a table.  Remove "args and" as it is redundant.

• Did not know that, but should have realized.

Comparisons return a boolean

• Didn't know that either, saves me a few lines of code.

-->> burnCoolDown() calls lineUpdate() before it is declared.  You need to move the lineUpdate() function above burnCoolDown() so it exists when burnCoolDown() is compiled.

• It didn't seem to matter? Does it really need to be organized like that? I thought alphabetical would have been better.

Feel free to reply to my remarks about those.


====================
Topic 2. Functions

One of the things I have noticed with my Lua programming is that I see that I have used a lot of set notions I have used languages, like in C#. I seem to have developed a sense of 'hiding' the code wherever possible, leading to single-use functions that Programmer's Notepad recognizes as a collapsible block of code. Normally for C#, you would use something like the #region directive (C# Reference) to tell the IDE to trigger the outlining feature.

That explains a small portion of why I use run-once functions, simply because they make it slightly more 'readable' but also just happen to hide a portion of the code in a summary of what it does i.e. printVariable() from line 56-63. I could understand if you're mad at me for writing run-once functions but if I gave them meaningful names, then It becomes more readable.

Intro -> getVariables -> printVariables -> <Main Sequence> -> exit()

In addition, I actually have an API called eTurt with Intro() that I usually include with the programs that I have programmed, I just didn't include it because most of it actually contains nothing relevant to this new program except for one function. (Total programs that call Intro() is about 7, I believe)
But I totally agree with you, If I did this for the sake of just want to have clean collapsible code (which I did), Then I am a fool and live up to the name.

I am however interested in that checking tree, Tell me more….

local tree = {
		["folder"] = {
		   "screenUtils"; "otherCrap"; --etc.
		["NotAFolder"] = true;
}

On the topic of how many functions you should have though, I agree with theoriginalbit. I prefer to break it down as much as possible without reducing readability.
I visualise code a tree, You may have many many branchs and leaves but it boils down to the trunk that is holding the whole thing together, main(). The branches are functions, if they are healthy, then you don't need to know what is behind them. Likewise the smaller branches do not need to know about how leaves work, they just need to produce energy via photosynthesis (Processing), and the branches supply the nutrients (Variables).

But thank you again for commenting on my code, I expect even more criticism soon.


PS. functions do shorten code if you use the function more than once.

Engineer, on 28 September 2013 - 10:13 AM said:

But then you are using a function just like goto, which I have heard is bad.

Do you understand WHY using a goto is bad? A goto statement moves the next executing line pointer to another point in the code, but DOES NOT RETAIN any information on where it was called from. This creates a needlessly complicated flow through the code, leading to confusion and much difficulty when trying to find bugs. In this case, the functions are being used more like a gosub, which moves the next executed line pointer but retains the call point, allowing code continuation from that point when the subroutine is finished.

About that checking tree, it isnt necessarily a checking tree. It is just a simple http://lua-users.org/wiki/TablesTutorial"]table where you loop over with some logic. Here is that for the checking thing:

--# This is my filetree:
local installation = {
	[""] = {
		"projects"
	}; --# Current directory
	["k/"] = true;
	["shizzle/"] = true;
	["folder/"] = {
		"file"
	}
}

local installation_complete = true
for folderName, files in pairs( installation ) do
	if fs.exists( cFolder .. folderName ) and fs.isDir( cFolder .. folderName ) then
		logf( "Folder exists: %s.",  cFolder .. folderName )
		if type( files ) == "table" then
			for _, fileName in pairs( files ) do
				if not fs.exists( cFolder .. folderName .. fileName ) then
					logf( "\t- File does not exist: %s.", fileName )
					installation_complete = false
				elseif fs.isDir( cFolder .. folderName .. fileName ) then
					logf( "\t- Folder is supposed to be a file: %s.", fileName )
					installation_complete = false
				elseif fs.exists( cFolder .. folderName .. fileName ) then
					logf( "\t- File exists: %s.", fileName )
				end
			end
		end
	else
		logf( "Folder does not exist: %s.", cFolder .. folderName )
		installation_complete = false
	end
end

Not that there are tons of undefined variables, as I copied this over from a project. But the point is just some good logic!