Skip to main content
Corrected code according to comment
Source Link
holroy
  • 11.8k
  • 1
  • 27
  • 59
class TreeNode { TreeNode left; TreeNode right; TreeNode(TreeNode left, TreeNode right) { this.left = left; this.right = right; } } /* Count all nodes having one leaf, and one non leaf node */ static int leafNoLeafCount(TreeNode node) { if (node == null) { return 0; } return leafNoLeafCount(node.left) + leafNoLeafCount(node.right) + (isLeafNoLeafNode(node) ? 1 : 0); } /* Does this node exists, and have one internal and one external child node? */ static boolean isLeafNoLeafNode(TreeNode node) { if (node == null && node.right != null && node.left != null){ return false; } boolean leftLeaf; boolean rightLeaf; // Not a big deal, but only check for leaf nodes once // once for any given node leftLeaf = isLeaf(node.left); rightLeaf = isLeaf(node.right); return (leftLeaf && !rightLeaf) || (!leftLeaf && rightLeaf); } /* Verify node exists, and has no child nodes */ public static boolean isLeaf(TreeNode node) { return node != null && node.left == null && node.right == null; } 

To me it is now a little easier to see what is actually happening, and it's a little more failsafe as the test for a null node is done in all methods, and there are no redundant methods.

Update: I was made aware that the isLeafNoLeaf() could allow a null to pass as an external node, which is not correct. Added a test at start to correct this behavior. This also triggers a question on what is correct behavior for isLeaf() when receiving a null node: Should it cast an exception if null, or silently ignore it and return false?

class TreeNode { TreeNode left; TreeNode right; TreeNode(TreeNode left, TreeNode right) { this.left = left; this.right = right; } } /* Count all nodes having one leaf, and one non leaf node */ static int leafNoLeafCount(TreeNode node) { if (node == null) { return 0; } return leafNoLeafCount(node.left) + leafNoLeafCount(node.right) + (isLeafNoLeafNode(node) ? 1 : 0); } /* Does this node exists, and have one internal and one external child node? */ static boolean isLeafNoLeafNode(TreeNode node) { if (node == null){ return false; } boolean leftLeaf; boolean rightLeaf; // Not a big deal, but only check for leaf nodes once // once for any given node leftLeaf = isLeaf(node.left); rightLeaf = isLeaf(node.right); return (leftLeaf && !rightLeaf) || (!leftLeaf && rightLeaf); } /* Verify node exists, and has no child nodes */ public static boolean isLeaf(TreeNode node) { return node != null && node.left == null && node.right == null; } 

To me it is now a little easier to see what is actually happening, and it's a little more failsafe as the test for a null node is done in all methods, and there are no redundant methods.

class TreeNode { TreeNode left; TreeNode right; TreeNode(TreeNode left, TreeNode right) { this.left = left; this.right = right; } } /* Count all nodes having one leaf, and one non leaf node */ static int leafNoLeafCount(TreeNode node) { if (node == null) { return 0; } return leafNoLeafCount(node.left) + leafNoLeafCount(node.right) + (isLeafNoLeafNode(node) ? 1 : 0); } /* Does this node exists, and have one internal and one external child node? */ static boolean isLeafNoLeafNode(TreeNode node) { if (node == null && node.right != null && node.left != null){ return false; } boolean leftLeaf; boolean rightLeaf; // Not a big deal, but only check for leaf nodes once // once for any given node leftLeaf = isLeaf(node.left); rightLeaf = isLeaf(node.right); return (leftLeaf && !rightLeaf) || (!leftLeaf && rightLeaf); } /* Verify node exists, and has no child nodes */ public static boolean isLeaf(TreeNode node) { return node != null && node.left == null && node.right == null; } 

To me it is now a little easier to see what is actually happening, and it's a little more failsafe as the test for a null node is done in all methods, and there are no redundant methods.

Update: I was made aware that the isLeafNoLeaf() could allow a null to pass as an external node, which is not correct. Added a test at start to correct this behavior. This also triggers a question on what is correct behavior for isLeaf() when receiving a null node: Should it cast an exception if null, or silently ignore it and return false?

Reformatted... The bold was a little to intense...
Source Link
holroy
  • 11.8k
  • 1
  • 27
  • 59
  • All methods are private?All methods are private? – I'm a bit rusty in my Java, but this does seem kind of strange. OK, that they are static or class methods, as they don't rely on internal mechanisms, but private?

  • Comments are slightly meaninglessComments are slightly meaningless – That the isWantedNode checks to see if it is a wanted node, is kind of obvious. It would be better having a comment stating something like "A wanted node has one internal child node and one external (or leaf) child node".

  • Very fond of ternary operations, are you?Very fond of ternary operations, are you? – Ternary can be a good thing, but too much of them can also be misleading and slightly confusing.

  • isInternal() and isLeaf() are negated version of each otherisInternal() and isLeaf() are negated version of each other – Any leaf is not internal, and vice versa. Could use one method, or if wanting both one of them could depend on the other.

  • Avoiding method calls are not always neededAvoiding method calls are not always needed – Having to call an extra set of functions to make the code look clearer is not always a bad thing. Calling methods which would trigger multiple (repeated) calls down the entire tree, that is another story. Compiler nowadays are quite efficient related to knowing when they can inline methods and so on.

  • Static or not?!Static or not?! – Why is the TreeNode class static? Shouldn't that be instantiated, and just left as a normal class?

  • No payload in your TreeNodeNo payload in your TreeNode – The TreeNode is not actually able to carry anything, which is kind of strange, but understandable for an exercise. But normally it would have an int or a string or something so that it had some meaning besides a tree building construct.

  • Don't be lazy when naming variablesDon't be lazy when naming variables – Don't use one letter variables, unless possibly the counters like i, j, and so on. There is no need, and only adds to the confusion to use n, l, and r instead of node, left and right.

  • Naming methods is hard!Naming methods is hard! – strangeCount() and isWantedNode() handles the count and test for the same thing, and should be named similarly, I think. Not sure if it's a better name, but the wanted nodes does one leaf node, and one none leaf node, so I used the leafNoLeaf in the code below.

  • All methods are private? – I'm a bit rusty in my Java, but this does seem kind of strange. OK, that they are static or class methods, as they don't rely on internal mechanisms, but private?

  • Comments are slightly meaningless – That the isWantedNode checks to see if it is a wanted node, is kind of obvious. It would be better having a comment stating something like "A wanted node has one internal child node and one external (or leaf) child node".

  • Very fond of ternary operations, are you? – Ternary can be a good thing, but too much of them can also be misleading and slightly confusing.

  • isInternal() and isLeaf() are negated version of each other – Any leaf is not internal, and vice versa. Could use one method, or if wanting both one of them could depend on the other.

  • Avoiding method calls are not always needed – Having to call an extra set of functions to make the code look clearer is not always a bad thing. Calling methods which would trigger multiple (repeated) calls down the entire tree, that is another story. Compiler nowadays are quite efficient related to knowing when they can inline methods and so on.

  • Static or not?! – Why is the TreeNode class static? Shouldn't that be instantiated, and just left as a normal class?

  • No payload in your TreeNode – The TreeNode is not actually able to carry anything, which is kind of strange, but understandable for an exercise. But normally it would have an int or a string or something so that it had some meaning besides a tree building construct.

  • Don't be lazy when naming variables – Don't use one letter variables, unless possibly the counters like i, j, and so on. There is no need, and only adds to the confusion to use n, l, and r instead of node, left and right.

  • Naming methods is hard! – strangeCount() and isWantedNode() handles the count and test for the same thing, and should be named similarly, I think. Not sure if it's a better name, but the wanted nodes does one leaf node, and one none leaf node, so I used the leafNoLeaf in the code below.

  • All methods are private? – I'm a bit rusty in my Java, but this does seem kind of strange. OK, that they are static or class methods, as they don't rely on internal mechanisms, but private?

  • Comments are slightly meaningless – That the isWantedNode checks to see if it is a wanted node, is kind of obvious. It would be better having a comment stating something like "A wanted node has one internal child node and one external (or leaf) child node".

  • Very fond of ternary operations, are you? – Ternary can be a good thing, but too much of them can also be misleading and slightly confusing.

  • isInternal() and isLeaf() are negated version of each other – Any leaf is not internal, and vice versa. Could use one method, or if wanting both one of them could depend on the other.

  • Avoiding method calls are not always needed – Having to call an extra set of functions to make the code look clearer is not always a bad thing. Calling methods which would trigger multiple (repeated) calls down the entire tree, that is another story. Compiler nowadays are quite efficient related to knowing when they can inline methods and so on.

