views:

213

answers:

5

I've trying to learn F#. I'm a complete beginner, so this might be a walkover for you guys :)

I have the following function:

let removeEven l = 
 let n = List.length l;
 let list_ = [];
 let seq_ = seq { for x in 1..n do if x % 2 <> 0 then yield List.nth l (x-1)}
 for x in seq_ do
  let list_ = list_ @ [x];
 list_;

It takes a list, and return a new list containing all the numbers, which is placed at an odd index in the original list, so removeEven [x1;x2;x3] = [x1;x3]

However, I get my already favourite error-message: Incomplete construct at or before this point in expression...

If I add a print to the end of the line, instead of list_:

...
print_any list_;

the problem is fixed. But I do not want to print the list, I want to return it!

What causes this? Why can't I return my list?

+2  A: 

Edit: If I understand your requirements correctly, here's a version of your function done functional rather than imperative style, that removes elements with odd indexes.

let removeEven list =
    list
    |> Seq.mapi (fun i x -> (i, x))
    |> Seq.filter (fun (i, x) -> i % 2 = 0)
    |> Seq.map snd
    |> List.ofSeq

> removeEven ['a'; 'b'; 'c'; 'd'];;
val it : char list = ['a'; 'c']
Joel Mueller
I would use `Seq` personally.
ChaosPandion
Doesnt this return the odd-numbers in the sequence, and not the numbers, placed at an odd index in the array?(I'll edit the original post to make this more clear).
Frederik Wordenskjold
I might as well, but I wanted to match the intended method signature of `(int list -> int list)`. Perhaps there's a reason Frederik wants the output as a list rather than a sequence.
Joel Mueller
@Frederik - I see I misunderstood your code - you want the elements located at odd indexes, not the odd elements. I'll update my answer.
Joel Mueller
I cannot decide which I like better. :(
ChaosPandion
Okay, my answer and ChaosPandion's both produce the same output. Mine has more transformations, but only loops through the input list one time. Because F# lists do not have random access, I believe ChaosPandion's must loop through the input once to get the length, and then must loop from 0..i every time it needs to retrieve an element at index i. If that's right, then I believe my version will perform better on large lists, and it won't make any difference on small lists.
Joel Mueller
I just checked reflector and `List.length` calls the length property on the list itself. I think it was added to aid with type inference.
ChaosPandion
That's handy to know, and I guess it makes sense that it can keep track of the length without counting. Each node in the list can return the length property of the previous node, plus 1. So no penalty for finding out the length, but it's still got to loop to get the nth element from the list.
Joel Mueller
Well I'll be damned. I just finished running an extensive test and yours blew mine away!
ChaosPandion
List Size=1000; Iterations=100; Result(ChaosPandion)=4200ms; Result(Joel Mueller)=150ms;
ChaosPandion
Wow! I wasn't expecting that dramatic of a difference.
Joel Mueller
Lets just say I know which one I like better now.
ChaosPandion
+1  A: 

I think this is what you are looking for.

let removeEven list = 
    let maxIndex = (List.length list) - 1;
    seq { for i in 0..2..maxIndex -> list.[i] }
    |> Seq.toList

Tests

val removeEven : 'a list -> 'a list

> removeEven [1;2;3;4;5;6];;
val it : int list = [1; 3; 5]
> removeEven [1;2;3;4;5];;
val it : int list = [1; 3; 5]
> removeEven [1;2;3;4];;
val it : int list = [1; 3]
> removeEven [1;2;3];;
val it : int list = [1; 3]
> removeEven [1;2];;
val it : int list = [1]
> removeEven [1];;
val it : int list = [1]
ChaosPandion
+6  A: 

To answer your question first, the compiler complains because there is a problem inside the for loop. In F#, let serves to declare values (that are immutable and cannot be changed later in the program). It isn't a statement as in C# - let can be only used as part of another expression. For example:

let n = 10
n + n

Actually means that you want the n symbol to refer to the value 10 in the expression n + n. The problem with your code is that you're using let without any expression (probably because you want to use mutable variables):

for x in seq_ do 
  let list_ = list_ @ [x]  // This isn't assignment! 
list_

The problematic line is an incomplete expression - using let in this way isn't allowed, because it doesn't contain any expression (the list_ value will not be accessed from any code). You can use mutable variable to correct your code:

let mutable list_ = [] // declared as 'mutable'
let seq_ = seq { for x in 1..n do if x % 2 <> 0 then yield List.nth l (x-1)}    
for x in seq_ do    
  list_ <- list_ @ [x] // assignment using '<-'

Now, this should work, but it isn't really functional, because you're using imperative mutation. Moreover, appending elements using @ is really inefficient thing to do in functional languages. So, if you want to make your code functional, you'll probably need to use different approach. Both of the other answers show a great approach, although I prefer the example by Joel, because indexing into a list (in the solution by Chaos) also isn't very functional (there is no pointer arithmetic, so it will be also slower).

Probably the most classical functional solution would be to use the List.fold function, which aggregates all elements of the list into a single result, walking from the left to the right:

[1;2;3;4;5] 
  |> List.fold (fun (flag, res) el -> 
     if flag then (not flag, el::res) else (not flag, res)) (true, [])
  |> snd |> List.rev

Here, the state used during the aggregation is a Boolean flag specifying whether to include the next element (during each step, we flip the flag by returning not flag). The second element is the list aggregated so far (we add element by el::res only when the flag is set. After fold returns, we use snd to get the second element of the tuple (the aggregated list) and reverse it using List.rev, because it was collected in the reversed order (this is more efficient than appending to the end using res@[el]).

Tomas Petricek
List Size=1000;Iterations=100;Result(ChaosPandion)=4200ms;Result(Joel Mueller)=150ms;
ChaosPandion
How about Tomas's `List.fold` approach?
Joel Mueller
His came in at about 135ms.
ChaosPandion
Thanks for the stats! If we were using arrays, it would be probably faster, but on the other hand, lists are nicely functional... So unless this is performance-critical, I would prefer lists.
Tomas Petricek
I think I'd pick the List.fold option, as long as I knew that I'd never have to change it to removing every 3rd or 5th element or something.
Joel Mueller
To remove every third element, you could use integer instead of a Boolean flag. In the condition, you would test `index % 3 = 0` and instead of flipping flag, you would increment the counter `index + 1`.
Tomas Petricek
@Joel, careful, you're dangerously close to invoking the FizzBuzz demons :)
Benjol
Thanks for all your solutions! I'll pick this as the answer since it gives a good explanation of what I didnt understand in the first place. But all your solutions are interesting! I'll use them to practice my, at the moment, very limited F# skills.
Frederik Wordenskjold
A: 

You can try a pattern-matching approach. I haven't used F# in a while and I can't test things right now, but it would be something like this:

let rec curse sofar ls =
    match ls with
    | even :: odd :: tl -> curse (even :: sofar) tl
    | even :: [] -> curse (even :: sofar) []
    | [] -> List.rev sofar

curse [] [ 1; 2; 3; 4; 5 ]

This recursively picks off the even elements. I think. I would probably use Joel Mueller's approach though. I don't remember if there is an index-based filter function, but that would probably be the ideal to use, or to make if it doesn't exist in the libraries.

But in general lists aren't really meant as index-type things. That's what arrays are for. If you consider what kind of algorithm would require a list having its even elements removed, maybe it's possible that in the steps prior to this requirement, the elements can be paired up in tuples, like this:

[ (1,2); (3,4) ]

That would make it trivial to get the even-"indexed" elements out:

thelist |> List.map fst // take first element from each tuple

There's a variety of options if the input list isn't guaranteed to have an even number of elements.

amr
A: 

Yet another alternative, which (by my reckoning) is slightly slower than Joel's, but it's shorter :)

let removeEven list =
    list
    |> Seq.mapi (fun i x -> (i, x))
    |> Seq.choose (fun (i,x) -> if i % 2 = 0 then Some(x) else None)
    |> List.ofSeq
Benjol