Refactor BasicType with documentation and exception improvements #532
Conversation
There was a problem hiding this comment.
Looks good.
Please see comment about a follow-up issue and PR to get rid of the static numColumns (or we should figure out if there's actually some need for that version of the method, though I cannot see one).
I also wonder about having more "is" methods (e.g. isScalar()) and going over the code base more generally to replace lots of the code that does "if(BasicType.allMatrixTypes().contains(...))" to use "isMatrix", similar for "isScalar", etc.
| @@ -344,26 +344,44 @@ public boolean isBoolean() { | |||
| BVEC4)); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Hmm, not sure why we have this static method at all. It looks like numColumns(mat) is the same as mat.getNumColumns().
Can you open an issue for us to delete the static method and change all code that used it to use the instance version?
Fixes #531.
This PR:
makes a distinction between usage of RuntimeException and UnsupportedOperationException - RuntimeException is still used in functions like toString(), where the type could possibly be in an invalid state. RuntimeException has been replaced by UnsupportedOperationException when the user can potentially (and realistically) misuse a function (sending invalid arguments or using an unintended type). Additionally, UnsupportedOperationException is thrown instead of asserting when an error state is realistically possible.
adds Javadoc comments to functions that throw UnsupportedOperationException.
replaces usage of static allTypes().contains(type) calls with the corresponding isType() call when the current scope is not static.