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

Push List Element to Top

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

Problem

Please evaluate this function. It takes a list [a] and an Int, i.e. the index, and returns a new list with the selected item at the top of the list. Note that it returns Maybe [a] to account for bad indexes.

pushToTop :: [a] -> Int -> Maybe [a]
pushToTop [] _   = Just []
pushToTop xs i 
  | i >= length xs || i  snd x) . filter (\x -> fst x /= i)) zipped


I'm also curious if my usage of !! is OK given the 2 guards on i.

Solution

Yes, the guards make !! safe, but your code has other issues, stylistic and functional:

Unneeded pointful style

First, it's often a good idea to try running your code through HLint. Doing so, you get the following suggestion:


pushToTop.hs:9:52: Error: Avoid lambda

Found: \x -> snd x

Why not: snd

1 suggestion

Indeed, why not. If we're to use point-free style for this, we could also simplify

filter (\x -> fst x /= i)


to

filter ((/= i) . fst)


At some point, point-free style can get difficult to read, but in this case I think it is still readable.

Parentheses that can be replaced with a use of $

Right here:

(map (\x -> snd x) . filter (\x -> fst x /= i)) zipped


Using $, you could also inline zipped non-awkwardly:

map snd . filter ((/= i) . fst) $ zip [1..] xs


Incorrect base case

Shouldn't any index on an empty list be considered an error? As is, you're completely ignoring the index. For consistency, I would make the first case return Nothing.

Unnecessary case

This case is already handled by the latter case:

| i == 0 = Just xs


It can be safely removed.

Failure to work on infinite lists

The operation you have defined (move an element at a certain index to the front) is well-defined on infinite lists, but your code fails to handle the case. In particular, length will not terminate on infinite lists. The current structure of your code cannot handle this. Rewriting it to make explicit the recursion and to avoid calculating the length should make it work on infinite lists.

Test case:

ghci> fmap (take 20) $ pushToTop [0..] 12
[12, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 13, 14, 15, 16, 17, 18, 19]

Code Snippets

filter (\x -> fst x /= i)
filter ((/= i) . fst)
(map (\x -> snd x) . filter (\x -> fst x /= i)) zipped
map snd . filter ((/= i) . fst) $ zip [1..] xs
| i == 0 = Just xs

Context

StackExchange Code Review Q#54583, answer score: 3

Revisions (0)

No revisions yet.