Hi is there anyone who could have a look at my code and provide feedback on style, etc

Learning
1

Hi, just embarking on my first Ocaml program after getting half way through “Real World Ocaml” and getting to critical mass with the language. Before I ingrain too many bad habits could I get some constructive criticism of the below.

open Core
open User

(* This code reads from a CSV file of user details and create Jabber for Windows softphone profiles in 
      Cisco CUCM via AXL or it will do eventualy if I manage to finish it LOL*)

let user_from_string line = (* TODO: Move this function into the user module*)
      let field_list = String.split line ~on:',' in (* TODO: Check there are enough fields so we dont throw an exception*)
      { User.
            first_name = (List.nth field_list 0); (* TODO: Is there a nicer way to do this??*)
            second_name = (List.nth field_list 1);
            user_id = (List.nth field_list 2);
            extension = (List.nth field_list 3);
            uri = (List.nth field_list 4);
      } 

let read_file_and_create_users filename =
      Printf.printf "Processing file : %s \n" filename;
        let lines = In_channel.read_lines  filename in
          let users =  List.map ~f:(fun line -> user_from_string line) lines in
      users
            
let () = 
      let args = Sys.get_argv () in
            let filename = args.(1) in (* TODO Add some error checking incase the file name is missing*)
              let users = read_file_and_create_users filename in
                let count =  List.length users in
                  Printf.printf "User count : %d \n" count; (* TODO: Create a string_from_user function on User module*)
                   List.iter users ~f:(fun x -> match x.first_name with
                                                | None ->  Printf.printf "%s\n" "unknown"
                                                | Some x -> Printf.printf "%s\n" x
                   )
2 Likes
2

Hello Ian. Here are a few comments.

Do not indent consecutive let bindings and stick to 80 columns. You may want to have a look at the programming guidelines and checkout ocp-indent (there’s also ocamlformat but it has disputable rendering choices and it seems that some of the options that improve its output are scheduled to be removed).

Another thing is to name your anonymous functions locally (e.g. those given to map/iter) if they don’t fit on the line. By doing so the extent of your expressions becomes smaller and that makes it easier for your fellow readers to parse the program into understandable units and discover its logic.

3 Likes
3

I hadn’t noticed your TODO comments so here’s how I would rewrite your program (didn’t try to compile it though). Note that except for the layout it’s really not far from what you wrote.

let user_from_string line = match String.split line ~on:',' line with
| [first_name; second_name; user_id; extension; uri] ->
    { User.first_name; second_name; user_id; extension; uri }
| _ -> failwith (Printf.sprintf "Malformed line: %S" line)

let read_file_and_create_users filename =
  try
    let lines = In_channel.read_lines filename in
    let users = List.map ~f:user_from_string lines in
    Ok users
  with Sys_error e | Failure e -> Error e

let main () =
  let args = Sys.get_argv () in
  let filename = args.(1) in
  Printf.eprintf "Processing file : %s \n" filename;
  match read_file_and_create_users filename with
  | Error e -> Printf.eprintf "Error: %s" e; exit 1
  | Ok users ->
      let count =  List.length users in
      let print_name u = Printf.printf "%s\n" u.User.first_name in
      List.iter print_name users;
      exit 0

let () = main ()
3 Likes
4

That great Daniel thanks. I didn’t realise you could pattern match on the position of items in a list ( line 2) so very useful feedback Thanks!!

1 Like
5

If each row could have 5 or more fields, I would pattern match it by breaking it down structurally:

let user_from_string line = match String.split line ~on:',' with
  | first_name :: second_name :: user_id :: extension :: uri :: _ ->
    { User.first_name; second_name; user_id; extension; uri }
  | _ ->
    failwith line
3 Likes
6

Yes, good point, it crashed the first time I ran it because my file has an extra column.

3 Likes