This week's book giveaway is in the OO, Patterns, UML and Refactoring forum.
We're giving away four copies of Five Lines of Code and have Christian Clausen on-line!
See this thread for details.
Win a copy of Five Lines of Code this week in the OO, Patterns, UML and Refactoring 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
  • Bear Bibeault
  • Ron McLeod
  • Jeanne Boyarsky
  • Paul Clapham
Sheriffs:
  • Tim Cooke
  • Liutauras Vilda
  • Junilu Lacar
Saloon Keepers:
  • Tim Moores
  • Stephan van Hulst
  • Tim Holloway
  • fred rosenberger
  • salvin francis
Bartenders:
  • Piet Souris
  • Frits Walraven
  • Carey Brown

Refactoring Exercise: An example

 
Saloon Keeper
Posts: 12136
258
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
They confused you by using 'rows' in the description of File, where they meant 'squares'. A file is one sequence of squares going up and down. So a column really. It even says that 'A' is on the white player's left.

Vice versa for Rank.
 
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ok, so now we're going to finally write a test. Ideally, we wouldn't be testing a static method like findOnePath(). There are a few reasons for avoiding this but we won't get into that right now. (If you really want to know more, search for why are static methods hard to test?) The good news is that findOnePath() is a pure function. It takes some input, does something, and returns a value based on the input. Most importantly, it doesn't maintain state nor does it produce any side effects. We made sure of that by removing its dependency on System.out. This mitigates any problems that static methods normally present to testability.

After doing a static import of the findOnePath() method in my test class, I have this first test to explore my understanding of the if-statement on line 25:

This test passes. Great! Now we're cookin'!

I add another assertion:

Note that having multiple assertions in one test method can be a test "smell" (something not good). However, we're still just poking around and exploring the code at this point and we just want to quickly verify our understanding of how the code works. This does that for us so we'll just acknowledge the smell for now and plan to address it later on, if we still feel there's a need to.

One thing I want to do though is make it easier to write tests. Having to call toCharArray() all the time gets tedious and it makes the test code really noisy. So, I'm going to refactor this with a quick Extract Method:

When writing characterization tests, I want to check both edge cases and normal cases:

The test passes so I'm pretty confident the bug isn't in that if-statement.

I could continue the bug hunt with the next if statement but here's the thing: when you have a moment of opportunity to clarify code, take that moment. Resist the urge to say "I'll come back to that later." Unclear code has a higher risk of having bugs and a big tendency to attract bugs. If it will take you a few seconds to make code clearer, do it. You and others who have to maintain the code later will be thankful you did.

So, we refactor (Extract Method) this:

and get this:

This makes if-statement self-documenting. The comment above it is now redundant so we can delete it.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:Please can we stick to rowIndex and columnIndex?


Just bear with me for now. I'm going to try to show that it doesn't even matter that much. What matters is understanding intent and making the code express that intent clearly. Row/Column, Rank/File, Y/X, these are different names for details that are more closer to the implementation. If we stick with the high-level ideas, we won't have to think about these names just yet.

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 hear you, but hear me: I don't care about the competition. I'm trying to make a point about code clarity and unit testing. Feel free to "optimize" however you see fit for your purpose. Just remember the old adage though: "Premature optimization is the root of all evil."—Donald Knuth. Also, "The road to programming hell is paved with premature optimizations."
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
We look at the else-if statement on line 30 next:

Before I write a test for this, I'd like to refactor it to clearly express its intent. That way, my test code can also clearly reflect my understanding.

The OP's (original programmer ) comment doesn't really help us much here though. Most comments are usually written in some context. The comment was relevant to that context and may have made sense to whoever wrote it at the time but since the context details are not explicitly explained in the comment, anyone reading the comment later will just be confused. Such is the case here. I have little idea of how the comment relates to the code but again, it's not very helpful in figuring out what the intent is. I would much rather delete that comment and refactor the code to make it more expressive.

After analyzing the formula for a bit, I believe that this checks for squares that are on the same diagonal. So, I refactor (Extract Method again) it:

I then write this test:

