Help: Verbose Unwrapping Logic

1

I have some rescript code that unwraps form data and saves it to a gist. The Form handles three fields.

{
  role: option<string>,
  inviteLink: option<string>,
  sponsorshipAddress: option<string>
}

Is there any better way to clean up the unwrapping part of this code so I can improve readability.

let action: Remix.actionFunction<'a> = async ({request, params}) => {
  open Webapi.Fetch
  let data = await Request.formData(request)
  let headers = HeadersInit.make({
    "Authorization": `Bot ${botToken}`,
  })
  let init = RequestInit.make(~method_=Patch, ~headers, ())

/// Unwrapping
  let role = data->Webapi.FormData.get("role")
  let inviteLink = data->Webapi.FormData.get("inviteLink")
  let sponsorshipAddress = data->Webapi.FormData.get("sponsorshipAddress")

//Unwrapping
  switch (role, inviteLink, sponsorshipAddress) {
  | (Some(role), Some(inviteLink), Some(sponsorshipAddress)) =>
    switch (
      role->Webapi.FormData.EntryValue.classify,
      inviteLink->Webapi.FormData.EntryValue.classify,
      sponsorshipAddress->Webapi.FormData.EntryValue.classify,
    ) {
// Unwrapping
    | (#String(role), #String(inviteLink), #String(sponsorshipAddress)) => {
        open WebUtils_Gist
        let config = makeGistConfig(
          ~id=Remix.process["env"]["GIST_ID"],
          ~name="guildData.json",
          ~token=Remix.process["env"]["GITHUB_ACCESS_TOKEN"],
        )
        let content = await ReadGist.content(~config, ~decoder=Shared.Decode.Gist.brightIdGuilds)
        let prevEntry = switch content->Js.Dict.get(guildId) {
        | Some(entry) => entry
        | None => GuildDoesNotExist(guildId)->raise
        }
        await UpdateGist.updateEntry(
          ~content,
          ~key=guildId,
          ~config,
          ~entry={
            ...prevEntry,
            role: Some(role),
            inviteLink: Some(inviteLink),
            sponsorshipAddress: Some(sponsorshipAddress),
          },
        )
      }

    | _ =>
      Remix.redirect(`/guilds/${guildId}/admin`)->ignore
      EmptySubmit->raise
    }
  | _ =>
    Remix.redirect(`/guilds/${guildId}/admin`)->ignore
    EmptySubmit->raise
  }
}
1 Like
2

