Howdy,
I took a look at your code more as a curiosity of how ruby is being used in LinuxMCE than in actually using the feature. I thought I'd give you a few comments in a friendly code review style.
* you might consider using rdoc style comments. That will make publishing the API easier.
* naming choices are good, long names
* mixed constant naming, sometimes all uppercase, sometimes mixed (camel) case (ex: NotReady = 1). Convention is constant object references should be all uppercase with underscores. Camelcase is used for class and module names.
* a few methods are a little high on the line count, but mostly because of all the good logging calls.
* Your log() methods have me a little confused. Why all the putc calls? Wouldn't something like the following accomplish the same?
def log(line)
File.open("/var/log/pluto/" + device_.devid_.to_s + "_Generic_Serial_Device.log", "a") do |logfile|
logfile.puts '(***)' + line.to_s.gsub('<',' ')
end
end
* in several debug methods you use this construct:
work = label + ":"
for c in 1..text.length
work += padhex("%X" %text[c-1]) + ' '
end
string concatenations are usually costly. It might be better to append to an array then join to a string:
array = []
for c in 1..text.length
array << padhex("%X" %text[c-1])
end
work = label + ':' + array.join(' ')
but then again, these are in debug methods so performance is probably not an issue. :-)
* in gsdcommand() I'm not sure what the return value will be when the case statement hit's one of the when statements that don't have a block. For example, what will be returned when @gsdcommand is 'REPORT ONLY ON PULSE'? You might need an extra return statement at the bottom of the method.
* Sometimes the really long cases can be replaced with a hash lookup. For example, in gsdcommand, you may want to play with something like:
FUNCTIONS = {'ALL UNIT OFF' => Proc.new {@gsdparams[10] = '0';return 48},
'ALL LTS ON' => Proc.new {@gsdparams[10] = '1';return 48},
#...
}
def gsdcommand #returns DCE EVENT!! and compiles params
p = FUNCTIONS[CommandFunctions[@gsdcommand]]
p.call unless p.nil?
return 0
end
But then again, the big case statement is probably more readable. :-)
Overall, very nice looking code. :-)
Have fun,
Roy
Roy,
Hello and Thanks for the review!
This is my SECOND attempt in ERb and I'm still learning the language..
RDOC style Comments: Don't know that yet. will look into it
(I have a book on it, but haven't read that chapter yet)
Naming Conventions:
I wasn't aware of that: Ruby's naming conventions are based on the first character..
Yes, I know a few methods are a bit high on the line count
half of those lines are logging and comments. (which can be taken out)
regarding my log():
ok, I originally wrote the log() routine when I started ruby..
I ran into problems displaying XML in the log.
Then, later, I ran into problems displaying Object.inspect in the log..
out of much frustration, that is what I ended up with, and as long as it worked (ie displayed what I needed it to) I left it.
ERb (at least in linuxmce) doesn't like displaying <object>. I tries to parse it.
Even in the code window, when you want to make a String='<?xml version="1.0"?>', when you save it, you get String=''
The debug calls are just them. debug. they can be removed.
That's an interesting way to use a hash! I like that!
As far as that long case statement.. Yeah, I should add a return at the end.
Most of those functions aren't needed.. but I copied them in from the specs, so they won't hurt.
I've been using Ruby for about 2-3 months now..
Nice to see some feedback!
Regards,
Dan