These tests pass so I'm pretty sure the bug is not here.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You might have noticed that I haven't really talked much about rank/file vs. row/column as I've been refactoring. So far, I have been able to ignore those details by focusing on the high-level ideas of "is same position" and "is same diagonal". Yes, the choice of using rank/file has created some confusion in the side discussions but it hasn't really been an issue in the main demo discussion because I'm not looking that deep into implementation details yet. I'm keeping it as high-level as I can so I don't even have to deal with them yet.
 
Junilu Lacar
Sheriff
Posts: 15767
264
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 code I have so far (I'm going to adjust the line numbers to match what I have in my IDE so it's less confusing for me):

In the original thread, we've already discussed how the comment on line 35 is misleading, so I delete the comment and again refactor (Extract Method) to make the code clearer:

And I create these passing tests to give me confidence this code also has no problem:

I didn't spend a lot of time analyzing these assertions for duplication of intent but they give me enough confidence to move on to the next section to hunt down the bug.
 
Rancher
Posts: 303
16
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Liutauras Vilda wrote:

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.



You know what?
Let's definitely change for rowIndex and columnIndex.
This File and Rank is getting all the focus. I am still unsure of who's who.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I've pretty much narrowed down the likely source of the problem to this section of code:

It would make sense that this code would have some hard-to-find bug in it. It's not easy to understand and the comments don't help much. In fact, given the experience with the comments in the sections above this, I don't even trust those comments to tell me the truth so, I will simply ignore and delete them. Instead I will analyze each of these lines to see if I can understand their intent.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:You know what?
Let's definitely change for rowIndex and columnIndex.


No. I already explained why a few posts ago.  

This File and Rank is getting all the focus.


Not from me. They haven't even played into anything else I've done since I renamed them.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
This is quite a nasty little bug.

#1 Impediment to finding the bug: The formulae used to calculate the intermediate position do not conform to the standard slope formula that I am familiar with

This made the code confusing and difficult to follow. Having test assertions that (mostly) passed made it difficult to argue against their validity though.

I tried ignoring the comment at first but could not get any insight into what the OP was thinking when they wrote this code. So I went back to the comment for a hint. The standard formula for slope is: Given points P1(x1, y1) and P2(x2, y2), the slope m of the line P1-P2 is equal to (y2 - y1) / (x2 - x1)

Trying to correlate the standard slope formula to this code is a mind-breaking exercise, even when I use different names and go back to x and y instead of file and rank:

There are at least three ways to flush out this bug:
1. Brute force by trying all combinations of positions that would result in this code being executed
2. Pure dumb luck: just keep trying different combinations until you find the bug
3. Mathematical analysis of the formulae and #1 or #2

To be honest, I ended up doing mathematical analysis and #2—I did use OP's comment about getting an invalid "I*" position so that helped narrow the field down to just positions on the right edge of the board. Explaining my debugging process would be too long at this point and because it involved pure dumb luck, not easy to reproduce anyway. I started going down the brute force route but I started writing code that was way beyond what most beginners would be able to write. (It was along the lines that Stephan showed earlier, where he was using @Theory and such)

Here are the two assertions that I found failed (and I'm pretty sure these are the only cases that would fail):

The tests for the reverse paths both pass:
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The OP suggested a fix to this bug which, in my opinion, was a kludge:

First of all, the change from 65 to 64 is even more mysterious. Anyone who knows a bit about ASCII will figure out that 65 is 'A' but what the heck is 64 (it's '@') and how the heck is that related to the problem domain ??? Even though the "fix" might work, it's a nonsensical fix.

Secondly, it does not address the problem of duplication in the code. When I say "duplication," it's in reference to the definition implied by the DRY Principle that states "Every piece of knowledge or logic must have a single, unambiguous representation within a system."

I'm going to leave this as an exercise for now: Where is the duplication of knowledge is in this code? Explain why it's duplication.
 
Junilu Lacar
Sheriff
Posts: 15767
264
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 really highlights what I've said before: Hard-to-read and hard-to-test code usually contains bugs. I call this kind of code "opaque" because it's not transparent and difficult to impossible to see through. Opaque code not only hides bugs, it actually invites bugs and more nastiness. Look at OP's suggested fix, for example.

We see this kind of thing happen all the time in the real world and it's the cause of many long nights and weekends, time away from family and doing things you'd rather be doing instead of chasing down an elusive bug buried in hundreds or thousands of lines of code.
Staff note (Junilu Lacar):

Note to OP: please don't take this as a criticism of you, personally. First, this is a common occurrence, even in code written by people paid to write code. Second, the criticism is really at the code and the problems it presents. To be an effective programmer, you have to be able to separate your "self" (or ego) from the code so that you can address problems in the code clinically and rationally. This is challenging, of course, since we often see code as "our own work." This is where the idea of Egoless Programming can help.

 
Marshal
Posts: 69799
277
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:. . . the change from 65 to 64 is even more mysterious. Anyone who knows a bit about ASCII . . . .

But you can still get your ASCII numbers wrong. It is better to try x − 'A' or x − 'A' + 1. The latter formula will give A=1, B=2, C=3...Z=26. But that sort of trick only works for the 26 letters in the English alphabet.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:But you can still get your ASCII numbers wrong. It is better to try x − 'A' or x − 'A' + 1. The latter formula will give A=1, B=2, C=3...Z=26. But that sort of trick only works for the 26 letters in the English alphabet.


True, but that's just a local fix and in my opinion doesn't begin to address the real problem: duplication and unclear intent. Using 'A' instead of 65 is a Band-Aid® at best.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If you carefully listen to how Ward Cunningham explains the Debt Metaphor, you'll understand why "Debt" really boils down to being "A lack of understanding in the code." A great example of code that lacks understanding is the section of code we've narrowed the bug down to.

Making the small tweaks like changing 65 to 64 does absolutely nothing to address that lack of understanding. I could just as easily "fix" the bug by doing this:

This approach and the one suggested before are no better than the ones you find here: https://www.popularmechanics.com/home/how-to-plans/how-to/g519/10-hilariously-bad-home-diy-projects/

To address the bug properly, you need to refactor the code for clarity of intent and eliminate all duplication. That's the challenge here. Any takers?
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
FWIW, my approach to addressing the problem is akin to surgical removal of cancerous tissue and grafting in healthy tissue. I will share my solution later to give others a chance to join in on the discussion and share their solutions.
 
D.J. Quavern
Rancher
Posts: 303
16
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:This really highlights what I've said before: Hard-to-read and hard-to-test code usually contains bugs. I call this kind of code "opaque" because it's not transparent and difficult to impossible to see through. Opaque code not only hides bugs, it actually invites bugs and more nastiness. Look at OP's suggested fix, for example.

We see this kind of thing happen all the time in the real world and it's the cause of many long nights and weekends, time away from family and doing things you'd rather be doing instead of chasing down an elusive bug buried in hundreds or thousands of lines of code.



Seems I cannot reply to your staff note.
That's fine, I don't take it as personal criticism. I think it's grand of you to invest so much time into teaching clean code.
My criminal and nasty code is just proof that I don't let common sense stop me when I am being creative 😎😎. At least I'm doing (ugly) things!

I am taking note of your extreme refactoring for bigger systems I will build. I admit I won't necessary apply it to next competitive programming problems. I like the conciseness when submitting to the judge, and Java is definitely too verbose.

In other words i am FILE-ing the knowledge for later, for as long as I am RANKing so low at competitive programming, I will stick to my coach standards. I am already long from being his favorite, so...

But very soon I will be building bigger systems.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ok, here's the refactored findOnePath() method:

Getting here from where we were previously took a number of small refactorings, with tests being run to green each time one small change was made.

Another challenge: Can you get from there to here by making small, non-breaking changes to the code, backed up by unit tests?

Note: I chose "wayPoint" because it's the first word that came to mind that fit my intent.

Admittedly, others may not be familiar with the term so feel free to suggest a different word. I'm only familiar with it because I used to work for a company whose name was based on that word/meaning. Apparently, it's a term commonly used in sailing. I'm not a sailor or know much about sailing but the word has stuck in my mind all these years for some reason.
 
D.J. Quavern
Rancher
Posts: 303
16
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:This is quite a nasty little bug.

#1 Impediment to finding the bug: The formulae used to calculate the intermediate position do not conform to the standard slope formula that I am familiar with

This made the code confusing and difficult to follow. Having test assertions that (mostly) passed made it difficult to argue against their validity though.

I tried ignoring the comment at first but could not get any insight into what the OP was thinking when they wrote this code. So I went back to the comment for a hint. The standard formula for slope is: Given points P1(x1, y1) and P2(x2, y2), the slope m of the line P1-P2 is equal to (y2 - y1) / (x2 - x1)

Trying to correlate the standard slope formula to this code is a mind-breaking exercise, even when I use different names and go back to x and y instead of file and rank:

There are at least three ways to flush out this bug:
1. Brute force by trying all combinations of positions that would result in this code being executed
2. Pure dumb luck: just keep trying different combinations until you find the bug
3. Mathematical analysis of the formulae and #1 or #2

To be honest, I ended up doing mathematical analysis and #2—I did use OP's comment about getting an invalid "I*" position so that helped narrow the field down to just positions on the right edge of the board. Explaining my debugging process would be too long at this point and because it involved pure dumb luck, not easy to reproduce anyway. I started going down the brute force route but I started writing code that was way beyond what most beginners would be able to write. (It was along the lines that Stephan showed earlier, where he was using @Theory and such)

Here are the two assertions that I found failed (and I'm pretty sure these are the only cases that would fail):

The tests for the reverse paths both pass:


Right line equation is kx+m.
For bishop moves, k will always ne 1 or -1, and for the same reasons, a descending line has a value of y0+x0, and an ascending line has y0-x0, y0 and x0 being the values we read from input.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:Right line equation is kx+m.
For bishop moves, k will always ne 1 or -1, and for the same reasons, a descending line has a value of y0+x0, and an ascending line has y0-x0, y0 and x0 being the values we read from input.


Thanks for the explanation but it still doesn't help me understand the code much. I see this kind of attempt to clarify in the real world as well and I myself have probably been guilty of it. The problem is that it really shouldn't matter much. All those details should be hidden deep in the implementation code, in private methods that are covered indirectly by tests that exercise public methods that call them. The public, high-level methods should be very clear and obviously correct. If we still have failing tests, the high-level code and the failing conditions should lead us to the exact low-level implementation code that is buggy.
 
Campbell Ritchie
Marshal
Posts: 69799
277
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:. . . a Band-Aid® at best.

Agree.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Another step in the ruthless refactoring journey would be to see something like this:

This suggests moving some of the logic into a separate class called BishopPath that encapsulates the calculation of the path a Bishop would take to move from one position to another.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
To make it easier to contrast the Before vs. After states of the code, here they are:

And here's what it was refactored to, with the bug fix:

Think about this: the refactored code is not significantly longer than the original code. However, how much clearer is the refactored code? Which version of code would you rather deal with? (And yes, I know there are more lines involved that aren't shown in the refactored version above but if you're going to quibble about that, I don't think you're getting the point I'm making)
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If I were working with code that had problems like this in the real world, I might stop where I got to above, especially if I was dealing with deadlines and other kind of business pressures. The motivation to refactor, as Martin Fowler writes, really comes down to $$$: how much does it cost to refactor vs. not refactor. You have to weigh both short-term and long-term gains and costs. In the long run, poorly factored code usually costs more if it creates significantly more work to change than it would have if you invested time and effort to refactor up front. This is one reason I'm a vocal proponent of Test-Driven Development.

It's a tough balancing act. Sometimes you can refactor too much, meaning you spend a lot of time refactoring code that nobody ever touches, doesn't really present any problems, or is used very infrequently. You have to choose your battles wisely and refactor only as much as the effort will likely pay good dividends. That's a line that's not easy to draw but with enough practice and experience, you can make wise choices most of the time.

However, when it comes to that initial decision of whether or not to refactor, I'm with Ward Cunningham: it's best to make sure you're paying as little interest on your debt as possible. Or as Uncle Bob puts it: The only way to go fast is to go well.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I couldn't resist (besides, this is great practice for me )

I'm going to try to refactor this:

to something that clearly expresses what it intends to do:
 
D.J. Quavern
Rancher
Posts: 303
16
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:To make it easier to contrast the Before vs. After states of the code, here they are:

And here's what it was refactored to, with the bug fix:

Think about this: the refactored code is not significantly longer than the original code. However, how much clearer is the refactored code? Which version of code would you rather deal with? (And yes, I know there are more lines involved that aren't shown in the refactored version above but if you're going to quibble about that, I don't think you're getting the point I'm making)



I must admit, it is starting to look really good! (for a software implementation!)

At line  (x < 1 || x > 7 || y < 1 || y > 8), it should be  (x < 1 || x > 8 || y < 1 || y > 8),
 
D.J. Quavern
Rancher
Posts: 303
16
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:I couldn't resist (besides, this is great practice for me )

I'm going to try to refactor this:




to something that clearly expresses what it intends to do:



I see you are having a lot of fun !

I can feel another question poping out in my head. Let me open a new thread...
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:At line  (x < 1 || x > 7 || y < 1 || y > 8), it should be  (x < 1 || x > 8 || y < 1 || y > 8),


No, in fact, if you leave everything else the way it was (not changing 65 to 64) and just change 8 to 7 on that line, it will fix the bug you had, or at least it fixed the bug in the code that I found with the two cases "H8C1" and "H7D1" that produced an incorrect intermediate position.

You mentioned a case where you got "I9" -- I haven't found any input that would make the code calculate I9 for the intermediate position. This is what I was saying earlier about dumb luck though. It's not a very reliable way of debugging.

Important Note: As I mentioned previously, neither the fix I show here (changing 8 to 7 in that one spot) nor the one you made (changing 65 to 64 in two spots) is a very good fix. They are kludges, equivalent to fixing a car with chicken wire and duct tape. The best approach is to replace that code with something that's clearer. "Clever" code like what you wrote is often very costly. If this were code in a real-world application, I would be sitting you down for a long talk about quality and technical excellence.
 
Rancher
Posts: 1197
22
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Liutauras Vilda wrote:To my understanding I got them right.



My contribution to a great thread:

Ranks are rows.
Files are columns.

Think of a "rank" as a military rank.  All the individuals of a given rank are at the same height from the bottom.
Think of a "file" people walking in "single file".  They are not side-by-side.  I picture them in a line from near me to farther away from me, much like a column on a sheet of paper on my desk.



 
D.J. Quavern
Rancher
Posts: 303
16
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:FWIW, my approach to addressing the problem is akin to surgical removal of cancerous tissue and grafting in healthy tissue. I will share my solution later to give others a chance to join in on the discussion and share their solutions.



AHAHAHA OMG! I missed that possed! How could I!
Can I put "cancerous tissue" to my collection of badge of honor?
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:At line  (x < 1 || x > 7 || y < 1 || y > 8), it should be  (x < 1 || x > 8 || y < 1 || y > 8),


I just went to Kattis and submitted the original code you posted with just that one tiny change I showed, changing 8 to 7. It was accepted on the first submission. Executed in 0.51s. I also submitted the refactored version of the code. It, too, was accepted; it ran in 0.48s, so slightly better than your code. I know a lot of people who would expect differently but I'm not surprised at all -- modern compilers have a better chance of applying optimizations to well-factored code than they do with code that's not.

So, yes, that change was another fix to your problem. Again, not a very good fix. I can't even say it's better than your fix of changing 65 to 64, which to me is nonsensical. Both fixes are equally bad in their own right.
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote:

Junilu Lacar wrote:surgical removal of cancerous tissue and grafting in healthy tissue.


AHAHAHA OMG! I missed that possed! How could I!
Can I put "cancerous tissue" to my collection of badge of honor?


I hope you are truly taking those comments as lightheartedly as you seem to be. Again, it's nothing personal against you and there's no intent of putting you down or anything like that. I always treat code as a separate entity from the person who wrote it: I will malign bad code any day, call it names, curse it to high heaven, and shoot it down but please believe me when I say that I'm not trying to bring you, the author of the code, down along with it. Like I said before, egoless programming.
 
D.J. Quavern
Rancher
Posts: 303
16
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:

D.J. Quavern wrote:

Junilu Lacar wrote:surgical removal of cancerous tissue and grafting in healthy tissue.


AHAHAHA OMG! I missed that possed! How could I!
Can I put "cancerous tissue" to my collection of badge of honor?


I hope you are truly taking those comments as lightheartedly as you seem to be. Again, it's nothing personal against you and there's no intent of putting you down or anything like that. I always treat code as a separate entity from the person who wrote it: I will malign bad code any day, call it names, curse it to high heaven, and shoot it down but please believe me when I say that I'm not trying to bring you, the author of the code, down along with it. Like I said before, egoless programming.



No, I mean it.
You actually opened my eyes on a lot of things in that thread.
 
Campbell Ritchie
Marshal
Posts: 69799
277
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:. . . . Executed in 0.51s. . . . the refactored version . . . ran in 0.48s . . .

Is that difference large enough to take any notice of?
 
Junilu Lacar
Sheriff
Posts: 15767
264
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:My hardware teacher called my code, (brace for it), "not a capital sin, but quite close", and "à criminal waste of memory" 😎.


Who cares about memory anymore these days?!! This is veeery old-school thinking, and not in a good way. The bulk of the cost of ownership in software these days is in maintenance. There's about a 1:10 to 1:50 ratio of good code to bad code out in the world today and even that might be too generous. There's way more bad code than there is good code out there. One big reason: there are way more bad programmers out there than there are good programmers.

Good programmers make sure they test and refactor their code. Good programmers make sure their code can be read and understood by others. Good programmers realize there is no "my code" any more. There's just code that needs to be maintained and enhanced. The only code that is "your code" is the code you're working on at the moment. Once you commit that code, it becomes "OUR code," where "OUR" refers to everyone in the organization that is paying for the code to be written.

These days, "criminal waste" is when you write code that is difficult to read and difficult to change. I can go to the local drugstore and get more memory any time of the day. Yeah, it's that cheap and ubiquitous. And we haven't even talked about cloud-based platforms. Basically, hardly anybody cares much about memory these days; except maybe hardware teachers stuck in the year 1966.    
 
Junilu Lacar
Sheriff
Posts: 15767
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:

Junilu Lacar wrote:. . . . Executed in 0.51s. . . . the refactored version . . . ran in 0.48s . . .

Is that difference large enough to take any notice of?


It's enough to rebut any argument that refactoring will negatively impact the performance of code.
 
Campbell Ritchie
Marshal
Posts: 69799
277
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

D.J. Quavern wrote: . . "à criminal waste of memory" 😎. . . .

If it is that criminal, please tell me where you live, so I can go there and shoot you. Alternatively, write if statements with and without {} and inspect the bytecode with javap −c MyClass. If you can actually see any difference, I shall have to kill you. If you can't see any difference, where is that waste of memory?

As you will see here, when I was young, 4kB of memory cost about the same as every house on the street (we have some short streets here). I can go onto eBay and buy 10⁶× that amount of memory for less than I could spend on two pints of beer. So the cost of the memory you are wasting can be calculated as less than the cost of 1 microlitre of beer. And that is criminal, isn't it.
 
D.J. Quavern
Rancher
Posts: 303
16
IntelliJ IDE Firefox Browser Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:

Junilu Lacar wrote:. . . . Executed in 0.51s. . . . the refactored version . . . ran in 0.48s . . .

Is that difference large enough to take any notice of?



Yes, it is even faster then my code that is 0.52s.
A reasonably good time is less than 0.1s. That's why you saw me using input custom classes InputReader or Kattio when I started posting on the Ranch. I wrote this problem with Scanner to minimize the confusion (ironically), and because InputReader throws an Exception in main.
Usually to get good times, this is what I do. Or write in C++.
 
    Bookmark Topic Watch Topic
  • New Topic