How to improve this code?

1

I am parsing json and wrote this code

let getInt = (obj: Js.Dict.t<Js.Json.t>, key: string) : int => {
  switch obj -> Js.Dict.get(key) {
  | Some(valueObj) => 
    switch valueObj -> classify {
    | JSONNumber(num) => num -> Belt.Int.fromFloat
    | _ => raise(PARSE_FAILED(`${key} is not a number`))
    }
  | None => raise(PARSE_FAILED(`${key} not found`))
  }
}
let getValue = (obj: Js.Dict.t<Js.Json.t>, key: string) : string => {
  switch obj -> Js.Dict.get(key) {
  | Some(valueObj) => 
    switch valueObj -> classify {
    | JSONString(value) => value
    | JSONNull => ""
    | _ => raise(PARSE_FAILED(`${key} is not a string`))
    }
  | None => raise(PARSE_FAILED(`${key} not found`))
  }
}

These two pieces of code are almost identical but differ only in the pattern matching. Usually in such cases I pass a function as an argument which contains the “diff” of the logic into a function which contains common code.

But I cannot do that here because the different is the pattern matching logic which cannot be passed as a function.

How can I make this code DRY?

1 Like
2

You can put the outer, more general pattern match into a callback function that returns a generic type ('a).

let getValue = (obj: Js.Dict.t<Js.Json.t>, key: string, fn: Js.Json.t => 'a) =>
  switch obj->Js.Dict.get(key) {
  | Some(valueObj) => fn(valueObj)
  | None => raise(PARSE_FAILED(`${key} not found`))
  }

let getInt = (obj: Js.Dict.t<Js.Json.t>, key: string): int =>
  obj->getValue(key, valueObj =>
    switch valueObj->classify {
    | JSONNumber(num) => num->Belt.Int.fromFloat
    | _ => raise(PARSE_FAILED(`${key} is not a number`))
    }
  )

let getValue = (obj: Js.Dict.t<Js.Json.t>, key: string): string =>
  obj->getValue(key, valueObj =>
    switch valueObj->classify {
    | JSONString(value) => value
    | JSONNull => ""
    | _ => raise(PARSE_FAILED(`${key} is not a string`))
    }
  )
1 Like
3

A couple more options just to show some variety…not saying any of these is better than fham’s answer, just wanted to show a couple of other ways.

(playground)

Mostly they take advantage of the fact that you are raising on any non-happy path.

let getInt = (obj: Js.Dict.t<Js.Json.t>, key: string): int => {
  switch obj->Js.Dict.get(key)->Js.Option.getExn->Js.Json.classify {
  | JSONNumber(num) => num->Belt.Int.fromFloat
  | _ => raise(...whatever...)
  }
}

Here is a pipeline version…

  • using getExn to raise rather than a custom exception to unwrap the option
  • using Js.Json.decodeNumber because you only care about the number case in your example
  • You do lose your nicer error message this way, but you could write a custom getExn that takes an argument for the error message you want.
let getInt = (obj: Js.Dict.t<Js.Json.t>, key: string): int => {
  obj
  ->Js.Dict.get(key)
  ->Js.Option.getExn
  ->Js.Json.decodeNumber
  ->Belt.Option.map(Belt.Int.fromFloat)
  ->Js.Option.getExn
}

Since we squash both exceptions into Option.getExn you could just do it once at the end if it is nicer to you…

  • Uses the flatMap instead of the first getExn 
let getInt = (obj: Js.Dict.t<Js.Json.t>, key: string): int => {
  obj
  ->Js.Dict.get(key)
  ->Belt.Option.flatMap(Js.Json.decodeNumber)
  ->Belt.Option.map(Belt.Int.fromFloat)
  ->Js.Option.getExn
}

Finally just to show that it is clean enough to return an option…

let getInt = (obj: Js.Dict.t<Js.Json.t>, key: string): option<int> => {
  obj
  ->Js.Dict.get(key)
  ->Belt.Option.flatMap(Js.Json.decodeNumber)
  ->Belt.Option.map(Belt.Int.fromFloat)
}
2 Likes
4

Thanks Ryan, I really like your solution. Now I can write getInt and getString as a single function getValue

open Js.Json
open Belt.Option
open Belt.Int

let getValue = (json: Js.Json.t, key: string, fn: (Js.Json.t => option<'a>)) : 'a => {
  json 
  	-> decodeObject
  	-> flatMap(dict => dict -> Js.Dict.get(key))
  	-> flatMap(fn)
  	-> getExn
}

let json = "{'abc': 'def', 'ghi': 10}" -> parseExn
let x : string = json -> getValue("abc", decodeString)
let y : int = json -> getValue("ghi", obj => {obj -> decodeNumber -> map(fromFloat)})
5

I recommend using a parsing library. The code will be more declarative and clean, as well as more performant.

Here’s an example with rescript-struct (which I develop):

let data = `{"abc": "def", "ghi": 10}`->Json.parseExn
let pointStruct = S.object(o => {
  "x": o->S.field("abc", S.string()),
  "y": o->S.field("ghi", S.float()),
})
let point = data->S.parseWith(pointStruct)->S.Result.getExn
2 Likes