How can I change this if-then-else construction to a construction that uses pattern matching or/and guards?

user_16940402 0
Viewed 110

I've been given the following exercise (one out of several that link together to pretty print a table and make selections in it):

Write a function select :: Field → Field → Table → Table that given a column name and a field value, selects only those rows from the table that have the given field value in the given column. If the given column is not present in the table then the table should be returned unchanged. (Hint: use the functions (!!), elemIndex, filter and maybe.)

And ended up with this solution:

select :: Field -> Field -> Table -> Table
select column value table@(header:rows) =
    let i = fromMaybe (-1) (elemIndex column header)
    in  if i == (-1) then table
        else header : filter (\r -> r !! i == value) rows

While it seems to be perfectly correct in its function - it works - I've been told that if-then-else constructions such as these are 'bad form' and should be avoided using guards (as should my use of fromMaybe using pattern matching).

How would I go about changing this into a 'better' style with pattern matching/guards?

pattern-matching list function haskell
2 Answers

One simple improvement that I immediately see when looking at your code is that it seems pointless to use fromMaybe to convert Nothing to -1, then simply do a check (whether with an if or with guards doesn't matter) on whether that value is -1. Why not check against Nothing in the first place?

I guess you might have been led this way by similar functions in other languages where if an index isn't found then -1 is returned as an artifical value (sometimes called a "sentinel value") to mean "the element wasn't found" - but in Haskell, Nothing communicates this much better.

Further Nothing can be pattern matched on, so rather than using if or guards, you can use a case statement. This turns your function into:

select :: Field -> Field -> Table -> Table
select column value table@(header:rows) =
    case elemIndex column header of
          Nothing -> table
          Just i -> header : filter (\r -> r !! i == value) rows

This to my mind is much better than your original.

I still feel it could be improved further - in particular (!!) is rarely used in idiomatic Haskell code as it risks crashing if the index is out of bounds. It's also quite inefficient compared to the array-indexing operators of other languages (because Haskell lists are linked-lists, not arrays, and to get say the 100th element it has to run through the previous 99 rather than being able to do direct "random access"). However I don't see how you can really avoid that given what you have to work with here.

user_84750540

Replace

let i = fromMaybe (-1) (elemIndex column header)
in if i == (-1) then ... else ...

with

case elemIndex column header of
   Nothing -> ... -- no index found
   Just i  -> ... -- index i found

Your original code suffers from "boolean blindness": you already have a useful value of type Maybe Int which tells you:

  • if there is no index (Nothing)
  • if there is an index, and what it is (Just i)

The last emphasized part is important! In your code you work hard to lose that information, reducing everything to a boolean telling you only:

  • if there is no index (True)
  • if there is an index, and nothing else (False)

Don't use booleans unless you really have to. Many languages must use if someBooleanCondition to conditionally branch. Haskell does not have to: we have case and pattern matching that combines branching and extracting information from each case (the i above).

user_32349590
Related