vkostyukov / la4j

Linear Algebra for Java
http://la4j.org
Apache License 2.0
373 stars 107 forks source link

Refactoring of decompositor package is needed #211

Closed SamoylovMD closed 9 years ago

SamoylovMD commented 9 years ago

When I worked on test refactoring, I discovered that decompositor package looks strange! I suppose to move DecompositorFactory enum (located inside of LinearAlgebra) to a separate class and use pattern Strategy to build clear code instead of using inner enum in static utility class LinearAlgebra (current implementation uses factory pattern, which is hardly applicable when we need to change behavior dynamically). This will make decompositors more easy to use and more flexible, and LinearAlgebra class will be independent from concrete implementations of decompositor classes.

vkostyukov commented 9 years ago

I agree. There is a ticket https://github.com/vkostyukov/la4j/issues/179. My vision is to use pipeTo operator everywhere. What I want to see at a high level is not usage of withDecompositor but something like this:

Matrix a = ???
MatrixDecomposer lu = a.pipeTo(LinearAlgebra.LU); 
vkostyukov commented 9 years ago

And we should also consider naming new things decomposer not decompositor. Actually, there is a PR that rename it: https://github.com/vkostyukov/la4j/pull/189. But I think we should do the following:

SamoylovMD commented 9 years ago

Dont sure that it's a good idea, by logic Matrix objects should be independent from tools which use Matrix. The disadvantage of this approach is that we'll need to change Matrix interface always when we add new tools to library. For example, if we want to add linear optimization problems solving (#198), we'll need to add those things to Matrix interface. It's probably a violation of open/closed principle.

Maybe we could leave Matrix interface alone and move these features to another structure unit? For example:

Matrix a = ...
Matrix[] a_LUDecomposed = MatrixOperations.createNewOperation().on(a).applyAction(new LUDecomposer());
vkostyukov commented 9 years ago

We're not violating anything as long as we're using interfaces. It's not a big deal to add just a new method like pipeTo(FancyTool tool) in matrix interface. Your approach is also nice, but we shouldn't overthink this. We should keep the things simple.

I personally really like the uniform operator pipeTo that does all the magic with matrices. I bet users are going to love it.

SamoylovMD commented 9 years ago

Ok, got it.