I've just been having a go at an exercise where I have to create and use a class called Point, with two fields of type double. I have to create some methods for it, one of which is a distanceTo(Point) method, that calculates the distance to another point. I've tried to keep the distanceTo(Point) method short so have created some other methods to use within the method. My question is about the getDistance() method that I've made. As you can see below, I've given it two parameters, which are references to values within two Point objects (this.x and otherPoint.x). Basically, I'm wondering if doing this kind of thing is a good idea or not. The method seems to do its job, but I would like to avoid getting into any bad habits. Any advice welcome.
But just on another note, do you know the distance formula for two points? you should be able to do it all on one line.
Mike Carroll wrote:Basically, I'm wondering if doing this kind of thing is a good idea or not. The method seems to do its job, but I would like to avoid getting into any bad habits. Any advice welcome.
This is a great start! There's a pattern called Composed Method and this is exactly what you've followed here, knowingly or unknowingly. You chose perfect names and you chose to make your code clearly communicate the intent rather than worry prematurely about performance and the perceived overhead of making calls to other methods. These are great habits to develop and I wish more developers did this kind of thing. Sadly, a lot of the code I have to deal with and clean up is the type where the programmer is trying to be "clever' instead of clear.
As for Knute's point about the first parameter of getDistance, I could go either way. Probably would depend on what other code I have in the Point class and what the visibility of the getDistance() method is. If it's private, then I might lean towards eliminating the first parameter.
The only niggle I have is that distanceTo and getDistance are too similarly named. I'd try to find a different name to avoid confusion. If you search for "distance between two points" you'll find a few pages that explain the formula. I'd try to see what kind of language they use and maybe pick names based on that. If you want to make it clearer that you are using the Pythagorean Theorem as the basis for calculations, then you might use names that are relatable to that. The point is to make your code "explain" to the reader what's happening. That's what separates beautiful code from code that just works. IMO, you've written some nice looking code - you can still make it beautiful.
I've been advising people to read their code out loud. When you hear what the code is doing, it seems that you can catch conceptual inconsistencies better. For example, read line 3 above out loud. Doesn't it sound a lot like the code is summarizing the Pythagorean Theorem? That's what I mean by "beautiful" code.
By contrast, read this code out loud:
Does it sound anywhere near as clear as the previous one?
Which version do you think would be easier to understand by someone else who has never seen your code? Like I said, the latter is the kind of code that I have to deal with all the time and I really wish there were more people who wrote code like the former.
Even when I read them silently, the former is easier to read than the latter. My eyes only do one left to right scan of the first version. For the second, I have to scan back and forth to see how the whole formula comes together, to see if the parentheses match, to see if there's symmetry in the subexpressions, and to try to separate the operators from the operands. At the least, the one-liner could really use some whitespace. All I can think when I see this kind of code is "Sheesh, how hard is it to hit the space bar now and then?! Do you really think it's saving anything by smushing everything together like that? Please use the space bar and save someone's sanity!"
The getDistance() method is indeed in the same class as the distanceTo() method. Now that you mention it Knute, I think I did read somewhere about not passing Class variables as parameters. If you had any references to where that is mentioned, I would like to check it out again. I'm sure it's in lots of books though. I guess the reason I did so was because I wanted the getDistance method to be able to simply take two parameters and then return the result. That way I can use the same method to get the distance between the x's and y's of both Point objects. I'm wondering if there's a better solution where I could still avoid filling out the distanceTo() method with lots of code?
Regarding your question Knute about being able to calculate the length between the two points on one line, I do know how to do that but, as Junilu pointed out it was for the sake of clarity. I read a really nice book called 'Clean Code' by Robert C. Martin where the author tells of visiting the inventor of Extreme Programming, Kent Beck, and being struck by how small the functions in his code were. He goes on to say that "every function in this program was just two, or three, or four lines long. Each was transparently obvious. Each told a story. And each led you to the next in a compelling order. That’s how short your functions should be!".
On the other hand, I read yesterday on p.189 of Joshua Bloch's widely respected 'Effective Java' that you should avoid going overboard on convenience methods. So I'm wondering at what point you cross the line of 'going overboard'. I'd be interested to hear views on this.
Thank you also for all your advice Junilu. I had no idea that my attempt was actually a pattern. I also agree that I could have better variable names. I recently did a little mortgage calculator exercise and found looking at real mortgage application forms to be really useful for coming up with better names so will take a look at relevant links for this too.
I don't know how long you've been programming but if you're already reading Uncle Bob's (as R. Martin is commonly known) "Clean Code" and Bloch's "Effective Java" then you're way ahead of the game. Consider adding another of Uncle Bob's books to your library: "Agile Software Development: Principles, Practices, and Patterns" otherwise known as the PPP book. To complete your "square meal" of foundational books, add "The Pragmatic Programmer" by Andrew Hunt and David Thomas to your plate (we're actually discussing this book in the Bunkhouse Lounge forum this month).
These books will give you a rock solid foundation on which to build your programming style. If you pay attention to the references and bibliography sections of these books and keep going from there, you will have a practically endless stream of advice from experts and top-of-their-game developers.
As far as striking a balance on convenience methods, you'll have to learn how to judge that yourself. Uncle Bob said that each method should "tell a story" while Bloch says each should "pull its own weight". I combine that into "each method should pull its weight in telling a compelling story" and judge the function's/method's reason for being based on that, mostly. I definitely would make convenience methods private at first until my test code tells me that I need to increase their scope. This approach is compatible with Bloch's advice to be especially careful with interfaces. I would say to be especially careful with any public API.
If you noticed, I said "my test code tells me..." That's another thing that helps me write better code and come up with better designs: Test-Driven Development. The book we promoted last week in the Design forum, BDD in Action by John F Smart actually describes the style of "driven development" that I practice and I encourage all developers to at least try it. Most agile developers work this way. It may seem counterintuitive at first but once you get used to it and become proficient in the style, there's no going back.
On names, you've probably noticed that in the same item that you referred to, Bloch's first piece of advice is to "choose method names carefully." Uncle Bob actually devotes an entire chapter to names. Names can make or break code. Good names are essential to telling a good story. I can''t emphasize that enough but sadly, it's the thing I find most developers overlook the most. They get hung up on the details of algorithms or the underlying technologies, on using patterns and the latest and greatest framework or tool. They either forget, ignore, or are simply ignorant of the fact that the majority of code's existence is spent in maintenance mode so therefore, you should strive to make your code maintainable above anything else. Code is read much more often than it is written. Strive to make your code readable.
I could go on and on but this is about where I usually stop so I don't lose the few people who actually stick around this long to read my ramblings. Thanks.
Mark Channer wrote:...you should avoid going overboard on convenience methods. So I'm wondering at what point you cross the line of 'going overboard'. I'd be interested to hear views on this.
Here's a bit more food for thought about this (sorry, can't help it)
It's kind of like when you're cooking and you wonder how much salt to add if the recipe doesn't give a specific amount -- often it just says "season to taste", right? The reasonable thing to do is to be conservative and add a little bit at a time, taste it, then keep doing that until you're satisfied. Adding a lot all at once is not a good idea.
This is not unlike the TDD approach to refactoring. If I "smell" unclear or complicated code, I refactor. I rename and/or extract method. I do that bit by bit, one line or variable or method at a time. I keep doing it until I'm satisfied. After doing this for a while, you'll know where your thresholds are and you can tell yourself to stop when things are good enough for you.