transpose: add to track by Smarticles101 · Pull Request #604 · exercism/java

Smarticles101 · 2017-05-24T23:09:43Z

Reviewer Resources:

Smarticles101 · 2017-06-22T18:51:18Z

Finally got around to doing this after I took a long break. I had some fun making the example solution for this. 😄

FridaTveit

This looks great @Smarticles101! :D I have a few comments and a couple of questions, but they're all minor things so if you feel you're done with this exercise then just say so or ignore the comments and I'll merge it in a few days :) Great work!!

FridaTveit · 2017-06-22T19:48:03Z

+    },
+    {
+      "slug": "transpose",
+      "difficulty": 1,


We've decided that it would be a good idea if people who add exercises also estimate the difficulty. Since you implemented it, you probably know best :) If you don't fancy estimating it, don't worry about it. Just add it at the end of the track like this and we'll create an issue for estimating it :)

FridaTveit · 2017-06-22T19:56:27Z

@@ -0,0 +1,41 @@
+public class Transpose {
+    public String transpose(String toTranspose) {


You could do public String[] transpose(String[] matrixToTranspose), and thereby use an array to encode the rows instead of newlines. But if you prefer to use a String then that's fine too :)

FridaTveit · 2017-06-22T20:08:18Z

+
+    private void padRows(String[] rows) {
+        for (int i = 0; i < rows.length; i++) {
+            for (int j = i; j < rows.length; j++) {


Could you pass in maxRowSize to this method and just do if (rows[i].length < maxRowSize instead of the second loop?

No, and this is a bit tricky to see. I struggled with seeing this while making the solution. The problem description says the following:

Pad to the left with spaces.
Don't pad to the right.

You should only add padding in the case where there is a row to the right that is larger than it. Using the maxRowSize as you suggested works fine on all test cases except the last one, where the largest row is in the middle, but the last row is smaller, this causes the problem of extra spaces being inserted at which point you're technically padding to the right.

I think whoever did this in the csharp track had the same idea, so when padding they just use the maxRowSize. However, their last test case is messed up, it has two extra spaces at the end where there shouldn't be. Ref this

I see. Thanks for explaining that! :)

FridaTveit · 2017-06-22T20:11:05Z

+    @Test
+    public void emptyString() {
+        String input = "";
+        String expected = "";


You could maybe name them inputMatrix and transposedMatrix? But that's up to you, input and `expected is fine as well :)

FridaTveit · 2017-06-22T20:12:08Z

+    public void twoCharsInARow() {
+        String input = "A1";
+        String expected = "A" +
+                "\n1";


The indentation here makes this a bit difficult to read. Could you maybe try and align the rows of the matrices so it looks more like a matrix?

Agreed. IntelliJ keeps moving the indents back. 🤦‍♂️

FridaTveit · 2017-06-22T20:15:32Z

+
+    @Test
+    @Ignore("Remove to run test")
+    public void simple() {


I know you've got these names from the canonical data, so it's not your fault, but simple is not a very descriptive test name. Maybe transposeSimpleMatrix or simpleMatrix? While we should use the test data in the canonical data, we don't have to stick to the exact test names given in the description :)

FridaTveit · 2017-06-22T20:25:54Z

+
+        for (int newRowOldCol = 0; newRowOldCol < maxRowSize; newRowOldCol++) {
+            for (int newColOldRow = 0; newColOldRow < rows.length; newColOldRow++) {
+                if (newRowOldCol < rows[newColOldRow].length()) {


If newRowOldCol is less than maxRowSize (by the conditions of the for loop), would this condition ever not be true?

Yes. The way you're supposed to pad, you end up having rows on one or two test cases that are not the length of the longest row

…o appropriate location within track.

FridaTveit

Looks great! :) It doesn't need starter implementation anymore now that it has an ordering (and it's ordered after the first exercises) but apart from that it's ready to merge! :) Thanks for taking the time to estimate the difficulty!

FridaTveit · 2017-06-23T08:14:52Z

+
+    private void padRows(String[] rows) {
+        for (int i = 0; i < rows.length; i++) {
+            for (int j = i; j < rows.length; j++) {


I see. Thanks for explaining that! :)

FridaTveit · 2017-06-23T08:24:16Z

+                }
+            }
+
+            if (newRowOldCol != maxRowSize - 1) {


From what I can see then maxRowSize is only being used to check whether you've reached the last new row? To simplify that you could store each new row in an array and then at the end do String.join("\n". newRows). You definitely don't have to do that though, it's just a suggestion :)

FridaTveit · 2017-06-23T08:29:10Z

@@ -0,0 +1,5 @@
+public class Transpose {


According to the POLICIES doc, since this exercise is now after the first 20 exercises, it shouldn't have any starter implementation. Instead there should just be a .keep file in this directory :)

FridaTveit · 2017-06-23T08:29:44Z

+    @Ignore("Remove to run test")
+    public void simpleMatrix() {
+        String input = "ABC\n" +
+                       "123";


These are very easy to read now. Great! :)

FridaTveit

Great! Thank you for this @Smarticles101, an excellent new exercise! :)

Smarticles101 added 3 commits May 24, 2017 18:07

transpose: dibs on implementing

f9fa345

Transpose exercise implemented

30fc6d6

Merge branch 'master' into transpose

b16946a

Smarticles101 changed the title ~~transpose: dibs on implementing~~ transpose: add to track Jun 22, 2017

FridaTveit approved these changes Jun 22, 2017

View reviewed changes

Improved readability of test cases, estimated difficulty, and moved t…

34116c5

…o appropriate location within track.

FridaTveit reviewed Jun 23, 2017

View reviewed changes

Removed starter implementation

4a9ea26

stkent assigned FridaTveit Jun 24, 2017

FridaTveit approved these changes Jun 24, 2017

View reviewed changes

FridaTveit merged commit 4853b11 into exercism:master Jun 24, 2017

		@@ -0,0 +1,41 @@
		public class Transpose {
		public String transpose(String toTranspose) {

Uh oh!

Conversation

Smarticles101 commented May 24, 2017

Uh oh!

Smarticles101 commented Jun 22, 2017

Uh oh!

FridaTveit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FridaTveit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FridaTveit left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants