Implementing `dropJ` to Drop `JoinList` Elements

Question

In the same vein as the (+++) function on JoinList and indexJ on JoinList, I implemented dropJ to drop elements from a JoinList.

Here's my attempt:

dropJ ::(Sized b, Monoid b) => Int -> JoinList b a -> JoinList b a
dropJ _ Empty = Empty
dropJ i jl | i <= 0 = jl
dropJ _ (Single _ _) = Empty
dropJ i (Append _ left right) 
 | i >= leftHt = dropJ (i-leftHt) right
 | otherwise = dropJ i left
 where leftHt = (getSize. size . tag) left

Testing

*JoinList>jlIndex2
Append (Size 3) 
 (Single (Size 1) "foo") 
 (Append (Size 2) 
 (Single (Size 1) "bar") 
 (Single (Size 1) "baz"))
*JoinList> dropJ 1 jlIndex2
Append (Size 2) (Single (Size 1) "bar") (Single (Size 1) "baz")
*JoinList> dropJ 2 jlIndex2
Single (Size 1) "baz"
*JoinList> dropJ 3 jlIndex2
Empty
*JoinList> dropJ 0 jlIndex2
Append (Size 3) 
 (Single (Size 1) "foo") 
 (Append (Size 2) 
 (Single (Size 1) "bar") 
 (Single (Size 1) "baz"))

• abuzittin gillifirca: There is a bug. Try dropJ 1 $ Append (Size 3) (Append (Size 2) (Single (Size 1) "bar") (Single (Size 1) "baz")) (Single (Size 1) "foo").
• Kevin Meredith: thank you, abuzittin, for pointing out the bug. I updated dropJ's to i >= leftHt = dropJ (i-leftHt) right Would you expect the dropJ result to have the correct Size? Perhaps it's implied?
• abuzittin gillifirca: The bug comes from this case otherwise = dropJ i left; and you can fix it like so otherwise = dropJ i left +++ right, provided you've already fixed the bug I mentioned in this answer of mine.

Answer 1

It's fine, but you have a bug in the Append case, as you completely disregard the right part of the tree:

dropJ i (Append _ left right) 
 | i >= leftHt = dropJ (i-leftHt) right
 | otherwise = dropJ i left -- where is right?
 where leftHt = (getSize. size . tag) left

You can fix this if you use +++ here again:

dropJ i (Append _ left right) 
 | i >= leftHt = dropJ (i-leftHt) right
 | otherwise = dropJ i left +++ right
 where leftHt = (getSize. size . tag) left

Other than that we could argue about formatting, but your code is fine. It works. Note that you should add more test cases though. A Arbitrary a => Gen (JoinList m a) generator might come in handy for QuickCheck, but that's left as an exercise.