My problem is rather simple. Reading my own code i have difficulty is seeing in an eye if something is a mutable variable an immutable variable a function or a higher order function when it is used.
Any ideas on given them different names, “a standard ?”, or places them in different blocks/modules ?
Differentiate between mutable variables , immutable variables , functions and higher_order functions
Why do you need to differentiate them?
To improve readability of the program.
What exactly do you mean by “readability”, and how does differentiating between these specific aspects improve it?
It seems likely that you’re overusing mutable variables, and the solution then is usually not to differentiate but to avoid it, using a number of different techniques depending on scenario, or narrow the scope to the point where it’s no longer a problem.
But if for some reason you actually do need mutation in a larger scope, you might want to consider hiding the variable and using get_x and set_x functions to access it.
I cannot think of any reason for needing to differentiate between “ordinary” functions and higher-order function, however. Perhaps you could illustrate the problem with an example?
Offcourse. A simple example system programming. The command lsblk in linux gives partition information, i rewrote it for freebsd. This is the code but i find my own code hard to read…
The code starts with 4 “associative arrays”
let () =
let nam2dev = ref [ ("", "") ] in
let dev2nam = ref [ ("", "") ] in
let dev2mount = ref [ ("", "") ] in
let dev2ro = ref [ ("", "") ] in
let read_process_lines (command:string) : (string list) =
let lines = ref [] in
let in_channel = Unix.open_process_in command in
(try
while true do
lines := input_line in_channel :: !lines
done
with End_of_file -> ignore (Unix.close_process_in in_channel));
List.rev !lines
in
let rec mylookupkv (sk:string) (alist:(string*string)list) : string =
match alist with
| [] -> ""
| (key, value) :: tail -> if sk = key then value else mylookupkv sk tail
in
let regexpos regex line : int =
try Str.search_forward regex line 0 with Not_found -> -1
in
let spacereg = Str.regexp " +" in
let glabelline line =
let lst = Str.split spacereg line in
let n = "/dev/" ^ List.hd lst in
let d = "/dev/" ^ List.nth lst 1 in
nam2dev := (n, d) :: !nam2dev;
dev2nam := (d, n) :: !dev2nam
in
let mountline line =
let lst = Str.split spacereg line in
let w = List.hd lst in
let t = List.nth lst 2 in
let v = mylookupkv w !nam2dev in
let w2 = if v <> "" then mylookupkv w !nam2dev else w in
dev2mount := (w2, t) :: !dev2mount;
let roreg = Str.regexp "read-only" in
let pos = regexpos roreg line in
if pos <> -1 then dev2ro := (w2, "ro") :: !dev2ro
else dev2ro := (w2, "rw") :: !dev2ro
in
let id_to_command (cid:string) : string = "ps -p " ^ cid ^ " -o command | sed 1d | sort -u" in
let ntfsline line =
let lst = Str.split spacereg line in
let id = List.hd lst in
let command = id_to_command id in
let pstxt = read_process_lines command in
let lst2 = Str.split spacereg (List.hd pstxt) in
let d = List.nth lst2 3 in
let m = List.nth lst2 4 in
let () =
let () = dev2ro := (d, "?") :: !dev2ro in
let () = dev2mount := (d, m) :: !dev2mount in
()
in
()
in
let eqline (x:bool) line =
let len = String.length line in
if x then String.sub line 2 (len - 2) else line
in
let ismin (x:bool) (line:string) :string = if x then "<Free>" else line in
let mountval (x:string) (d:string) : string = if x = "" then "-" else mylookupkv d !dev2mount in
let roval (x:string) (d:string) : string = if x = "" then "-" else mylookupkv d !dev2ro in
let padme (x:int) (s:string) : string =
let numspaces = x - String.length s in
let spacesref = ref "" in
for i = 0 to numspaces do
spacesref := " " ^ !spacesref
done;
let sum = s ^ !spacesref in
sum
in
let gpartline (line : string) =
if String.length line > 0 then
let c = line.[0] in
let qline = eqline (c = '=') line in
let asplit = Str.split spacereg qline in
let el2 = List.nth asplit 2 in
let el3orfree = List.nth asplit 3 in
let el3 = if el3orfree = "free" then "-" else el3orfree in
let d = "/dev/" ^ el2 in
let aname = ":" ^ ismin (el2 = "-") d in
let amount = ":" ^ mountval (mylookupkv d !dev2mount) d in
let atype = ":" ^ el3 in
let ro = ":" ^ roval (mylookupkv d !dev2ro) d in
let openreg = Str.regexp "(" in
let closereg = Str.regexp ")" in
let openpos = regexpos openreg qline in
let closepos = regexpos closereg qline in
if openpos <> -1 then
if closepos <> -1 then
let asize = String.sub qline openpos (closepos - openpos + 1) in
let output =
padme 19 aname ^ padme 13 asize ^ padme 20 atype
^ padme 20 amount ^ padme 11 ro ^ "\n"
in
print_string output
else ()
else ()
in
let glabeltxt_to_kv (lines : string list) = List.map glabelline lines in
let glabeltxt : string list = read_process_lines "glabel status | awk '{if (NR!=1) print $1,$3}'" in
let mounttxt_to_kv (lines : string list) = List.map mountline lines in
let mounttxt : string list = read_process_lines "mount" in
let ntfstxt_to_kv (lines : string list) = List.map ntfsline lines in
let ntfstxt : string list = read_process_lines "pgrep ntfs-3g" in
let gparttxt_to_kv (lines : string list) = List.map gpartline lines in
let gparttxt : string list = read_process_lines "gpart show -p" in
let (_ : unit list) = glabeltxt_to_kv glabeltxt in
let (_ : unit list) = mounttxt_to_kv mounttxt in
let (_ : unit list) = ntfstxt_to_kv ntfstxt in
let (_ : unit list) = gparttxt_to_kv gparttxt in
()
It’s still not obvious to me from this in what way differentiating between higher-order functions and ordinary functions would improve readability. But there are a lot of other things that could.
Let me try to illustrate my point about the overuse of mutable variables above, as one example of how the underlying problem is in how you structure the program.
nam2dev is a ref cell (mutable variable) that is mutated in exactly one location, inside glabelline, which in turn is also just used in one location, inside glabeltxt_to_kv. So why does the entire program have access to mutate it?
What if glabelline just returned the (nam, dev) pair for each line, and glabeltxt_to_kv used those to construct and return the nam2dev and dev2nam lists, instead of a unit list that is just thrown away? Then there would be no need to differentiate.
let glabelline line =
let lst = Str.split spacereg line in
let nam = "/dev/" ^ List.hd lst in
let dev = "/dev/" ^ List.nth lst 1 in
(nam, dev)
in
let glabeltxt_to_kv (lines : string list) =
let nam2dev = List.map glabelline lines in
let dev2name = List.map (fun (n, d) -> (d, n)) nam2dev in
(nam2dev, dev2nam)
in
...
1 Like
I’d like to point out that strictly speaking, OCaml has no mutable variables. It has mutable data structures, like arrays, reference cells, and so on, but when you define let x = ref 0 in ... you define an immutable variable, x, bound to a mutable reference cell. Sometimes the compiler will manage to optimise the cell away, but sometimes not.
Some tips that help readability:
- Don’t forget comments. Typically, when several operations are performed in a sequence, it eventually makes sense to try to split these into sections and use comments to separate the sections.
- Don’t use references for things that are initialised, then never changed again (as said above). For things that are built incrementally, it’s still better to only use a reference when building, and only expose an immutable value once it’s initialised.
- Don’t use
List.nthwith constant indices. If you’re expecting a fixed number of elements, you can uselet [x; y; z] = my_list in ..., if you only have a minimum number of elements you can uselet x :: y :: z :: rest = my_list in .... Note that these will get you warnings about partial matches, which is a good sign that you’re doing something a bit strange; you can work around that using:
let (x, y, z) =
match mylist with
| [x; y; z] -> x, y, z
| _ -> failwith "<some useful error message>"
in
...
- Look a bit at the standard library for useful functions.
List.mapwith a function returningunitis better written asList.iter, thepadmefunction could useString.make numspaces ' ', thePrintfandformatmodules have very useful functions for creating strings in a more readable way than^(look in particular at thesprintffunction). - Use variant types. The option type could prove useful, but for instance the
isminfunction would read better as:
type device_or_free = Free | Device of string
let ismin (dev : device_or_free) =
match dev with
| Free -> "<Free>"
| Device d -> d
in
...
let d =
if el2 = "-" then Free else Device ("/dev/" ^ el2)
in
... ismin d ...
- Don’t be afraid of higher order functions. For example, currently your
read_process_linesfunction only returns the lines. But in all cases, the lines will be handled one by one anyway, so you might as well streamline the process:
let handle_process_lines ~(handler: string -> unit) (command : string) : unit =
let in_channel = Unix.open_process_in command in
let rec loop () =
let line = input_line in_channel in
handler line;
()
in
try loop () with End_of_file -> ignore (Unix.close_process_in in_channel)
in
...
let () = handle_process_lines ~handler:glabelline "glabel status | awk '{if (NR!=1) print $1,$3}'" in
...
(Note that if you want to combine it with the suggestion to return the nam2dev and dev2nam lists in this step, you’ll need to tweak a bit the code to make it look more like a fold function)
Overall, it looks like you’ve translated mechanically code from another language into OCaml. This is never going to end up very readable. It’s a good way to get initial working code, but if you want to be able to maintain it you should spend time to make it look more like idiomatic OCaml (incrementally, you don’t need to rewrite everything at once).
4 Likes
With all due respect. I look at things as ada.
This means here a function, its clear input & output & variables.
It’s just now breaking my head looking at the same way in a different thing.
Take what I say with a grain of salt … in relative terms I’m an OCaml newbie at ~1 year.
The previous comments make sense. But even still I don’t think I would be able to read your code with those suggested modifications. Your code is missing types; right now everything has been collapsed into strings. I’ve taken a stab at introducing types … and noticed in a couple places I couldn’t get the type checker to pass (if the compiler can’t understand it, I doubt a human can read it).
Summary of changes:
- Some mutations have become derivatives. For example, when
dev2mountis “changed” I introduce a new variabledev2mount'. - I’ve captured the types into modules for four reasons:
- A module signature lets me hide the underlying representation of
Nam.tandDev.t(etc.) so thatNam.tcan’t be compared to aDev.t. Then I can let the type checker catch semantic errors, and it should also help understanding the code. - Those types start simple (
stringin your case) but inevitably they would get more complex. I would replace them with records (ex.module Dev = struct type t = { inode: int; mutable name: string } (*...*) end) without having to refactor much code. - A module keeps related things together in the code, so I typically don’t find it hard to analyze where , for example, a mutable field
type t = { mutable name: string}is getting changed. - It is annoying to consume two record types that have the same record name in the same module. Qualifying the type with a module name avoids the problem.
- A module signature lets me hide the underlying representation of
- Once I introduced the types
Nam.t,Nam2dev.t, etc. I just changed the code so that it passed the type checker. To do that I had to switch a few of yourList.mapintoList.foldas suggested elsewhere.
From Ada to OCaml:
-
outparameters are just return arguments. In the code below I just used tuples, but in real code I will often use a new single type (ex.module Result = struct type t = { val1: int; val2: string } end) and just let the caller access the output fields by name.
(** {1 Item} *)
module type ITEM = sig
type t
val empty : t
val create : string -> t
val show : t -> string
end
module ItemBasic = struct
type t = string
let create s = s
let empty = ""
let show s = s
end
module Nam : ITEM = ItemBasic
module Dev : ITEM = ItemBasic
module Mount : ITEM = ItemBasic
module Ro : ITEM = ItemBasic
(** {1 Item Maps} *)
module type ITEMMAP = sig
type key
type value
type t
val empty : t
val of_list : (key * value) list -> t
val push : t -> key -> value -> t
val mylookupkv : key -> t -> value
end
module ItemMapBasic (Key: ITEM) (Value: ITEM) = struct
type key = Key.t
type value = Value.t
type t = (Key.t*Value.t) list
let empty = []
let of_list lst = lst
let push lst n d = (n, d) :: lst
let rec mylookupkv (sk:Key.t) (alist:(Key.t*Value.t)list) : Value.t =
match alist with
| [] -> Value.empty
| (key, value) :: tail -> if sk = key then value else mylookupkv sk tail
end
module Nam2dev : ITEMMAP with type key = Nam.t and type value = Dev.t = struct
include ItemMapBasic (Nam) (Dev)
end
module Dev2nam : ITEMMAP with type key = Dev.t and type value = Nam.t = struct
include ItemMapBasic (Dev) (Nam)
end
module Dev2mount : ITEMMAP with type key = Dev.t and type value = Mount.t = struct
include ItemMapBasic (Dev) (Mount)
end
module Dev2ro : ITEMMAP with type key = Dev.t and type value = Ro.t = struct
include ItemMapBasic (Dev) (Ro)
end
let () =
let read_process_lines (command:string) : (string list) =
let lines = ref [] in
let in_channel = Unix.open_process_in command in
(try
while true do
lines := input_line in_channel :: !lines
done
with End_of_file -> ignore (Unix.close_process_in in_channel));
List.rev !lines
in
let regexpos regex line : int =
try Str.search_forward regex line 0 with Not_found -> -1
in
let spacereg = Str.regexp " +" in
let glabelline line =
let lst = Str.split spacereg line in
let n = Nam.create @@ "/dev/" ^ List.hd lst in
let d = Dev.create @@ "/dev/" ^ List.nth lst 1 in
n, d
in
let mountline ~nam2dev ~dev2mount ~dev2ro line =
let lst = Str.split spacereg line in
let w = List.hd lst in
let t = Mount.create @@ List.nth lst 2 in
let v = Nam2dev.mylookupkv (Nam.create w) nam2dev in
let w2 = if v <> Dev.empty then Nam2dev.mylookupkv (Nam.create w) nam2dev else Dev.create w in
let dev2mount' = Dev2mount.push dev2mount w2 t in
let roreg = Str.regexp "read-only" in
let pos = regexpos roreg line in
let dev2ro' = if pos <> -1 then Dev2ro.push dev2ro w2 (Ro.create "ro")
else Dev2ro.push dev2ro w2 (Ro.create "rw") in
dev2mount', dev2ro'
in
let id_to_command (cid:string) : string = "ps -p " ^ cid ^ " -o command | sed 1d | sort -u" in
let ntfsline ~dev2ro ~dev2mount line =
let lst = Str.split spacereg line in
let id = List.hd lst in
let command = id_to_command id in
let pstxt = read_process_lines command in
let lst2 = Str.split spacereg (List.hd pstxt) in
let d = Dev.create @@ List.nth lst2 3 in
let m = Mount.create @@ List.nth lst2 4 in
let dev2ro' = Dev2ro.push dev2ro d (Ro.create "?") in
let dev2mount' = Dev2mount.push dev2mount d m in
(dev2mount', dev2ro')
in
let eqline (x:bool) line =
let len = String.length line in
if x then String.sub line 2 (len - 2) else line
in
let ismin (x:bool) (line:string) :string = if x then "<Free>" else line in
let mountval ~dev2mount (x:string) (d:Dev.t) : Mount.t = if x = "" then Mount.create "-" else Dev2mount.mylookupkv d dev2mount in
let roval ~dev2ro (x:string) (d:Dev.t) : Ro.t = if x = "" then Ro.create "-" else Dev2ro.mylookupkv d dev2ro in
let padme (x:int) (s:string) : string =
let numspaces = x - String.length s in
let spacesref = ref "" in
for i = 0 to numspaces do
spacesref := " " ^ !spacesref
done;
let sum = s ^ !spacesref in
sum
in
let print_gpartline ~dev2ro ~dev2mount (line : string) =
if String.length line > 0 then
let c = line.[0] in
let qline = eqline (c = '=') line in
let asplit = Str.split spacereg qline in
let el2 = List.nth asplit 2 in
let el3orfree = List.nth asplit 3 in
let el3 = if el3orfree = "free" then "-" else el3orfree in
let d = "/dev/" ^ el2 in
let aname = ":" ^ ismin (el2 = "-") d in
(* THIS LOOKS FISHY ... CAN'T GET THE TYPES TO LINE UP ... REPLACING IT! *)
(* let amount = ":" ^ Mount.show (mountval ~dev2mount d (Dev2mount.MYLOOKUPKV d dev2mount)) in *)
let amount = ":" ^ Mount.show (Dev2mount.mylookupkv (Dev.create d) dev2mount) in
let atype = ":" ^ el3 in
(* THIS LOOKS FISHY ... CAN'T GET THE TYPES TO LINE UP ... REPLACING IT! *)
(* let ro = ":" ^ Ro.show (roval ~dev2ro d (Dev2ro.mylookupkv d dev2ro)) in *)
let ro = ":" ^ Ro.show (Dev2ro.mylookupkv (Dev.create d) dev2ro) in
let openreg = Str.regexp "(" in
let closereg = Str.regexp ")" in
let openpos = regexpos openreg qline in
let closepos = regexpos closereg qline in
if openpos <> -1 then
if closepos <> -1 then
let asize = String.sub qline openpos (closepos - openpos + 1) in
let output =
padme 19 aname ^ padme 13 asize ^ padme 20 atype
^ padme 20 amount ^ padme 11 ro ^ "\n"
in
print_string output
else ()
else ()
in
let glabeltxt_to_nam2dev (lines : string list) =
let nam_dev_lst : (Nam.t * Dev.t) list = List.map glabelline lines in
Nam2dev.of_list nam_dev_lst
in
let glabeltxt : string list = read_process_lines "glabel status | awk '{if (NR!=1) print $1,$3}'" in
let mounttxt_to_kv ~nam2dev (lines : string list) =
List.fold_right
(fun line (dev2mount, dev2ro) -> mountline ~nam2dev ~dev2mount ~dev2ro line)
lines
(Dev2mount.empty, Dev2ro.empty)
in
let mounttxt : string list = read_process_lines "mount" in
let ntfstxt_to_kv ~dev2mount ~dev2ro (lines : string list) =
List.fold_right
(fun line (dev2mount, dev2ro) -> ntfsline ~dev2mount ~dev2ro line)
lines
(dev2mount, dev2ro)
in
let ntfstxt : string list = read_process_lines "pgrep ntfs-3g" in
let print_gparttxt ~dev2mount ~dev2ro (lines : string list) =
List.iter
(fun line -> print_gpartline ~dev2mount ~dev2ro line)
lines
in
let gparttxt : string list = read_process_lines "gpart show -p" in
let nam2dev = glabeltxt_to_nam2dev glabeltxt in
let dev2mount, dev2ro = mounttxt_to_kv ~nam2dev mounttxt in
let dev2mount', dev2ro' = ntfstxt_to_kv ~dev2mount ~dev2ro ntfstxt in
print_gparttxt ~dev2mount:dev2mount' ~dev2ro:dev2ro' gparttxt
2 Likes