Result concept exercise (initial draft)

[blackk-foxx]

This is not ready for a full review. At this point, I am seeking only high-level feedback on the initial draft, which hopefully conveys the intended design of the exercise.

TODO:

• Either rename the exercise to "Drink Recipes", or change the content to be food-oriented instead of alcohol-oriented.
• Add required infrastructure files
• Add concept doc
• Add exercise instructions

[github-actions]

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.

---

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

[blackk-foxx]

At this point, I think exemplar.fs and FamilyRecipesTests.fs are complete in terms of design. This version includes integer parsing, but not fractions as @BNAndras proposed. But I think it is complex enough as is. Other opinions?

[ErikSchierboom]

I'll try and take a look soon! Sorry for the delay

[ErikSchierboom]

I like the general idea! My main comment is that I feel like the solution is too complex, as it requires recursion and mutually recursive functions. I feel like that's a bit much.

[blackk-foxx]

I like the general idea! My main comment is that I feel like the solution is too complex, as it requires recursion and mutually recursive functions. I feel like that's a bit much.

Thanks for the review! My goal with the exemplar was to demonstrate a purely functional solution with immutable data. Instead, I could rewrite it as a loop with mutable state.

[blackk-foxx]

I like the general idea! My main comment is that I feel like the solution is too complex, as it requires recursion and mutually recursive functions. I feel like that's a bit much.

Thanks for the review! My goal with the exemplar was to demonstrate a purely functional solution with immutable data. Instead, I could rewrite it as a loop with mutable state.

Even better: Here is a purely functional solution without any mutable data, based on fold. How does this grab you?

[ErikSchierboom]

Let's take a small step back to discuss the basics of concept exercises. They should be about one specific concept, in this case results. Anything else that is being used should be absolutely necessary. For example, this exercise uses both strings and integers, so those must be prerequisites: things that the student must have learnt in other concept exercises before this exercise is. This exercise also uses records, lists, options and recursion/folds. This is not necessarily wrong, but we should try to minimize the number of concepts the student needs to know.

It might be useful to make a short list of the things the exercise wants to teach the student. I'd say it boils down to:

• Returning an Ok value
• Returning an Error value
• Using a Result value via pattern matching
• Using a function from the Result module (e.g. Result.defaultValue)

Maybe you could try simplifying the exercise a bit more? With simplifying I'm thinking of:

• Make the exercise shorter: the exemplar implementation should ideally be the one best solution to the exercise. The smaller the exercise is, the more likely it is that is true.
• Each part of the exercise should be clearly linked to one of the teaching goals of the exercise. If there are multiple functions that teach the same thing, remove the others to end up with just one function.
• Remove the list iteration/recursion/folding
• Don't use tuples as the Ok type (e.g. Result<ParseState * Recipe, ParseError>)
• Simplify the tasks for the user. The exercise as it is is very interesting and would make for a good practice exercise as it "makes sense," but a concept exercise can (and should) be quite minimal and barebones. It doesn't have to be complete.

Does this make sense?

[blackk-foxx]

...Does this make sense?

Thanks for the correction, Erik. Yes, it makes sense. I'm now thinking that maybe this exercise would focus on parsing only the ingredients list. I'll post something within the next 1-2 days.

[ErikSchierboom]

Thanks for being so patient! Creating Concept Exercises is quite hard!

[blackk-foxx]

The new version reduces the scope of the exercise to validate only the ingredients list.

[blackk-foxx]

The purpose of this pre-written function is to provide the student an opportunity to practice consuming a Result, since the standard library doesn't really provide any. I'm not really happy with it, though -- the student could decide to ignore it or remove it altogether. I guess it could be moved to a separate module that is read-only for the user, and maybe the tests could be designed (somehow) to ensure that it is called. But that seems overly pedantic to me.

Another idea is to introduce a mock readFile API that returns a Result, and the tests could be structured so that the given input is a fake filename, and the readFile API would return either an Error, or Ok with the ingredients text for the given test case.

Other thoughts?

[ErikSchierboom]

Ehm, I'm not entirely. I'll try and find some time this week to do a proposal for some further changes, okay?

[ErikSchierboom]

Okay, I've tried to come up with something I hope is even simpler and omits as much boilerplate as can be:

module FamilyRecipes

open System

type RecipeError =
    | InvalidQuantity 
    | MissingIngredient
    | UnknownIngredient

let parseQuantity (quantity: string): Result<int, RecipeError> =
    match Int32.TryParse (quantity.Trim()) with
    | true, value -> Ok value
    | false, _ -> Error InvalidQuantity
    
let parseIngredient (ingredient: string): Result<string, RecipeError> =
    match ingredient.ToLower() with
    | "" -> Error MissingIngredient
    | "apples" -> Ok "apples"
    | "lemons" -> Ok "lemons"
    | "honey" -> Ok "honey"
    | _ -> Error UnknownIngredient
    
let printIngredient (ingredient: string): string =
    match parseIngredient ingredient with
    | Ok parsedIngredient-> sprintf "Ingredient: %s" parsedIngredient
    | Error _ -> "Invalid ingredient"

The key things are:

1. We no longer parser multiple lines but are just given the individual strings to parse
2. We slowly build up the solution by having the student implement three functions
3. parseIngredient is all about returning Ok and Error values as well as showing that parsing can also be about transforming (match on any casing and return lowercase)
4. parseQuantity is all about dealing with the fact that the BCL does not use the result type and that you need to write wrappers around its methods to work with results
5. printIngredient is about pattern matching or, if the student prefers, using Result.map and Result.defaultValue via:

let printIngredient (ingredient: string): string =
    parseIngredient ingredient
    |> Result.map (sprintf "Ingredient: %s")
    |> Result.defaultValue "Invalid recipe"

