anonym wrote (26 Jul 2013 12:49:36 GMT) :
> Hi,
> branch: test/firewall-check-tag
> ticket: Feature #5644 <https://labs.riseup.net/code/issues/5644>
This:
> @mac = REXML::Document.new(@net.xml_desc).elements['network/ip/dhcp/host/'].attributes['mac']
in the Sniffer class, looks like broken encapsulation (see my review
of test/reorg for details + a proposal about a similar situation).
> - @bridge_name = bridge_name
> - @mac = mac
> + @vmnet = vmnet
> + @net = @vmnet.net
> + @bridge_name = @net.bridge_name
> + @mac = REXML::Document.new(@net.xml_desc).elements['network/ip/dhcp/host/'].attributes['mac']
It's not clear to me why @vmnet and @net are added as an instance
attributes, since they are apparently not used anywhere else in the
class or by its users. Perhaps it's not worth making the class
interface & amount of maintained state larger, if all we need in
practice is a way for some methods to get the MAC address and
bridge name?
> def assert_no_leaks
Now that this isn't a step definition anymore, but an instance method,
I don't think `puts' is the optimal way to report detailed information
before throwing an exception. How about storing the detailed text in
a local variable, in each of the `if' blocks, and then passing this
additional information to `raise'?
It seems to me 48a32dd introduces a dependency on @custom_sniffer,
while this attribute is only created by the next commit.
This breaks bisection.
That's all for today :)
> This branch depends on test/reorg.
Once that other one has its own ticket, #5644 can be marked as blocked
by it.
Cheers!
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
