Win a copy of Reactive Streams in Java: Concurrency with RxJava, Reactor, and Akka Streams this week in the Reactive Progamming forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Liutauras Vilda
  • Junilu Lacar
  • Jeanne Boyarsky
  • Bear Bibeault
Sheriffs:
  • Knute Snortum
  • Tim Cooke
  • Devaka Cooray
Saloon Keepers:
  • Ron McLeod
  • Stephan van Hulst
  • Tim Moores
  • Tim Holloway
  • Carey Brown
Bartenders:
  • Piet Souris
  • Frits Walraven
  • Ganesh Patekar

! Refactoring Exercise: An example

 
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 5
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm starting this thread to walk through the refactoring of code that most programmers would find difficult to put under (automated) test. The code used here is from this thread: https://coderanch.com/t/716816/java/Chess-calculating-intersection-point-bishops

I have copied the original (buggy) code below. The author of the code only knew that it didn't work quite as expected but didn't know exactly what was expected. The challenge was to figure out what that expected behavior was and why the code didn't exhibit it.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 3
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There are many problems with above code and you can go back to the original thread to see what kind of problems others have pointed out. I'll focus on the fact that this code is difficult to test in an automated way.

Automated tests are essential to debugging code. If you think about what a bug is, it is essentially a misunderstanding, a failure to communicate intent clearly and correctly. Well-written automated tests can help us figure out and express exactly what we intend the code to do and then verify that it does indeed do it.

With that in mind, this is the general approach I'm going to take here to find the bug(s) in this code (not necessarily in the order shown):

1. Bring the code under test
2. Refactor to clarify intent and remove duplication
3. Write a failing test that isolates the bug
4. Fix the bug and verify the fix
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
When attempting to refactor legacy code[1] you often start in a Catch-22 situation where in order to write tests to make it safer to refactor, you first need to refactor to make it easier to test.

The biggest impediment to testability with this code is its use of System.out.println() statements. At this point, it's the only way to verify the results. Eyeballing results is a very slow and error-prone way of debugging code. We need a way to capture the program results so that we can use a unit testing framework like JUnit to verify them instead.

This is where the concept of a seam[2] becomes very important. There isn't an apparent seam in this code because all of it is in the main() method, proving once again the JavaRanch adage that MainIsAPain (←that's a link to the article that explains why). So, we'll address that first.

Notes
[1] Michael Feathers defines "legacy code" as any code that doesn't have unit tests, even code that you just wrote.
[2] Michael Feathers defines a "seam" as a place in the code where you can change behavior without editing in that place.
 
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu, should I be honored that you took the effort of using my ugly code to teach a point  ?

No just kidding. Thank you. In the end I understood what the bug was with manual tests, when a position "I9" showed up (I probably oversaw it before, I guess I was tired). "I9" is NOT a column in chess.
By manual testing I mean that I entered those coordinates in the console:


I did not do automated tests because it was such a pain to create 31 BishopPositions and assert 31 results, so I look forward to see how you will do the testing.
 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote: At this point, it's the only way to verify the results. Eyeballing results is a very slow and error-prone way of debugging code. We need a way to capture the program results so that we can use a unit testing framework like JUnit to verify them instead.



Yes, I confirmed that happened, and I missed "I9" (A chess board goes from A to H and from 1 to 8) in 31 rows of test.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
This is the first step to addressing the MainIsAPain issue:

The only thing we did here was to extract all the code that was in main() and moved it to another static method, findPaths(). It may not seem like much but it does one important thing: it starts to clarify the intent of the code, which at this point we're expressing as "finding paths." But finding paths for what? If you read the problem statement at https://open.kattis.com/problems/chess, it's finding paths a bishop (the chess piece) would take to move from one square to another.

You might also notice that I've made the findPaths() method have private access. I always start with the smallest scope possible when extracting methods because this is the best way to preserve the original behavior. If I had made it have a wider scope, I would have been adding to the public API of the class. There's no need to do that right now so keeping the extracted method private preserves the "surface area" of our class' API.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:Junilu, should I be honored that you took the effort of using my ugly code to teach a point  ?


