Implemented ReadPointedScalar #111
Conversation
|
@SherifWaly You did quite a lot of work here :D next time break it down in puzzles, it will also easier for me to review. Plus, in the tasks estimation, you should also calculate the time you spend doing code review changes :) |
There was a problem hiding this comment.
@SherifWaly I think you misunderstood the requirement (or maybe I got it wrong). The algorithm you wrote seems way too complicated :D
| * @return String | ||
| */ | ||
| public String value() { | ||
| StringBuilder builder = new StringBuilder(); |
There was a problem hiding this comment.
@SherifWaly are you sure this algorithm is correct? It seems too complicated to me.
The requirement is: newlines are ignored (turned into spaces), unless it is at the end of an empty line, or a more intended line
All that needs to be done is: iterate over the lines and concatenate the ones that are at the same indentation, but with the condition that their numbers are consecutive. So basically, this line number constraint is the only difference from the other wrapped scalar (piped).
With the other lines, which are empty or more indented, you don't have to do anything, their line endings stay the same. And the same indentation lines' numbers have to be consecutive, because you don't want to concatenate lines which have nested nodes/empty lines between them:
>
Sammy Sosa completed another (0)
fine season with great stats. (1)
(2)
63 Home Runs (3)
0.288 Batting Average (4)
(5)
What a year! (6)turns into
>
Sammy Sosa completed another (0) fine season with great stats. (1)
(2)
63 Home Runs (3)
0.288 Batting Average (4)
(5)
What a year! (6)You don't concatenate lines 1 and 6 :D
Is this what your algorithm does, or am I missing something?
| */ | ||
| @Test | ||
| @Ignore | ||
| public void returnsValueWithSeveralIndentation() { |
There was a problem hiding this comment.
@SherifWaly I think this test should pass if the algorithm is correct (see my comment above)
There was a problem hiding this comment.
@amihaiemil This test should pass but lines that are indented are dropped by the YamlLines iterator() because they have different indentation from the first line :D .
So I ignored the test only :)
Should we add a decorator class to modify the iterator such that it returns all the line with any indentation?
There was a problem hiding this comment.
@SherifWaly But why are you interested in the lines that have inner indentation? As I understand, you are not supposed to touch them (they keep their line endings). So it's okay that they are skipped, no? :D
There was a problem hiding this comment.
@SherifWaly AAh, you need it only for these test purposes, to see that those lines weren't affected.... well, I would say we can leave it like that for now, maybe add a puzzle :D
There was a problem hiding this comment.
@amihaiemil Yes, I need it for testing :D
As the first case tests pass can I add a puzzle for the decorator class?
For the algorithm I think it works properly as the normal case for concatenation works properly and the second case I add \n at lines 84 to separate from previous lines and other one at line 93 to separate from the next lines.
There was a problem hiding this comment.
@SherifWaly Yes, add a puzzle for that test case.. but it's not going to be a decorator necessarily (at least, right now I can't imagine how that would work).
If the algorithm works then it's ok, but also add a puzzle for refactoring -- I will take a look later, maybe I can make it shorter :)
|
@rultor merge |
@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here |
@amihaiemil Done! FYI, the full log is here (took me 1min) |
PR for #109