add atRow: method for PMMatrix #201
Conversation
|
|
||
| ] | ||
|
|
||
| { #category : #'cell accessing' } |
There was a problem hiding this comment.
Any programmer tests to write here, @rakki-18 ? I seems simple enough
There was a problem hiding this comment.
@hemalvarambhia Yeah sure, I will write some tests.
There was a problem hiding this comment.
@hemalvarambhia Yeah sure, I will write some tests.
TYSM
hemalvarambhia
left a comment
There was a problem hiding this comment.
Small comments. Would appreciate your thoughts.
src/Math-Matrix/PMMatrix.class.st
Outdated
| ] | ||
|
|
||
| { #category : #'cell accessing' } | ||
| PMMatrix >> atRow: anInteger [ |
There was a problem hiding this comment.
anInteger -> rowNumber . Here should be 0 index it, do you think?
There was a problem hiding this comment.
By cell accessing, you mean accessing a matrix element? If so, maybe that's a more domain-driven naming. Again, thoughts welcome.
There was a problem hiding this comment.
anInteger->rowNumber. Here should be 0 index it, do you think?
Yes, rowNumber makes more sense. But atColumn: method had the argument name as anInteger, so I thought it would be better to stick on to the same here also. Shall I change it to rowNumber?
There was a problem hiding this comment.
By cell accessing, you mean accessing a matrix element? If so, maybe that's a more domain-driven naming. Again, thoughts welcome.
Yeah, cell accessing doesn't seem the exact category for this method but again I stuck on to this because atColumn: method also had the same category.
There was a problem hiding this comment.
anInteger->rowNumber. Here should be 0 index it, do you think?Yes,
rowNumbermakes more sense. ButatColumn:method had the argument name asanInteger, so I thought it would be better to stick on to the same here also. Shall I change it torowNumber?
Yes please. It makes the code clearer, I think.
I would suggest you do the same for the argument name for atColumn. Great spot
There was a problem hiding this comment.
By cell accessing, you mean accessing a matrix element? If so, maybe that's a more domain-driven naming. Again, thoughts welcome.
Yeah, cell accessing doesn't seem the exact category for this method but again I stuck on to this because
atColumn:method also had the same category.
Leave it as it is in this PR, and propose a separate one for improving the category name.
|
@hemalvarambhia I have made all the changes. Can you please check again now? |
hemalvarambhia
left a comment
There was a problem hiding this comment.
Approved. One small change to make, and then merge at your leisure.
A nice small PR that was easy to review.
Thank you for taking my feedback on board. ❤️
src/Math-Matrix/PMMatrix.class.st
Outdated
| PMMatrix >> atColumn: acolumnNumber [ | ||
|
|
||
| ^ self columnAt: anInteger | ||
| ^ self columnAt: acolumnNumber |
There was a problem hiding this comment.
Thanks, I have corrected it.
|
I notice that it's master being merged to master. That looks odd. Is it intended, @rakki-18 ? If it was, normally our practise is to create a well named branch. |
Oh my bad, I will make a new branch for my PR from next time. |
The
PMMatrixclass did not have the methodatRow:initialised even though the methodatColumn:was initialised.The method,
atRow:is introduced through this PR.