New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coverage now 100%, added POD... #4

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@BrianRossmajer

BrianRossmajer commented Jan 20, 2015

I did change how two methods could be called ... matrix_multiply is now a proper instance method, and new can no longer be called on an instance ... so updated the version number.

I hope that last change isn't annoying. The construct that allowed new to be called on the class or an instance was hard to test properly, and when I researched it, found a recommendation to not do it that way that made a lot of sense: http://www.stonehenge.com/merlyn/UnixReview/col52.html ... I hope that's okay!

@liyanage

This comment has been minimized.

Show comment
Hide comment
@liyanage

liyanage Jan 20, 2015

Owner

This all looks good, thanks again.

I'm OK committing it like it is, but I do have one remark that we should discuss first: Personally I would not have exposed all these internal methods (matrix_multiply, set_matrix_2x3, determinant, matrix_2x3, concatenate_matrix_2x3).

That is a lot of extra API surface and cognitive burden for the client. They were all supposed to be hidden and the interface was supposed to be just "concatenate". I think a client can access all the functionality through that one simple API call.

I assume that you exposed them for ease of coverage/unit testing, and not to improve the user-facing API. If that's the case, I would try to hit these paths with coverage tests without also exposing all these internals.

What do you think? And thanks again :)

Owner

liyanage commented Jan 20, 2015

This all looks good, thanks again.

I'm OK committing it like it is, but I do have one remark that we should discuss first: Personally I would not have exposed all these internal methods (matrix_multiply, set_matrix_2x3, determinant, matrix_2x3, concatenate_matrix_2x3).

That is a lot of extra API surface and cognitive burden for the client. They were all supposed to be hidden and the interface was supposed to be just "concatenate". I think a client can access all the functionality through that one simple API call.

I assume that you exposed them for ease of coverage/unit testing, and not to improve the user-facing API. If that's the case, I would try to hit these paths with coverage tests without also exposing all these internals.

What do you think? And thanks again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment