Problem
the below code does what it needs to do. However I would like to know if there are better (shorter) ways to express the below while maintaining the explicitness of showing the decision like with the usage of if
.
first a few definitions
type Numbers = Array Number
type Sizes = Tuple Int Int
foreign import _dot :: Numbers -> Numbers -> Number
data Matrix = Matrix (Array Numbers) Sizes
data MatrixError =
VectorsExpected
| InvalidVectorSize Int Int
| InvalidRowSize Int
| UnexpectedError
then the implementation
dot :: Matrix -> Matrix -> Either MatrixError Number
dot (Matrix a sa) (Matrix b sb) = if areSimilarVectors sa sb then Right $ dot' a b else failed sa sb
where
areSimilarVectors s1 s2 = isVector s1 s2 && isSameSize s1 s2
dot' a b = _dot (join a) (join b)
isVector sa sb = fst sa == 1 && fst sb == 1
isSameSize sa sb = snd sa == snd sb
failed sa@(Tuple ax ay) sb@(Tuple bx by) = if isVector sa sb then
if isSameSize sa sb then
Left $ UnexpectedError
else
Left $ InvalidVectorSize ay by
else
Left $ VectorsExpected
I experimented with the use of >>=
so my code looked something like (from memory).
First a few utility fns
check' b r = if b then Right r else Left r -- the else case could be dismissed
check b r = _ -> if b then Right r else Left r
and here the rewrite
dot (Matrix a sa@(Tuple ax ay)) (Matrix b sb@(Tuple bx by)) =
check' (isVector sa sb) VectorsExpected
>>= check (isSameSize sa sb) (InvalidVectorSize ay by)
>>= check true (dot' a b)
where
dot' a b = _dot (join a) (join b)
isVector sa sb = fst sa == 1 && fst sb == 1
isSameSize sa sb = snd sa == snd sb
This is shorter IF I factor out the check
functions as generic utility functions. But its definitely not clearer (to me at least) what this does.
I am looking forward to your suggestions and some opportunities to learn new stuff.
The code is in Purescript but it should be possible to Haskellers to follow this answer as well.
Solution
Without changing the code too drastically I see two immediate improvements, for one you can pull out the Left
constructor in front of the call to fail, thus keeping Right
and Left
on the same level and making the distinction clearer:
if areSimilarVectors sa sb
then Right $ dot' a b
else Left $ failed sa sb
Secondly because the code handles failure cases I’d suggest you reverse the predicates for the ifs to reduce the distance between Reason <-> Error
if not $ isVector sa sb then
VectorsExpected
else
if not $ isSameSize sa sb then
InvalidVectorSize ay by
else
UnexpectedError
Other things you might want to consider is the naming of isVector
and the amount of shadowing of sa
and sb
. Why are you using a where clause and then still pass the arguments around explicitly? Maybe it would be even better if you matched the Tuple
s in the toplevel pattern match and stopped using fst
and snd
alltogether? Applying that we get to:
dot :: Matrix -> Matrix -> Either MatrixError Number
dot (Matrix a (Tuple ax ay)) (Matrix b (Tuple bx by)) =
if areSimilarVectors then Right $ dot' a b else Left failed
where
dot' a b = _dot (join a) (join b)
areSimilarVectors = areVectors && areSameSize
areVectors = ax == 1 && bx == 1
areSameSize = ay == by
failed =
if not areVectors then
VectorsExpected
else
if not areSameSize then
InvalidVectorSize ay by
else
UnexpectedError
I would also look at using guards:
dotProduct :: Num a => [a] -> [a] -> Either String a
dotProduct as bs
| notSimilar = Left "bad operands"
| otherwise = Right $ sum $ zipWith (*) as bs
where notSimilar = length as /= length bs
Another option is to use throwError
:
import Control.Monad
import Control.Monad.Except
dotProduct'' :: Num a => [a] -> [a] -> Either String a
dotProduct'' as bs = do
when (length as /= length bs) $ throwError "bad operands"
return $ sum $ zipWith (*) as bs
You’re forcing the reader to read the code linearly, but it also removes a level of indentation.
The Either monad allows you to use do notation here:
dot :: Matrix -> Matrix -> Either MatrixError Number
dot (Matrix a (Tuple ax ay)) (Matrix b (Tuple bx by)) = do
unless (ax == 1 && bx == 1) $ Left VectorsExpected
unless (ay == by) $ Left $ InvalidVectorSize ay by
Right $ _dot (join a) (join b)