Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(list-ops): add exercise list-ops to vlang track (resolves #144) #146

Merged
merged 3 commits into from
Aug 5, 2023

Conversation

m-charlton
Copy link
Contributor

General

Found this initially difficult. Though once I understood that an array could be a generic parameter, everything dropped into place. I've given this a difficulty rating of 5. What do you think?

This exercise requires an understanding of generics and touches on some aspects of functional programming techniques (mapping, filtering, folding, ..., etc). Do these topics need to be mentioned anywhere as a prerequisite in the prerequisites field of list-ops/config.json?

Implementation (list-ops.v)

As per instructions the implementation uses no inbuilt functions/methods. I do call the .len property on an array though, but the length method could be used in its place.

All code has been tested with 2D arrays as input.

The map function has been changed to map_of to avoid a conflict with Vs built in map datatype.

Tests (run_test.v)

All tests explicitly state the types of the generic parameters even if the compiler could infer them. The main reason for this is that the testcases will give the student a hint as to how to properly use generics to solve the
problem, especially when it comes to multi-dimensional (e.g. []int)

test_concat_empty_lists(line 25). According to the problem specification expects an input of [] & an output of []. Because V is statically typed the input has been changed to [[]].

test_foldl_with_direction_dependent_function (line 113) - uses the suggested test input from UUID d2cf5664-... due the integer division issue.

This is the test scenario I want you to examine test_foldr_with_non_empty_list_directional_dependent_function (line 133). From the problem specification:

{
      "uuid": "be396a53-c074-4db3-8dd6-f7ed003cce7c",
      "description": "direction dependent function applied to non-empty list",
      "comments": [
        "Expects integer division (expects / to discard fractions).       "
      ],
      "property": "foldr",
      "input": {
        "list": [2, 5],
        "initial": 5,
        "function": "(x, y) -> x / y"
      },
      "expected": 2
}

I would expect 0 as the fractional part is discarded and I'm expecting 'x' to be the accumulator and 'y' the element. If the roles are transposed (i.e. x -> y, y -> x) then, we will get 2. There's a discussion here.

I follow the V convention of specifying the accumulator as the first argument and the element as the second. See arrays.reduce & arrays.fold.

test_foldr_direction_dependent_function_applied_to_non_empty_list (line 151). Uses data from be396a53- due to discarding fractional part after division.

@1ethanhansen
Copy link
Contributor

Honestly, I would probably bump the difficulty up to 6. This one requires a decent amount of code and understanding lots of different functions.

The prerequisites field of list-ops/config.json is specifically if there are concept exercises (as opposed to practice exercises) that are prereqs for this. Once we have some of those functional concepts as concept exercises, we should start updating existing prerequisite fields, but leaving that blank is good for now.

For the foldr test on line 133, your solution seems reasonable to me. I'm not even sure how the canonical data is supposed to be equal to 2 (based on the comment showing what x and y correspond to (accumulator and element)).

Any reason this is still marked as draft?

@hraftery
Copy link
Contributor

hraftery commented Aug 2, 2023

I'm not even sure how the canonical data is supposed to be equal to 2 (based on the comment showing what x and y correspond to (accumulator and element)).

Same. Curiosity led me to petertseng's explanation:

note for posterity: The possible ambiguous interpretation for this function were:

  • acc, el -> acc / el

    • acc: 5, el: 5. new acc 1
    • acc: 1, el: 2. new acc 0
  • acc, el -> el / acc

    • acc: 5, el: 5. new acc 1
    • acc: 1, el: 2. new acc 2

Would never have occurred to me to use the second method, but that explains it.

@1ethanhansen
Copy link
Contributor

@hraftery ahhhh okay so in order to get that interpretation, you have to have (element, accumulator) -> element / accumulator AND traverse the array right-to-left, yeah?

@m-charlton m-charlton marked this pull request as ready for review August 3, 2023 12:17
@m-charlton
Copy link
Contributor Author

Thanks for the extra pair of eyes. Now marked as ready for review. I'll push the difficulty up to 6 and update this PR.

Copy link
Contributor

@1ethanhansen 1ethanhansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change requested, otherwise THANK YOU! This was a big lift, and much appreciated

exercises/practice/list-ops/list-ops.v Outdated Show resolved Hide resolved
Co-authored-by: Ethan Hansen <1ethanhansen@protonmail.com>
Copy link
Contributor

@1ethanhansen 1ethanhansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@1ethanhansen 1ethanhansen merged commit 426c221 into exercism:main Aug 5, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants