HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Get Nth item from end of list

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
itemnthgetendlistfrom

Problem

I am starting out in Haskell and thought I would try to make a function that got the index (nth) from the end of the list.

So getNthFromEnd 5 [1..10] would equal 5

The code I have so far is

getNthFromEnd :: Int -> [a] -> a
getNthFromEnd _ [] = error "Empty list given"
getNthFromEnd nth list
    | listLength = nth = list !! (listLength - nth - 1)
    where listLength = length list
getNthFromEnd _ _ = error "Unknown exception occurred"


The code works as intended as far as I can tell, but seeing as though I am just getting to know Haskell I thought your suggestions on how this code could be cleaned up or refactored to make better use of FP concepts would be invaluable to my learning of Haskell and the FP paradigm overall.

Solution

I'll go through the code line-by-line:

1    getNthFromEnd :: Int -> [a] -> a
 2    getNthFromEnd _ [] = error "Empty list given"
 3    getNthFromEnd nth list
 4        | listLength = nth = list !! (listLength - nth - 1)
 7        where listLength = length list
 8    getNthFromEnd _ _ = error "Unknown exception occurred"


Line 1

Type signature - good!

Line 2

A function which calls out to error is kinda frowned upon. Such functions are
called partial functions because they could throw an exception and that is not
indicated in the type signature. That's why some people don't like to use head
and tail - they prefer to use pattern matching instead for these functions.
This is not really a critic of your code, more just an observation. If you were
to write documentation for this function you might warn users that it is partial,
although they might be able to guess from the description that it necessarily has to be.

More on partial functions:

https://begriffs.com/posts/2013-08-18-dont-be-partial-to-partial-functions.html

Line 4

I would change the error message to List length must be at least ... to match the comparison.

Lines 4 -- 6

Clearly one of the three comparisons has to hold, so for efficiency you can change
the guard in line 6 to an otherwise clause:

| listLength < nth  = ...
| listLength == nth = ...
| otherwise         = ...


This avoids an unnecessary comparison check when listLength is greater or euqal
to nth.

Another common Haskell idiom is to match on the result of compare like this:

-- may need Data.Ord

case compare listLength nth of
  LT  -> ...
  EQ  -> ...
  GT  -> ...


This has the advantage of being provably exhaustive by the compiler. However, it may turn out to be the slowest of the three. It depends a lot on how smart the compiler is. I don't see it used a lot in the Prelude functions for integer comparisons, and I imagine there is an efficiency reason behind that.

To summarize:

  • The code you've written is exhaustive (handles all the cases) just because we know that one of the comparisons has to hold. But GHC can't prove this, and it may perform an extra comparison when it doesn't need to.



  • Using an otherwise clause is exhaustive because we're using an otherwise clause. But it avoids an extra check sometimes.



  • Using compare is provably exhaustive by the compiler.



In any case, this declaration will handle all the cases which leads to...

Line 8

Based on the previous discussion, your function will never get to this point so
this line isn't needed. Even if it did, GHC will throw it's own error with a message
like:

*** Exception: /tmp/err.hs:3:1-17: Non-exhaustive patterns in function foo


Moreover, having this catch-all clause is hiding a potentially useful warning. If you remove that line and compile with -Wall GHC reports:

/tmp/pats.hs:5:1: Warning:
    Pattern match(es) are non-exhaustive
    In an equation for ‘getNthFromEnd’: Patterns not matched: _ (_ : _)


This raises two points:

-
The way you've written the code you are not proving to GHC that you are handling all of the list patterns. GHC would like you to write the second stanza with a pattern:

getNthFromEnd nth list@(:) =

That enables GHC to know that you've covered both the empty list and non-empty list cases.

  • Using the catch-all clause prevents GHC from making this check.

Code Snippets

1    getNthFromEnd :: Int -> [a] -> a
 2    getNthFromEnd _ [] = error "Empty list given"
 3    getNthFromEnd nth list
 4        | listLength < nth = error $ "List length must be greater than " ++ show nth
 5        | listLength == nth = head list
 6        | listLength >= nth = list !! (listLength - nth - 1)
 7        where listLength = length list
 8    getNthFromEnd _ _ = error "Unknown exception occurred"
| listLength < nth  = ...
| listLength == nth = ...
| otherwise         = ...
-- may need Data.Ord

case compare listLength nth of
  LT  -> ...
  EQ  -> ...
  GT  -> ...
*** Exception: /tmp/err.hs:3:1-17: Non-exhaustive patterns in function foo
/tmp/pats.hs:5:1: Warning:
    Pattern match(es) are non-exhaustive
    In an equation for ‘getNthFromEnd’: Patterns not matched: _ (_ : _)

Context

StackExchange Code Review Q#128747, answer score: 7

Revisions (0)

No revisions yet.