Skip to content

Matrix: create exercise#33

Merged
sshine merged 5 commits intoexercism:masterfrom
glennj:port/matrix
Sep 10, 2019
Merged

Matrix: create exercise#33
sshine merged 5 commits intoexercism:masterfrom
glennj:port/matrix

Conversation

@glennj
Copy link
Copy Markdown
Contributor

@glennj glennj commented Aug 15, 2019

The README rendered.

problem spec canonical-data.json

@glennj glennj changed the title WIP: Matrix: create exercise Matrix: create exercise Sep 2, 2019
@glennj glennj requested a review from sshine September 2, 2019 01:59
Copy link
Copy Markdown
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent structure!

Why is Transpose uppercased when row and column are lowercased?

I happened to take part in a discussion of this exercise in exercism/ruby#962 in which my main point was that constructors shouldn't do real work. Providing a matrixFrom static constructor is excellent for achieving this; I would move the row/column construction to matrixFrom:

    constructor {rows columns} {
        variable rows $rows
        variable columns $columns
    }

...

proc matrixFrom {inputString} {
    set rows [lmap line [split $inputString \n] {split $line}]
    set columns ...
    Matrix new $rows $columns
}

although since Transpose is used in the constructor makes calling it in matrixFrom a little difficult... Say, don't typed OO languages generally prohibit calling methods in constructors before the object has been created? I won't judge OO practices, but it's something I've not seen before.

Added: Since Transpose is not part of the tested interface, wouldn't a minimal solution not have this method?

Also, notice that matrix tests were updated in exercism/problem-specifications#1576 bumping it to 1.2.0.

@glennj
Copy link
Copy Markdown
Contributor Author

glennj commented Sep 2, 2019

Excellent structure!

Thank you.

Why is Transpose uppercased when row and column are lowercased?

Methods starting with a lower-case letter are automatically exported (https://tcl.tk/man/tcl8.6/TclCmd/define.htm#M12). So Transpose is private. It seems that is the common style. Starting the method name with an underscore would accomplish the same result.

I happened to take part in a discussion of this exercise in exercism/ruby#962 in which my main point was that constructors shouldn't do real work.

I see your point. I'll refactor a bit.

Also, notice that matrix tests were updated in exercism/problem-specifications#1576 bumping it to 1.2.0.

And I'll update the tests.

I appreciate the feedback.

@glennj
Copy link
Copy Markdown
Contributor Author

glennj commented Sep 2, 2019

I noticed the "can extract column from non-square matrix with no corresponding row" canonical test has some extra whitespace around the newline, so I added a couple of tests specifically for that.

... I also see that there's effort currently to rectify that extra whitespace. I'm happy to leave it in.

@sshine
Copy link
Copy Markdown
Contributor

sshine commented Sep 3, 2019

Nice. I see that you've changed the exercise example solution to lazily parse columns/rows. This was @yyyc514's idea.

The structure even allows for non-OO solutions, which is great. I think, however, this means that if we don't place it before an exercise that contains OO-related stub code, we won't see a lot of OO solutions to this exercise.

Should we wait a little for canonical version 1.3.0 in exercism/problem-specifications#1577?

@yawpitch
Copy link
Copy Markdown

yawpitch commented Sep 3, 2019

Version 1.3.0 is now merged.

- only store parsed data as rows
- no duplication of data
- walk the rows to return requested column
@glennj glennj requested a review from sshine September 3, 2019 15:30
@glennj
Copy link
Copy Markdown
Contributor Author

glennj commented Sep 3, 2019

Or should we just drop the OO stuff and

proc matrixFrom {inputString} {
     return [lmap line [split $inputString \n] {split $line}]
}

proc row {matrix n} {
    return [lindex $matrix $n-1]
}

proc column {matrix n} {
    return [lmap row $matrix {lindex $row $n-1}]
}

@glennj
Copy link
Copy Markdown
Contributor Author

glennj commented Sep 10, 2019

@sshine, it's been a week: are you satisfied with the current situation?

Copy link
Copy Markdown
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, thanks for the ping, @yawpitch.

@glennj: Yes, very. Sorry for the delay.

We can keep the OO stuff in the example.

But as I said, I don't expect that people will solve it the OO way unless we provide either a hint under the description, or a stub that forces the use of OO. For this exercise I do prefer the stub as it is. We can think about the didactic details and fine-tune core stubs once the track opens.

@sshine sshine merged commit 55fa60f into exercism:master Sep 10, 2019
@glennj glennj deleted the port/matrix branch September 10, 2019 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants