patternMinor
Push List Element to Top
Viewed 0 times
listpushtopelement
Problem
Please evaluate this function. It takes a list
I'm also curious if my usage of
[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)) zippedI'm also curious if my usage of
!! is OK given the 2 guards on i.Solution
Yes, the guards make
Unneeded pointful style
First, it's often a good idea to try running your code through HLint. Doing so, you get the following suggestion:
Found:
Why not:
1 suggestion
Indeed, why not. If we're to use point-free style for this, we could also simplify
to
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:
Using
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
Unnecessary case
This case is already handled by the latter case:
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,
Test case:
!! 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 lambdaFound:
\x -> snd xWhy not:
snd1 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)) zippedUsing
$, you could also inline zipped non-awkwardly:map snd . filter ((/= i) . fst) $ zip [1..] xsIncorrect 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 xsIt 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)) zippedmap snd . filter ((/= i) . fst) $ zip [1..] xs| i == 0 = Just xsContext
StackExchange Code Review Q#54583, answer score: 3
Revisions (0)
No revisions yet.