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

zero_zilch_nada: remove ambiguous reference to empty #1500

Merged
merged 19 commits into from
Sep 22, 2021
Merged

zero_zilch_nada: remove ambiguous reference to empty #1500

merged 19 commits into from
Sep 22, 2021

Conversation

sudomateo
Copy link
Contributor

@sudomateo sudomateo commented Sep 4, 2021

In Go, there can be a difference between a type's zero value and its
empty value. For example, consider the following code snippet:

func main() {
	var s1 []string
	fmt.Printf("%#v\n", s1)
	if s1 == nil {
		fmt.Println("s1 is equal to nil")
	}

	s2 := []string{}
	fmt.Printf("%#v\n", s2)
	if s2 == nil {
		fmt.Println("s2 is equal to nil")
	}
}

The variable s1 will be set to its zero value (nil) whereas the
variable s2 will be set to a slice of strings with zero elements. This
behavior makes the term "empty" ambiguous and may confuse students as they
continue learning about Go.

In addition to code updates, the exercise text was updated to introduce
the var keyword as well as various verbiage updates.

Updated the documentation for the nil and zero-values concepts to better
reflect the new zero_zilch_nada exercise content. Long term, I'd
probably recommend merging the two concepts into one but for now keeping
their content the same ensures that students will learn the relevant
content regardless of which concept they click on.

In Go, there can be a difference between a type's zero value and its
empty value. For example, consider the following code snippet:

```go
func main() {
	var s1 []string
	fmt.Printf("%#v\n", s1)
	if s1 == nil {
		fmt.Println("s1 is equal to nil")
	}

	s2 := []string{}
	fmt.Printf("%#v\n", s2)
	if s2 == nil {
		fmt.Println("s2 is equal to nil")
	}
}
```

The variable `s1` will be set to its zero value (`nil`) whereas the
variable `s2` will be set to a slice of strings with zero elements. This
behavior makes the term "empty" ambiguous and may confuse students as they
continue learning about Go.

In addition to code updates, the exercise text was updated to introduce
the `var` keyword as well as various verbiage updates.
Updated the documentation for the nil and zero-values concepts to better
reflect the new `zero_zilch_nada` exercise content. Long term, I'd
probably recommend merging the two concepts into one but for now keeping
their content the same ensures that students will learn the relevant
content regardless of which conept they click on.
concepts/nil/about.md Outdated Show resolved Hide resolved
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this! I've left some comments to help explain what is expected in the various documents.

I don't know if you're interested, but this exercise could use a little love. In its current state it is, well, rather matter-of-factly. You just return a single value from the functions and there is no real story/theme around the functionality to give the students something fun to work with. Would you be interested in trying to add a bit more flavor to this exercise and make it more fun for students? If so, let me know, and I can help you with that.

concepts/nil/.meta/config.json Outdated Show resolved Hide resolved
concepts/nil/about.md Outdated Show resolved Hide resolved
concepts/nil/about.md Outdated Show resolved Hide resolved
concepts/nil/introduction.md Outdated Show resolved Hide resolved
concepts/nil/introduction.md Outdated Show resolved Hide resolved
exercises/concept/zero-zilch-nada/.docs/instructions.md Outdated Show resolved Hide resolved
concepts/zero-values/introduction.md Outdated Show resolved Hide resolved
concepts/zero-values/.meta/config.json Outdated Show resolved Hide resolved
concepts/zero-values/introduction.md Outdated Show resolved Hide resolved
concepts/zero-values/about.md Outdated Show resolved Hide resolved
Updated the concept documentation to focus on zero values.

Changed the exercise to be a bit more involved than the previous
one-liner.
@sudomateo
Copy link
Contributor Author

Thank you for the feedback! I reworked the zero-values concept per your feedback. I also updated the zero-zilch-nada exercise to be a bit more involved for the student.

I'll do the same for the nil concept once I get more time to work on this. For now I'm leaving conversations open around the nil concept.

@ErikSchierboom
Copy link
Member

I also updated the zero-zilch-nada exercise to be a bit more involved for the student.

Maybe I'm missing something, but the instructions of the exercise look to be relatively unchanged. With more involved, I meant giving the exercise a little bit of story/theme, to have the tasks make sense for the student (and not just a list of fill-in methods). Note that this story/theme doesn't have to be very involved, it can be relatively simple. For some inspiration, I'd look at existing exercises like https://github.com/exercism/elixir/blob/main/exercises/concept/dna-encoding/.docs/instructions.md or https://github.com/exercism/elixir/blob/main/exercises/concept/high-school-sweetheart/.docs/instructions.md

@junedev
Copy link
Member

junedev commented Sep 14, 2021

@sudomateo As a story I can recommend the amusement park story: https://github.com/exercism/javascript/blob/main/exercises/concept/amusement-park/.docs/instructions.md

Write some introduction sentences about working for the amusement park IT and being in charge of data quality of the visitor data etc.

Then you can set up the tasks the following way:

  • First Task: let the student write a NewVisitor function that creates and returns a visitor struct given some data as parameters. Describe in the task descriptions which fields the struct should have, e.g. name as string, age as int, address as map (street, city), etc. This task does not practice anything yet really, it is more for the setup.
  • Then the following tasks can be things like "Determine whether the visitor provided a name" where you pass a visitor struct as parameter and the function e.g. "nameIsSet" returns true or false depending on the value of that field.
  • I would see the above as the minimum to make this a bit more story-like. If you want to go further you can also mix it up with something like this task where you provide some map and the student needs to return some text: https://github.com/exercism/javascript/blob/main/exercises/concept/amusement-park/.docs/instructions.md#4-improve-the-ticket-status-response