I assume that's a tongue-in-cheek statement but "honored" isn't the word I'd use. Just look at this as a great learning opportunity. It also helps me practice explaining my refactoring and debugging process.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Previously, I said that we need to make the code more testable. Then I said the first step was to extract code from public static void main() to private static void findPaths(). How does this get us closer to having a testable class though?

When I'm doing Test-Driven Development (TDD), a technique that involves a lot of refactoring and testing, one of the rules of thumb I follow is "Write the code you wish you could write." This is the kind of test code I wish I could write:

Looking at that leads me to think I'm going to need a Bishop class. A Bishop class that I can instantiate and call methods on will be much easier to test than the code we have right now. What I need to do is move the code that's currently in the findPaths() method into proper methods of a Bishop class, then change the code in findPaths() to use Bishop objects instead.

So, instead of this:

I could write this:

This would make the intent of the code much clearer and I could easily write a test for the Bishop.isAt(Position) method.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Here's what the Bishop class might look like.

Since I'm doing TDD, I wouldn't start by writing that code immediately though. I'd start with a unit test first:

This test code is going to have a few failures which we'll address next.
 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:

D.J. Quavern wrote:Junilu, should I be honored that you took the effort of using my ugly code to teach a point  ?


I assume that's a tongue-in-cheek statement but "honored" isn't the word I'd use. Just look at this as a great learning opportunity. It also helps me practice explaining my refactoring and debugging process.



Yes, I was jokingly being thankful. Next time I will donate my ugly code anonymously       ! I want to remain employable when I am out of college  


But seriously I am very grateful that you use your precious Sunday free time to teach me some decent programming.

Edit 3: But how do you write a test for 31 Bishops at the same time, without doing it in hours?
 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Here's what the Bishop class might look like.

Since I'm doing TDD, I wouldn't start by writing that code immediately though. I'd start with a unit test first:

This test code is going to have a few failures which we'll address next.



I am pretty sure we don't need this test. The problem guarantees that the bishop WILL have a start position (problem description).
Can we save our efforts for the ruthless murdering that the bishops will do?
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:But how do you write a test for 31 Bishops at the same time, without doing it in hours?


Sure it takes time to write tests but think of how much time it took you to find a problem in your code. People seem very willing to spend inordinate amounts of time doing that kind of frustrating, low-return work instead of investing their time and effort in writing high-value, high-return tests. I think it's the epitome of being penny wise but pound foolish. Even people who are paid to write code. This is what is costing our society countless $$$ per day, even costing lives when software is mission-critical.

And I'm sure you don't need to write tests for 31 bishops. I'd estimate about 10 tests in all should cover most if not all the scenarios you'll need to cover to solve this.

I am pretty sure we don't need this test. The problem guarantees that the bishop WILL have a start position


Sure, you may not need this test to verify functionality but tests also serve another important purpose: to drive your thinking and design decisions. Without tests as example code for your designs, you're basically guessing as you go, groping around in the dark with no feedback and guidance.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Let's look at how writing that test drives our design thinking.

The test suggests a constructor signature: Bishop(Position start)

Not only does this test define semantics for creating a Bishop instance, it also establishes a dependency on another class, Position. This is design. Programmers seldom think deeply about design nor are they consciously aware of how each of their programming decisions is basically a design decision. Choosing a name is a design decision. Choosing which parameters to pass in is a design decision. Choosing static vs. non-static, public vs. private, void or having a return value, these are all design decisions that contribute to the overall quality or lack thereof of the code you write.

If you're going to try to write better quality code, it would behoove you to think more deeply and more deliberately about each design decision you make as you write the code.

The original code has a class called Point. This is easily renamed to Position to make more semantic sense in the context of a chess board. "Position" is part of the normal language you'd use when talking about chess, not "point." You want to think in the same terms used in the domain of the problem. Hence, it also makes more sense to create a position like so:

rather than what you wrote:

The latter is very cryptic, the former more straightforward and easy to correlate to the illustration in the problem.

The crypticness of the design you chose is the result of thinking about implementation too much. I find many programmers fall into this trap. When you think about implementation before having a clear idea of what you're intent is, this is the kind of hard-to-read, hard-to-work-with code that results. It's the product of implementation-based thinking when designing code and it's bad.
 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Since we have two bishops (the attacker and the target) does it make sense to have:



Only the attacker can move though.

I find it more sensefull to have a BishopPosition class. After all we are not working with Bishops, but paths.
In that case we can use the class BishopPosition to return the square where the murderer makes its U-turn towards the target. At this point of the program, we identified (correctly   ) that an attack was always possible. Example: from start at F 1, the attacker can turn at BishopPosition B5, and reach E 8.

It allows us to use a BishopPosition outOfRange instead of returning null, which does not signify anything.

(Don't ask me about the static, I still have no idea).

Also, we need a smiley with sunglasses at Code Ranch.


 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Let's look at how writing that test drives our design thinking.

The original code has a class called Point. This is easily renamed to Position to make more semantic sense in the context of a chess board. "Position" is part of the normal language you'd use when talking about chess, not "point." You want to think in the same terms used in the domain of the problem. Hence, it also makes more sense to create a position like so:

rather than what you wrote:

The latter is very cryptic, the former more straightforward and easy to correlate to the illustration in the problem.



I cannot send just "A" to Position. line[0] is my input in ascii. So it needs to be "x-'A'" and "y-48". It actually must be "x-'@'" and "y-48", OR "x-'A'" and "y-49" because of the offset.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:I cannot send just "A" to Position. line[0] is my input in ascii. So it needs to be "x-'A'" and "y-48". It actually must be "x-'@'" and "y-48", OR "x-'A'" and "y-49" because of the offset.


Of course you can. I just showed an example how. You are just (artificially) constrained into thinking that you can't because again, you are thinking about your implementation. Stop thinking about implementation and start thinking about intent and a meaningful API.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:So it needs to be "x-'A'" and "y-48". It actually must be "x-'@'" and "y-48", OR "x-'A'" and "y-49" because of the offset.


That means absolutely nothing if you were to talk to someone who knows chess notation. On the other hand, Position('A', 1) would make a lot of sense. Again, you just think it "must" be that way because you are influenced by what you have decided will be your implementation. Your implementation is driving your design. It should be that your design drives your implementation. People always think that TDD is backwards but in fact, I find the "normal" way programmers think when they develop programs to be entirely backwards from what it should be. Unfortunately, many of those responsible for teaching people how to program think in this "normal" backwards way because it was how they were trained. It was how I was trained to think until I discovered TDD.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:I find it more sensefull to have a BishopPosition class. After all we are not working with Bishops, but paths.


Feel free to try your own ideas out in actual code. I will continue using Bishop for now since it's what makes more sense to me. In a real-world pairing situation, I would compromise with my partner(s) and say something like "Maybe, but the code we have now still makes sense semantically so let's try sticking with Bishop until we write some code that tells us that BishopPosition really is a better name."
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:
Only the attacker can move though.
...
In that case we can use the class BishopPosition to return the square where the murderer makes its U-turn towards the target. .. that an attack ..,the attacker


I find it peculiar that you choose to view this problem in terms like this, i.e. murder and attacker. Nowhere in the problem statement are those terms ever used. The problem was simply to see if a bishop has a path from one position on the board to another. There's no mention of it murdering another bishop or any other chess piece.
 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You are absolutely right.
I read the problem description and understood from it that the bishop should be able to reach another bishop. But nowhere in the question another bishop is mentioned.

I look forward to see more of your refactoring. Since you mention compromising with your partner, can we compromise on limitint the number of methods to strictly necessary ? It's a competitive programming problem and the expected solution should be very short.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Before I proceed with the main refactoring, I'd like to take care of a couple of structural/organizational changes first.

First, I've renamed Point to Position to make it more aligned with commonly used chess terms. It's better to use names that come from the problem domain. I have also renamed the fields accordingly to file and rank. Lastly, I changed the scope of the fields from public to package-private (default). I want the scope of fields to be as small as possible so that I have more control over what can access them. Ideally, I would make them private but this change disturbs the current code as little as possible so it will do for now.

Next, I moved the Position class outside of the main class so there's no need to do this:

Here's the result:

Making these kind of changes without automated tests is not ideal but they're not as risky as more intensive refactorings. I find if you're careful enough, Rename and Extract refactorings are fairly safe to perform even without the safety net of unit tests.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:It's a competitive programming problem and the expected solution should be very short.


If you're going to compete based on lines of code, then you should learn to program in Perl. Perl programmers pride themselves in writing programs with as few characters as possible.

On the other hand, Java is a relatively verbose language to begin with. You can write expressive Java code that is also short and, in fact, I find that well-factored Java code is often shorter than the equivalent complex code. Often, but not all the time. I'm not guaranteeing that the resulting code we get here will be short, just more expressive, better designed, and much easier to test.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The idea that shorter is better when it comes to programs is a double-edged sword.

On one hand, elegant code is often shorter than less-than-elegant code that does the same thing. On the other hand, shorter code may lack expressiveness, which is costly and often attracts bugs. I always prefer elegant, expressive code to short but cryptic code.

Kent Beck's "Four Rules of Simple Design" list "has the fewest elements" as the fourth item, meaning it's the least important. Here they are in order of importance:

1. All tests pass
2. Clearly expresses intent
3. Contains no duplication
4. Has the fewest elements

I like Corey Haines' interpretation of Rule #4: It is small. That is, small classes, small interfaces, small scope, small parameter lists, small responsibilities, etc. I can't emphasize this enough: fewer lines of code does not necessarily translate to more efficient and better code.
 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Short reply!
What that does mean?



I'm sorry, I might not reply as thoroughly during the week as school is very intense...
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:
What that does mean?


As I mentioned previously, I renamed the fields in the Position class from x and y to file and rank, respectively. The purpose of doing so was to use common terms from the problem domain (chess). What does it mean? Well, now that you know what file and rank mean, you should know what that whole expression means since you were the one who wrote it.  What did you mean when you wrote sB.y - sB.x before?
 
Saloon Keeper
Posts: 10657
227
  • Likes 5
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Now you see how cryptic the original code is to us. :P

'Rank' is another work for the row. 'File' is another word for the column. In your original code, YOU wrote fx.y - fb.x, which Junilu refactored to use the more expressive members of Position instead of Point.

D.J. Quavern wrote:Next time I will donate my ugly code anonymously       ! I want to remain employable when I am out of college  


Owning your mistakes and learning from them is a sign of drive and intelligence. You should be proud of them.

But how do you write a test for 31 Bishops at the same time, without doing it in hours?


Junilu already pointed out that you don't necessarily have to test ALL cases, just a couple of regular cases and edge-cases. This is still time consuming, but it pays itself back: Not only does it make it easier for you to find bugs in the current code, it helps you find new bugs when you add new features to your application that break assumptions you made about your application logic before. I would say that this is actually the primary reason to write tests: You can be sure your program still works as intended when somebody makes a new feature or bugfix.

Note however that it is possible to 'automate' your automated tests. Parameterized tests allow you to build you testcases en masse, and then you test your application by making generalized assumptions about them. JUnit 4 has an experimental feature called 'Theories' that I absolutely adore. Here's an example:
 
Stephan van Hulst
Saloon Keeper
Posts: 10657
227
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Note that although the code above contains only four theories, it tests those four theories for ALL possible combinations of origin and target positions. Some of the assumptions and assertions depend on the correctness of methods such as Position.isOnSameDiagonalAs(Position), but you would test that those methods work correctly in a class such as PositionTheories.

As you can see, even if you remove all the descriptive messages from the test code, it would still tell a story in what's almost human language:


If you start off by writing tests like this, you will force your class to split up its responsibilities in methods clear names and smaller bodies. This is what Junilu means by design driving the implementation.
 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:

D.J. Quavern wrote:
What that does mean?


As I mentioned previously, I renamed the fields in the Position class from x and y to file and rank, respectively. The purpose of doing so was to use common terms from the problem domain (chess). What does it mean? Well, now that you know what file and rank mean, you should know what that whole expression means since you were the one who wrote it.  What did you mean when you wrote sB.y - sB.x before?



Honestly I'd rather have a variable that means something to me (x,y), than chess terminology directed towards à chess affiscionado that will never read this program
 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Stephan van Hulst wrote:Now you see how cryptic the original code is to us. :P

'Rank' is another work for the row. 'File' is another word for the column. In your original code, YOU wrote fx.y - fb.x, which Junilu refactored to use the more expressive members of Position instead of Point.

D.J. Quavern wrote:Next time I will donate my ugly code anonymously       ! I want to remain employable when I am out of college  


Owning your mistakes and learning from them is a sign of drive and intelligence. You should be proud of them.

But how do you write a test for 31 Bishops at the same time, without doing it in hours?


Junilu already pointed out that you don't necessarily have to test ALL cases, just a couple of regular cases and edge-cases. This is still time consuming, but it pays itself back: Not only does it make it easier for you to find bugs in the current code, it helps you find new bugs when you add new features to your application that break assumptions you made about your application logic before. I would say that this is actually the primary reason to write tests: You can be sure your program still works as intended when somebody makes a new feature or bugfix.

Note however that it is possible to 'automate' your automated tests. Parameterized tests allow you to build you testcases en masse, and then you test your application by making generalized assumptions about them. JUnit 4 has an experimental feature called 'Theories' that I absolutely adore. Here's an example:



I love it. I am watching it on my phone which is quite small but it almost look like logic programming. But way clearer.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Stephan introduces a lot of interesting techniques that are very useful. We'll come back to those once we've established some good footing around unit testing and breaking dependencies. For now, I'd like to get back to bringing the code under test.

While we could continue to do some relatively safe refactoring without tests, I'd rather bring tests in as soon as possible so that's what I'm going to do now. The biggest impediment to this code is that it is tightly coupled to its input (System.in) and output (System.out). We could use these as the enabling points.

In his book, Working Effectively with Legacy Code, Michael Feathers wrote

Michael Feathers wrote:Every seam has an enabling point, a place where you can make the decision to use one behavior or another



It's possible to redirect System.in and System.out but we can do something I think is simpler and more straightforward. Let's look at the code I extracted into findPaths() again:

The bug in this code is likely to be somewhere on lines 14-45. Therefore, I'll want to isolate that code even more. To do that, I will extract it once again. This is the result:
 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Can I ask for a cow for Junilu for opening that thread? How many cows per week can I propose?
Staff note (Stephan van Hulst):

As much as you want, he definitely deserves it. I've added one.

Staff note (Junilu Lacar):

Thanks, guys. I probably get as much as I give in these discussions. D.J., I think your participation has been great and it's wonderful to see some of these ideas making some kind of impact on you. A cow for you as well.

 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:Honestly I'd rather have a variable that means something to me (x,y), than chess terminology directed towards à chess affiscionado that will never read this program


That's fine if you want to just be a hobby programmer who doesn't have to talk to users. Professional programmers don't often create software in a vacuum though. They have to talk to domain experts who don't use technical terms but terms from the problem domain. In these situations, it's the programmers who need to adapt to the language of the domain, not the other way around.

Eric Evans, author of Domain Driven Design talks about a "ubiquitous language" that software developers and domain experts use to communicate effectively with each other to ensure that everyone is speaking in the same terms and using consistent language to represent ideas that are encoded in the software. When developers use one set of terms and users use another, the need for translation between the two groups arises and this is where most of the misunderstanding and miscommunication occurs and gets manifested in the software as bugs.

So, if you have ambitions to be a professional software developer, you'd be well served to stop thinking in "techie" terms and start training your brain to use terms from the domain of whatever problem it is you're trying to solve.
 
Stephan van Hulst
Saloon Keeper
Posts: 10657
227
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:Honestly I'd rather have a variable that means something to me (x,y), than chess terminology directed towards à chess affiscionado that will never read this program


I understand what you're saying, but one-letter identifiers are easily missed when scanning code. Even though x and y are probably the clearest one-letter identifiers a programmer could use, it's better to use something that stands out when reading.

If you're not comfortable with rank and file, use row and column instead. Better yet, use rowIndex and columnIndex instead, because those accurately describe what the variables represent (a column index is not a column itself).
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
So, by extracting a bunch of code that's inside the main for-loop of the findPath() method into a findOnePath(char[] line) method, I have decoupled the code from the input mechanism. Now I don't have a hard dependency on Scanner or System.in.  If I make this code accessible to a test by increasing its scope to package-private (default), I'm halfway to bringing it under test.

The next problem is the System.out dependency. Luckily, I can quickly see that I can shuffle things around a bit and move all the System.out.println() calls back to the findPaths() method if I just make the findOnePath() method return the string that needs to be displayed. I go ahead and do that, resulting in this:

This code is still really ugly but now I can put it under test.

Next, I will write some characterization tests to document my understanding of how this code works.
Staff note (Junilu Lacar):

If you read this reply and the other one it continues from shortly after I originally posted them, you may have noticed some discrepancies in the code in this and the previous reply. I think I have fixed all those discrepancies. Just some stupid copy-paste mistakes on my part. Sorry if that created any confusion.

 
Marshal
Posts: 7178
491
Mac OS X VI Editor BSD Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Stephan van Hulst wrote:If you're not comfortable with rank and file, use row and column instead.


Just slight clarification in case somebody misinterprets reading in such sequence: rank and file would be column and row respectively.

In different words:
file = rowIndex
rank = columnIndex
 
Stephan van Hulst
Saloon Keeper
Posts: 10657
227
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You have your terminology confused, Liutauras.

[edit]

It's actually quite easy for a Dutch person to remember. "File" is the Dutch word for a queue in traffic, which might also be called a "column of vehicles". "Rank" is like "rang", which is also a row of seats side by side.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
File - (A-H) - identifies the column
Rank - (1-8) - identifies the row

The confusion could be a good argument against using the names rank and file ... quite ironic, actually. Even I got confused when I wrote this and had to edit it before I submitted. We'll come back to this later but for now, I'm going to stick with the domain nomenclature.

 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:File - (A-H) - identifies the column
Rank - (1-8) - identifies the row

The confusion could be a good argument against using the names rank and file ... quite ironic, actually. Even I got confused when I wrote this and had to edit it before I submitted. We'll come back to this later but for now, I'm going to stick with the domain nomenclature.



Noooooooo
Please can we stick to rowIndex and columnIndex? I know it looks like a chess problem, it smells like a chess problem, it taste like a chess problem BUT it is still competitive programming problem that should be only sen by the judge. So it's not like we are developping chess software.
I need to stop and think about rank and file everytime.

 
D.J. Quavern
Ranch Foreman
Posts: 266
12
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:


This code is still really ugly but now I can put it under test.



Thank you Junilu. I will take this as a compliment and wear this as a badge of honor.

My hardware teacher called my code, (brace for it), "not a capital sin, but quite close", and "à criminal waste of memory" 😎.
At least I try stuff!!


... Can you at least delete my comment "because I do clean Java" in your copy paste 😀?

(can we get a smiley cow and a sunglasses smiley)

And thank you for the cow <3!
 
Liutauras Vilda
Marshal
Posts: 7178
491
Mac OS X VI Editor BSD Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Stephan van Hulst wrote:You have your terminology confused, Liutauras.

[edit]

It's actually quite easy for a Dutch person to remember. "File" is the Dutch word for a queue in traffic, which might also be called a "column of vehicles". "Rank" is like "rang", which is also a row of seats side by side.


Well, I took those from the link Junilu linked

chesscentral.com wrote:File: The rows of a chess board going up and down, lettered a-h (lower case), with “a” always on White’s left (and Black’s right).


Up and Down = rows

chesscentral.com wrote:Rank: The rows of a chessboard going sideways, numbered 1st-8th starting from White’s side as 1st.


sideways = columns

To my understanding I got them right.
 
I have a knack for fixing things like this ... um ... sorry ... here is a consilitory tiny ad:
Java file APIs (DOC, XLS, PDF, and many more)
https://products.aspose.com/total/java
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!