• Post Reply Bookmark Topic Watch Topic
  • New Topic

Three almost identical methods to one  RSS feed

 
Scotty Steven
Ranch Hand
Posts: 80
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have three methods that are almost 100% idential except for one line. I'm trying to figure out how to reduce this to one method, if at all possible, but I'm banging my head and pulling hair trying to figure out how. Any suggestions?


 
Bear Bibeault
Author and ninkuma
Marshal
Posts: 66306
152
IntelliJ IDE Java jQuery Mac Mac OS X
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
While I agree that code repetition is something best avoided, so is jamming together functionality.

If the goal is to avoid the code repetition, can you think of another way to achieve that other than reducing the number of methods?

If your goal is to simply reduce the number of methods, why?
 
Scotty Steven
Ranch Hand
Posts: 80
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
To be honest, I have no reason for wanting to do this other then it looked like something that could (or should?) be done. It was more of a gut feeling this really that I wanted to use for my own educational purposes. An attempt to grow and improve, if you will. If I'm best to leave alone, then that too is educational.
 
Bear Bibeault
Author and ninkuma
Marshal
Posts: 66306
152
IntelliJ IDE Java jQuery Mac Mac OS X
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Scotty Steven wrote:To be honest, I have no reason for wanting to do this other then it looked like something that could (or should?) be done.

Could? Sure. Should? Not so much.

You've got three methods that, through similar, do different things. How would collapsing them into one method improve code clarity?

It was more of a gut feeling this really that I wanted to use for my own educational purposes. An attempt to grow and improve, if you will.

That's good. Very good. And, your gut is correct. The code can be improved. But I would argue that collapsing the methods is not the best approach.

If I'm best to leave alone, then that too is educational.

If you look at the code, each method has the exact same code in each of their last 4 lines. What can you do about that?
 
Scotty Steven
Ranch Hand
Posts: 80
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I came up with the following. Much cleaner, in my opinion. I really don't see any way of reducing them any further. Thanks for the help/advice.

 
Campbell Ritchie
Marshal
Posts: 56546
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Don't use \n because it probably does not give you the correct line end for your operating system. To print two blank lines try System.out.printf("%n%n");
Don't us println(""). Use System.out.println(); which simply moves to the next line with the correct line end.

Apart from the fact that printNext() is not a good name for that method, that does look a lot better. You correctly used object‑orientation, putting those three iterator methods in the tree class.

But:   Why are the methods private and static? If they are private, they are only accessible inside your class. If they are inside the class only, the tree must be a member of that class, and ought to be an instance field. So why have you made those methods static?
 
Chan Ag
Rancher
Posts: 1090
14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Scotty Steven wrote:I have three methods that are almost 100% idential except for one line. I'm trying to figure out how to reduce this to one method, if at all possible, but I'm banging my head and pulling hair trying to figure out how. Any suggestions?
................


Also like already stated, the three methods are very different; they give you different results. It is important to also understand what those three methods accomplish. Generally with algorithms, we first understand the algorithm ( I like to draw the trees and nodes on a piece of paper ) and then work on/try to understand its implementation.

Algorithms like a pre order, post order, or an in order traversal are standard algorithms. They can be easily found on google. And some of these links are quite good. Not sure if you have tried it already, or if you already know about the three kinds of traversals ( ignore this post if you do ), just I feel it is good to first understand the algorithm even before you look at any code that implements these algorithms.

Chan.
 
Campbell Ritchie
Marshal
Posts: 56546
172
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I presume the algorithms are implemented by the Iterators.
You can improve the code by making the titles, e.g. “Post‑Order” fields of the Iterators.
 
Scotty Steven
Ranch Hand
Posts: 80
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:

But: Why are the methods private and static? If they are private, they are only accessible inside your class. If they are inside the class only, the tree must be a member of that class, and ought to be an instance field. So why have you made those methods static?


The truth is, I never gave it much thought. These methods were extracted from a larger method. The program duplicated these lines, so I used Eclipse to extract them into their own methods and figuring that Eclipse knows java way better then I, never questioned it.

As for the \n, I removed it and placed it elsewhere. My understanding of java is System.out.println(""); and System.out.println(); are treated exactly the same at compiler time. I find System.out.println(""); easier to read and use it purely as a personal preference.
 
Campbell Ritchie
Marshal
Posts: 56546
172
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Let's try it.javap -c PrintDemo
No, the two calls are different. Obviously you will not notice that difference in the output.
Never make anything static without a good explanation. eclipse noticed that you are not using any instance members, so it said, “might be declared static”. But you don't have to believe it. There is no need to pass something as an argument which is available as an instance field, in which case Eclipse would not have suggested making the method static.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!