I don't think it is necessary that the exercise covers all the types. Concentrate on a few tasks to cover the most common types. That's enough for the student to understand the idea.

If you go ahead with these types of tasks, don't forget to add structs as a prerequisite.

Erik says it is ok to change the slug and folder name to "amusement-park".

@ErikSchierboom
Copy link
Member

I don't think it is necessary that the exercise covers all the types. Concentrate on a few tasks to cover the most common types. That's enough for the student to understand the idea.

100% this ☝️

@sudomateo
Copy link
Contributor Author

Thank you for the feedback @ErikSchierboom and @junedev. I've been a bit busy in and out of work the past week. I'll try to take a look at this in the next few days. I like the idea of making the exercise a bit more story-like. When I first opened this pull request, I was under the impression I should leave the exercise alone. It's nice to see that I can help shape the exercise to be more engaging.

@jmrunkle
Copy link
Contributor

FYI: CI should be fixed at head if you do not mind syncing your fork with main and then rebasing this branch.

@sudomateo
Copy link
Contributor Author

I'm going modify the zero-zilch-nada exercise and rename it to census. This census exercise will ask the user to create new residents with their necessary information, check whether or not required information is missing from a given resident, allow the counting of valid residents with all required information present, and allow residents to "move out" by setting their fields to their zero value. It's similar to the amusement park exercise with a bit more focus on zero values. I'll update this pull request once I finish the exercise. Apologies for the delay, last minute travel plans.

@ErikSchierboom
Copy link
Member

I'm going modify the zero-zilch-nada exercise and rename it to census. This census exercise will ask the user to create new residents with their necessary information, check whether or not required information is missing from a given resident, allow the counting of valid residents with all required information present, and allow residents to "move out" by setting their fields to their zero value. It's similar to the amusement park exercise with a bit more focus on zero values. I'll update this pull request once I finish the exercise. Apologies for the delay, last minute travel plans.

That sounds excellent.

config.json Outdated Show resolved Hide resolved
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

This looks great! I'll let one of the Go maintainers review the Go side of things.

Thanks a lot!

config.json Outdated Show resolved Hide resolved
concepts/nil/about.md Outdated Show resolved Hide resolved
concepts/nil/introduction.md Outdated Show resolved Hide resolved
concepts/nil/about.md Outdated Show resolved Hide resolved
exercises/concept/census/.docs/hints.md Outdated Show resolved Hide resolved
exercises/concept/census/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/census/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/census/.docs/instructions.md Outdated Show resolved Hide resolved
@ErikSchierboom
Copy link
Member

There is a merge conflict.

config.json Outdated Show resolved Hide resolved
exercises/concept/census/.docs/hints.md Outdated Show resolved Hide resolved
exercises/concept/census/.docs/hints.md Outdated Show resolved Hide resolved
exercises/concept/census/.docs/hints.md Outdated Show resolved Hide resolved
exercises/concept/census/.docs/hints.md Outdated Show resolved Hide resolved
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Nice! @junedev could you look at the Go side of things?

concepts/nil/about.md Outdated Show resolved Hide resolved
concepts/nil/about.md Outdated Show resolved Hide resolved
concepts/nil/about.md Outdated Show resolved Hide resolved
concepts/zero-values/about.md Show resolved Hide resolved
concepts/zero-values/links.json Outdated Show resolved Hide resolved
config.json Outdated Show resolved Hide resolved
exercises/concept/census/.docs/hints.md Outdated Show resolved Hide resolved
exercises/concept/census/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/census/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/census/census_test.go Show resolved Hide resolved
@junedev
Copy link
Member

junedev commented Sep 21, 2021

Nice! @junedev could you look at the Go side of things?

Done

- Ensure the links in the hints are inline.
- Ensure the introduction includes information on struct types.
- Restore the exercise code to an unfinished state.
- Update the testing code.
@sudomateo sudomateo requested a review from junedev September 22, 2021 04:41
@junedev
Copy link
Member

junedev commented Sep 22, 2021

@sudomateo The last thing that remains is getting the CI to pass.

For the prerequisites you need to write "string-formatting" instead of "strings" for now. (It will be changed later when "strings" becomes available. Additionally you need to remove "pointers" for now as it does not exists yet. You can leave a comment here #1643 that it needs to be added again later.

@junedev
Copy link
Member

junedev commented Sep 22, 2021

As for the other CI failures, can you try to use "package census" instead of "package census_test" for the test file? This is how it was done in the other concept exercises. (Not sure this will fix the issue though.)

@iHiD iHiD added the x:size/large Large amount of work label Sep 22, 2021
Copy link
Contributor

@ekingery ekingery left a comment

Choose a reason for hiding this comment

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

Rubber stamping based on the other reviews / fixes and the tests passing.

@sudomateo
Copy link
Contributor Author

Fixed the merge conflict.

@ekingery
Copy link
Contributor

Fixed the merge conflict.

By the way, thanks for your work on this exercise, @sudomateo! Looks great.

@sudomateo
Copy link
Contributor Author

Fixed the merge conflict.

By the way, thanks for your work on this exercise, @sudomateo! Looks great.

Anytime! I'm happy to help!

@ekingery ekingery merged commit c67eb70 into exercism:main Sep 22, 2021
@sudomateo sudomateo deleted the sudomateo/zero-value branch September 22, 2021 16:25
@ErikSchierboom
Copy link
Member

Yeah, great work @sudomateo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants