# Refactor code to be more succinct but yet preserve the distinction of the `if` [closed]

Posted on

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

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)
``````