Hi,
anonym wrote (26 Jul 2013 12:46:34 GMT) :
> branch: test/reorg
The general idea looks totally sensible to me, and I'm happy to read
it will bring some performance improvements with it.
Now, let's inspect some of the proposed code in detail, as I have
some concerns.
To start with, I'm sorry if any comment that follows results from my
poor knowledge of Ruby. Also, if some of the issues mentionned are not
regressions, please make it clear so that this branch can be merged
first, and the old problems dealt with later, one step at a time.
Here we go.
In the VMNet class, the clean_up and destroy methods do very similar
things. It's not obvious to me when I should use the one as opposed to
the other. May you please add comments or rdoc or something to explain
how their purpose is different?
Also, it may be the Ruby way to design OO stuff, but having 2 methods
(clean_up and destroy) implicitly depend on a 3rd one having been
successfully run on the instance before looks fragile to me.
Perhaps instead use a read-only accessor (possibly already provided by
attr_reader?) that errors out if the attribute wasn't set yet?
Or simply force the `update' step, that apparently is cricitally
needed for the instance to do anything useful, to run at
construction time?
And while we're at it, once `clean_up' depends on `update' to work
properly, I don't get why we don't use the @net attribute that was
initialized in `update', and instead look it up again by name.
Or perhaps that `net' there isn't the same as the @net
instance attribute?
This:
> net_xml = REXML::Document.new(@vmnet.net.xml_desc)
> @ip = net_xml.elements['network/ip/dhcp/host/'].attributes['ip']
> @mac = net_xml.elements['network/ip/dhcp/host/'].attributes['mac']
in the VM class, looks like broken encapsulation to me: the VM class
shouldn't need to go that far into low-level implementation details of
the @vmnet instance. Perhaps provide helper `ip' and `mac' public
methods in VMNet (with some kind of caching or memoization to avoid
recomputing it every time)?
I look forward to see this improved and merged! :)
Given that this was not a trivial merge, unless you intend to address
all these problems in the next few days, please create a ticket so
that these commits from April do not slip through for months again.