What do you think?

[blackk-foxx]

What do you think?

I mostly like it. I like the simplicity of parsing just a single ingredient at a time, and I like how it teaches both the creation and consumption of Result values. But have a couple of reservations:

1. Only ingredients within a predetermined, hard-coded set are considered to be valid.
2. parseQuantity is not integrated with the rest of the solution.

What would you think about replacing parseIngredient with parseItem with the following signature:

let parseItem (item: string) (itemsOnHand: string list): Result<string, RecipeError>

where itemsOnHand represents the list of known ingredients.

Also, why not allow a valid ingredient to be an integer followed by a known item? Then printIngredient would call both parseQuantity and parseIngredient.

[ErikSchierboom]

Only ingredients within a predetermined, hard-coded set are considered to be valid.
Also, why not allow a valid ingredient to be an integer followed by a known item?

Why is this a reservation? Is this because it doesn't feel "real?" If so, concept exercises don't have to be real, they just need to teach a student a concept. I feel like you're maybe trying to make it make "sense" more than it needs to be.

parseQuantity is not integrated with the rest of the solution.

That is true. I tried integrating it and it made the solution more complicated. Again, the goal is not to have everything make perfect sense, as long as it provides some well-defined tasks for the student that help teach the student.

Also, why not allow a valid ingredient to be an integer followed by a known item? Then printIngredient would call both parseQuantity and parseIngredient.

Could you maybe show the code what that would look like?

[blackk-foxx]

Also, why not allow a valid ingredient to be an integer followed by a known item? Then printIngredient would call both parseQuantity and parseIngredient.

Could you maybe show the code what that would look like?

I envision renaming your proposed parseIngredient to parseItem, and adding a parseIngredient function like the following:

let parseIngredient (ingredient: string): Result<string, IngredientError> =
    match ingredient.IndexOf(' ') with
    | -1 -> Error MissingIngredientItem
    | separatorPos ->
        let quantity = ingredient.Substring(0, separatorPos);
        let item = ingredient.Substring(separatorPos + 1);
        match (parseQuantity quantity, parseItem item) with
        | (Ok _, Ok _) -> Ok ingredient
        | (Ok _, Error e) -> Error e
        | (Error e, _) -> Error e

Only ingredients within a predetermined, hard-coded set are considered to be valid.
Also, why not allow a valid ingredient to be an integer followed by a known item?

Why is this a reservation? Is this because it doesn't feel "real?" If so, concept exercises don't have to be real, they just need to teach a student a concept. I feel like you're maybe trying to make it make "sense" more than it needs to be.

I understand and accept your point. But I would like to try come up with an engaging exercise, like the role playing game, and I fear that an exercise about recipes, but with so many restrictions, might not be very engaging. Maybe recipes aren't the best use case for a concept exercise, since recipes by their nature have a lot of variability.

What would you think about a simple arithmetic calculator as an alternative? The input would be a string in a very strict format: <integer> <space> <operator> <space> <integer>. The error cases would be InvalidOperand, UnknownOperator, and InvalidFormat. It could be built up incrementally as in your proposal, and the function signatures might look something like

let parseOperand (quantity: string): Result<int, ParseError> =
let parseOperator (operator: string): Result<char, ParseError> =
let evaluateExpression (text: string): Result<int, ParseError> =

BTW, thank you also for your patience with me, as I try to muddle through this process.

[ErikSchierboom]

I envision renaming your proposed parseIngredient to parseItem, and adding a parseIngredient function like the following:
[...]

To me,. that implementation is a bit messy, especially this part:

| (Ok _, Ok _) -> Ok ingredient
| (Ok _, Error e) -> Error e
| (Error e, _) -> Error e

The ignoring of the Ok cases inner values is a code smell to me.

I understand and accept your point. But I would like to try come up with an engaging exercise, like the role playing game, and I fear that an exercise about recipes, but with so many restrictions, might not be very engaging. Maybe recipes aren't the best use case for a concept exercise, since recipes by their nature have a lot of variability.

I totally understand that. But did you see how focused and simple that exercise is? It's basically only focusing on the concept at hand, without any distractions:

module RolePlayingGame

type Player = { 
    Name: string option
    Level: int
    Health: int
    Mana: int option
}

let introduce (player: Player): string = 
    Option.defaultValue "Mighty Magician" player.Name

let revive (player: Player): Player option = 
    if player.Health > 0 then
        None

    elif player.Level >= 10 then
        Some { player with Health = 100; Mana = Some 100 }

    else
        Some { player with Health = 100; Mana = None }

let castSpell (manaCost: int) (player: Player): Player * int =
    match player.Mana with
    | None ->
        { player with Health = max 0 (player.Health - manaCost) }, 0

    | Some mana ->
        if mana >= manaCost then
            { player with Mana = Some (mana - manaCost) }, 2 * manaCost

        else
            player, 0

That is how concept exercises should be designed IMHO.

What would you think about a simple arithmetic calculator as an alternative?

To be honest: more boring that recipes. Maybe we can change the theme to something else, like a treasure map's instructions or an encrypted message or something like that.

Sorry, this is not what you want to hear :)

[blackk-foxx]

But did you see how focused and simple that exercise is? It's basically only focusing on the concept at hand, without any distractions

Yes. I get it.

To be honest: more boring that recipes. Maybe we can change the theme to something else, like a treasure map's instructions or an encrypted message or something like that.

I agree; it would be pretty boring. I'll try to come up with an engaging alternative that would naturally fit with a focused approach like the role-playing game.

Sorry, this is not what you want to hear :)

Perhaps, but it is probably what I need to hear. :-)

I appreciate your honesty and your constructive style of communication.

[ErikSchierboom]

And thank you for being so receptive and willing to take feedback!