So if all of role, inviteLink, or sponsorshipAddress are Some(#String(value)) then you treat that as your “success” case. But any other situation (i.e., any one of the role, inviteLink, or sponsorshipAddress is either None or not #String, ie, #File(...), that should be treated as a failure right?

Option 1: helper functions

Well, to get rid of some of that nested switching, I would make some helper functions. (Note, I annotated the types of these functions just so it’s clear.)

First a function to get the classify result (a polymorphic variant) back into the Option type.

// If the entryValue is #String(x) then return Some(x), else return None.
let some_if_string: Webapi.FormData.EntryValue.t => option<string> = entryValue => {
  switch Webapi.FormData.EntryValue.classify(entryValue) {
  | #String(x) => Some(x)
  | #File(_) => None
  }
}

Since that function has a signature of the form 'a => option<'b>, that means we can use it as the 2nd argument of Belt.Option.flatMap. So we pipe the output of Webapi.FormData.get into flatMap which will “get” the value if it is there and is not a file. Check it out:

// If the `field` is present in the `formData` and it is not a file, return `Some(value)`, else `None`.
let get_if_string: (Webapi.FormData.t, string) => option<string> = (formData, field) => {
  Webapi.FormData.get(formData, field)->Belt.Option.flatMap(some_if_string)
}

Now your switch becomes a single level:

let data = await Request.formData(request)

... more stuff ...

switch (
  get_if_string(data, "role"),
  get_if_string(data, "inviteLink"),
  get_if_string(data, "sponsorshipAddress"),
) {
| (Some(role), Some(inviteLink), Some(sponsorshipAddress)) => {
    // ... do something with your good data ...
  }
| _ => // ... handle the failure ...

That’s cool and all, but I still don’t like that sort of messy switch there for my caller to deal with. Let’s try something else.

Option 2: hide form logic in a module

Since your form is fixed, why not make a module and hide all the gory details? We can just call it Form for now.

module Form: {
  // It's private so you can only construct it through the `make` function, but
  // you can get still destructure the values..
  type t = private {
    role: string,
    inviteLink: string,
    sponsorshipAddress: string,
  }

  let make: Webapi.FormData.t => option<t>
} = {
  type t = {
    role: string,
    inviteLink: string,
    sponsorshipAddress: string,
  }

  // Assumes the helper functions from above are defined here or at least available here.

  // You could imagine using Result type here if you need to track errors or whatever.
  let make = formData => {
    switch (
      get_if_string(formData, "role"),
      get_if_string(formData, "inviteLink"),
      get_if_string(formData, "sponsorshipAddress"),
    ) {
    | (Some(role), Some(inviteLink), Some(sponsorshipAddress)) =>
      Some({role, inviteLink, sponsorshipAddress})
    | _ => None
    }
  }
}

Couple things to note:

  • The type Form.t doesn’t have any options in it. Makes it nicer to deal with that way.
  • Since all three elements have to be there and all have to be #String or the whole thing is invalid, let your make function deal with that logic. It will either return Some(t) if the form is good, or None if it is bad.

And now your consumer code gets cleaned up nicely:

let action: Remix.actionFunction<'a> = async ({request, params}) => {
  let data = await Request.formData(request)

  ... stuff ...

  switch Form.make(formData) {
    | Some({role, inviteLink, sponsorshipAddress}) => // ... do stuff with your good data ...
    | None => // ... handle the error ...
  }
}

It’s a lot simpler in the consumer code, because the form data as a whole can only be in two states: valid or not. So it only has to deal with that. The Form module itself deals with all the details.

5 Likes
3

This is exactly the kind of detail I needed. Thanks a bunch

But any other situation (i.e., any one of the role, inviteLink, or sponsorshipAddress is either None or not #String, ie, #File(…), that should be treated as a failure right?

Actually I’d rather be able to handle the intersecting cases as well. e.g. (Some(role), None, None) , ( None, Some(inviteLink), Some(sponsorshipAddress)), etc.

I shaped it this way to avoid verbosity

1 Like
4

Ah, I see, that makes sense…sorry for my misinterpretation. But hopefully it still gives you some useful ideas to try out.

5

Trying to make it so if the value is None, it is not included in the record and instead uses the spread. I might just be massively brain farting here. I plan on extending this form to contain more fields, this doesn’t seem feasible at scale.

It might just be simpler to just pass the entire object every time like I was doing, but if you have any advice it is much appreciated.

  switch (role, inviteLink, sponsorshipAddress) {
  | (Some(role), Some(inviteLink), Some(sponsorshipAddress)) =>
    await UpdateGist.updateEntry(
      ~content,
      ~key=guildId,
      ~config,
      ~entry={
        ...prevEntry,
        role: Some(role),
        inviteLink: Some(inviteLink),
        sponsorshipAddress: Some(sponsorshipAddress),
      },
    )
  | (None, Some(inviteLink), Some(sponsorshipAddress)) =>
    await UpdateGist.updateEntry(
      ~content,
      ~key=guildId,
      ~config,
      ~entry={
        ...prevEntry,
        inviteLink: Some(inviteLink),
        sponsorshipAddress: Some(sponsorshipAddress),
      },
    )
  | (Some(role), None, Some(sponsorshipAddress)) =>
    await UpdateGist.updateEntry(
      ~content,
      ~key=guildId,
      ~config,
      ~entry={
        ...prevEntry,
        role: Some(role),
        sponsorshipAddress: Some(sponsorshipAddress),
      },
    )
  | (Some(role), Some(inviteLink), None) =>
    await UpdateGist.updateEntry(
      ~content,
      ~key=guildId,
      ~config,
      ~entry={
        ...prevEntry,
        role: Some(role),
        inviteLink: Some(inviteLink),
      },
    )
  | (None, None, Some(sponsorshipAddress)) =>
    await UpdateGist.updateEntry(
      ~content,
      ~key=guildId,
      ~config,
      ~entry={
        ...prevEntry,
        sponsorshipAddress: Some(sponsorshipAddress),
      },
    )

  | (None, Some(inviteLink), None) =>
    await UpdateGist.updateEntry(
      ~content,
      ~key=guildId,
      ~config,
      ~entry={
        ...prevEntry,
        inviteLink: Some(inviteLink),
      },
    )
  | (Some(role), None, None) =>
    await UpdateGist.updateEntry(
      ~content,
      ~key=guildId,
      ~config,
      ~entry={
        ...prevEntry,
        role: Some(role),
      },
    )
  | (None, None, None) => EmptySubmit->raise
  }
6

What is the type of prevEntry?

Edit: i assume it is this?

{
  role: option<string>,
  inviteLink: option<string>,
  sponsorshipAddress: option<string>
}
7 
type brightIdGuild = { 
   role: option<string>, 
   name: option<string>, 
   inviteLink: option<string>, 
   roleId: option<string>, 
   sponsorshipAddress: option<string>, 
   usedSponsorships: option<int>, 
   assignedSponsorships: option<int> 
 }
8

And what fields do you expect your form (the FormData.t value) to have…exactly same ones as in the brightIdGuild type?

Another question just so I’m clear: say in the form data, you have role = None. From your example code, in that case, it looks like you want to completely ignore role and leave whatever value was in the role field in prevEntry correct? In other words, you do not want a None in the form to flip something in prevEntry to None, is that correct? If so, do you want to ever be able to flip something from Some to None in previous entry based on the info you get back from the form?

9

Looking at just this example, can’t you do something like this

let entry = switch (role, inviteLink, sponsorshipAddress) {
  | (None, None, None) => EmptySubmit->raise
  | (role, inviteLink, sponsorshipAddress) => {
    let entry = Option.isSome(role) ? {...prevEntry, role: role} : prevEntry
    let entry = Option.isSome(inviteLink) ? {...prevEntry, inviteLink: inviteLink} : entry
    let entry = Option.isSome(sponsorshipAddress) ? {...prevEntry, sponsorshipAddress: sponsorshipAddress} : entry
    entry
  }
}

await UpdateGist.updateEntry(~content, ~key=guildId, ~config, ~entry)

Sure probably not the pretty and elegant solution you’re askin for, but it is simple

10

I’d like to update my answer since I’ve gotten some sleep :stuck_out_tongue:

The sample above can be abstracted to an alt function that returns [a] if Some otherwise [b]. And then since you’re just raising an exception, could just knock that out first with a helper function that checks if at least one value is set

let alt = (a: option<'a>, b: option<'a>): option<'a> =>
  switch a {
  | Some(_) as some => some
  | None => b
  }

// ... 

let atleastOneSome = Js.Array2.some([
  role, 
  inviteLink, 
  sponsorshipAddress
 ], Belt.Option.isSome)
if !atleastOneSome { EmptySubmit->raise }

let entry = {
  ...prevEntry,
  role: role->alt(prevEntry.role),
  inviteLink: inviteLink->alt(prevEntry.inviteLink),
  sponsorshipAddress: sponsorshipAddress->alt(prevEntry.sponsorshipAddress)
}

await UpdateGist.updateEntry(~content, ~key=guildId, ~config, ~entry)
2 Likes
11

And what fields do you expect your form (the FormData.t value) to have…exactly same ones as in the brightIdGuild type?

The form will probably have most of these fields yes.

If so, do you want to ever be able to flip something from Some to None in previous entry based on the info you get back from the form?

So, how it is now, I pass all form data to the record. It would be better if a field has no change to label it as None and use prevEntry instead. I have no intention of changing it back to None.

12

In that case I would take a similar idea to hellos3b above and use a function like alt with an update function that takes the FormData and the previous entry…something like

let update = (prevEntry, formData) => {
  let role = 
    formData
    ->get_if_string("role")
    ->alt(prevEntry.role)
  
  let inviteLink =
    formData
    ->get_if_string("inviteLink")
    ->alt(prevEntry.inviteLink)
  
  let sponsorshipAddress =
    formData
    ->get_if_string("sponsorshipAddress")
    ->alt(prevEntry.sponsorshipAddress)
  
  {role, inviteLink, sponsorshipAddress}
}
2 Likes