  • Static or not?! – Why is the TreeNode class static? Shouldn't that be instantiated, and just left as a normal class?

  • No payload in your TreeNode – The TreeNode is not actually able to carry anything, which is kind of strange, but understandable for an exercise. But normally it would have an int or a string or something so that it had some meaning besides a tree building construct.

  • Don't be lazy when naming variables – Don't use one letter variables, unless possibly the counters like i, j, and so on. There is no need, and only adds to the confusion to use n, l, and r instead of node, left and right.

  • Naming methods is hard! – strangeCount() and isWantedNode() handles the count and test for the same thing, and should be named similarly, I think. Not sure if it's a better name, but the wanted nodes does one leaf node, and one none leaf node, so I used the leafNoLeaf in the code below.

Source Link
holroy
  • 11.8k
  • 1
  • 27
  • 59

Lets review some of the coding style firstly:

  • All methods are private? – I'm a bit rusty in my Java, but this does seem kind of strange. OK, that they are static or class methods, as they don't rely on internal mechanisms, but private?

  • Comments are slightly meaningless – That the isWantedNode checks to see if it is a wanted node, is kind of obvious. It would be better having a comment stating something like "A wanted node has one internal child node and one external (or leaf) child node".

  • Very fond of ternary operations, are you? – Ternary can be a good thing, but too much of them can also be misleading and slightly confusing.

  • isInternal() and isLeaf() are negated version of each other – Any leaf is not internal, and vice versa. Could use one method, or if wanting both one of them could depend on the other.

  • Avoiding method calls are not always needed – Having to call an extra set of functions to make the code look clearer is not always a bad thing. Calling methods which would trigger multiple (repeated) calls down the entire tree, that is another story. Compiler nowadays are quite efficient related to knowing when they can inline methods and so on.

And not using methods, would require either using try..catch sequences or long ugly stuff like (for the isWantedNode() left side internal):

 n != null && n.left != null && n.left.left == null && n.left.right == null && n.right != null && (n.right.left != null || n.right.right != null) 

Not very nice or easy to read. Is it?

  • Static or not?! – Why is the TreeNode class static? Shouldn't that be instantiated, and just left as a normal class?

  • No payload in your TreeNode – The TreeNode is not actually able to carry anything, which is kind of strange, but understandable for an exercise. But normally it would have an int or a string or something so that it had some meaning besides a tree building construct.

  • Don't be lazy when naming variables – Don't use one letter variables, unless possibly the counters like i, j, and so on. There is no need, and only adds to the confusion to use n, l, and r instead of node, left and right.

  • Naming methods is hard! – strangeCount() and isWantedNode() handles the count and test for the same thing, and should be named similarly, I think. Not sure if it's a better name, but the wanted nodes does one leaf node, and one none leaf node, so I used the leafNoLeaf in the code below.

Code refactored

The following code is untested, but you'll get the gist of the idea, I hope. I don't have a java compiler available currently.

class TreeNode { TreeNode left; TreeNode right; TreeNode(TreeNode left, TreeNode right) { this.left = left; this.right = right; } } /* Count all nodes having one leaf, and one non leaf node */ static int leafNoLeafCount(TreeNode node) { if (node == null) { return 0; } return leafNoLeafCount(node.left) + leafNoLeafCount(node.right) + (isLeafNoLeafNode(node) ? 1 : 0); } /* Does this node exists, and have one internal and one external child node? */ static boolean isLeafNoLeafNode(TreeNode node) { if (node == null){ return false; } boolean leftLeaf; boolean rightLeaf; // Not a big deal, but only check for leaf nodes once // once for any given node leftLeaf = isLeaf(node.left); rightLeaf = isLeaf(node.right); return (leftLeaf && !rightLeaf) || (!leftLeaf && rightLeaf); } /* Verify node exists, and has no child nodes */ public static boolean isLeaf(TreeNode node) { return node != null && node.left == null && node.right == null; } 

I'm not saying this code is optimal, but I think the comments are slightly better, and I've shaved off some methods. Lastly I removed the multiple calls to isLeaf() and isInternal() which was called 4 times in the original call, with 2 calls to isLeaf().

To me it is now a little easier to see what is actually happening, and it's a little more failsafe as the test for a null node is done in all methods, and there are no redundant methods.