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 Leftconstructor 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 Tuples 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)

Leave a Reply

Your email address will not be published. Required fields are marked *