Help making a method more concise

Ruby
1

Hello,

I’m looking to improve my skills as a Rubyist and would like to know if
the
“depth” method could be expressed more precisely. Any help would be
greatly
appreciated, thanks!

require ‘test/unit’

class Node
attr_accessor :value, :lchild, :rchild

def depth
[lchild ? lchild.depth : 0, rchild ? rchild.depth : 0].max + 1
end
end

class NodeTest < Test::Unit::TestCase
def setup
@n = Node.new
end

def test_depth_of_one
@n.lchild = Node.new
@n.lchild.lchild = Node.new
@n.rchild = Node.new
assert @n.depth == 1, “depth of node should be one, was #{@n.depth}”
end

def test_depth_of_two
@n.lchild = Node.new
@n.rchild = Node.new
assert @n.lchild.depth == 1, “depth of lchild should be one”
assert @n.rchild.depth == 1, “depth of rchild should be one”
assert @n.depth == 2, “depth of tree should be two, was #{@n.depth}”
end
end

–

Mark
[email protected]
[email protected]

2

On 02.05.2011 20:17, Mark H. wrote:

 [lchild ? lchild.depth : 0, rchild ? rchild.depth : 0].max + 1

end
end

I think it’s pretty OK that way. You could get rid of a bit of
redundancy but whether that actually makes the program better? Judge
for yourself:

def depth
[lchild, rchild].map {|ch| ch ? ch.depth : 0}.max + 1
end

def depth
d = 0

[lchild, rchild].each do |ch|
d = ch.depth if ch && ch.depth > d
end

d + 1
end

def depth
d = 0

[lchild, rchild].each do |ch|
d = [d, ch.depth].max if ch
end

d + 1
end

Kind regards

robert

3

Thanks Robert, I think I’ll go with this one:

def depth
[lchild, rchild].map {|ch| ch ? ch.depth : 0}.max + 1
end

On Mon, May 2, 2011 at 11:30 AM, Robert K.
[email protected]wrote:

class Node

d + 1

–

Mark H.
[email protected]
[email protected]

4

Hey Josh,

Oops … I see that my test case was incorrect. It should’ve been:

def test_depth_of_one
@n.lchild = Node.new
@n.lchild.lchild = Node.new
@n.rchild = Node.new
assert @n.depth == 3, “depth of node should be one, was #{@n.depth}”
end

Thanks for bringing that to my attention!

On Mon, May 2, 2011 at 12:55 PM, Josh C. [email protected]
wrote:

require ‘test/unit’
def setup
def test_depth_of_two
Mark
[email protected]
[email protected]

First test fails, with a value of 3.
3 seems like the correct value, your test may be wrong.

–

Mark H.
[email protected]
[email protected]

5

On Mon, May 2, 2011 at 1:17 PM, Mark H. [email protected] wrote:

attr_accessor :value, :lchild, :rchild

assert @n.lchild.depth == 1, “depth of lchild should be one”

First test fails, with a value of 3.
3 seems like the correct value, your test may be wrong.

6

On Mon, May 2, 2011 at 11:17 AM, Mark H. [email protected] wrote:

def depth
[lchild ? lchild.depth : 0, rchild ? rchild.depth : 0].max + 1
end
end

def depth
1 + [lchild,rchild].map {|ch| [ch}.max
end

or even:

def depth
1 + [lchild,rchild].map(&:to_i).max
end

alias to_i depth

or:

add empty (no value) left & right children when you first add a value
to a node, and:

def empty?
@value.nil?
end

def depth
empty? ? 0 : [lchild,rchild].map(&:depth).max
end

7

On Tue, May 3, 2011 at 6:28 AM, Christopher D. [email protected]
wrote:

attr_accessor :value, :lchild, :rchild

def depth
[lchild ? lchild.depth : 0, rchild ? rchild.depth : 0].max + 1
end
end

def depth
1 + [lchild,rchild].map {|ch| [ch}.max
end

Does not work: apart from the syntax invocation of #depth is missing.
If we add that we have a solution that has been proposed already.

or even:

def depth
1 + [lchild,rchild].map(&:to_i).max
end

alias to_i depth

Now that’s an interesting idea to use the knowledge that nil.to_i => 0!

empty? ? 0 : [lchild,rchild].map(&:depth).max
end

I don’t think this captures the original semantics properly. Now
there are only two states: empty, not empty. But the original design
allowed for more states: empty, left set, right set, both set. Even
if not for #depth this is likely important for other tree algorithms.

Kind regards

robert

8

On Tue, May 3, 2011 at 12:30 AM, Robert K.
[email protected] wrote:

class Node
end

Does not work: apart from the syntax invocation of #depth is missing.
If we add that we have a solution that has been proposed already.

Uh, yeah. I messed something up in editing that. I’m not entirely sure
what it was supposed to be.

empty? ? 0 : [lchild,rchild].map(&:depth).max
end

I don’t think this captures the original semantics properly. Now
there are only two states: empty, not empty. But the original design
allowed for more states: empty, left set, right set, both set. Even
if not for #depth this is likely important for other tree algorithms.

Yeah, it depends on things in the code we can’t see; most of the tree
implementations I’ve seen either use nil pointers for children and
populate them with real nodes when they get data for them, or use
empty nodes for children only of populated nodes and then populate the
empty nodes when data arrives for them, but its possible that this one
needs more differentiation.