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

custom-set: update, remove and re-order tests #454

Merged
merged 1 commit into from
Jan 21, 2017

Conversation

robphoenix
Copy link
Contributor

@robphoenix robphoenix commented Jan 17, 2017

@robphoenix
Copy link
Contributor Author

"I'll just quickly regenerate these custom-set tests" I thought.

Well, this was an interesting learning exercise. I got a wee bit carried away and I realise I've made some drastic changes to the exercise here, and my brain has melted a little bit, so am very open to any and all criticism and am OK with the possibility of discarding this. 🤷‍♂️

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

Sorry not done reviewing yet (ran out of time, will have to do again in a few hours)

I haven't done this particular exercise in Go, so that means to determine whether the idea to provide type Set map[string]bool is a good idea I can only phrase it in these questions:

  • If you've participated in conversations on this exercise, did people indicate that they enjoyed the challenge of coming up with what internal representation to use for the set? Or did they rather focus on implementing the set operations?
  • What about you when you did this exercise? Well, I assume you wanted to focus more on the latter, but should make sure I guess
  • When looking over others' solutions, do you see a lot of variation in the implementation of their sets? For example do some people just use a []string and make sure it has no duplicates on add? If pretty much everyone just used map[string]bool anyway, then we don't lose anything, but if people were, say, split 50/50 between map[string]bool vs []string, then maybe we have taken some freedom away.

testBinOp("SymmetricDifference", SymmetricDifference, symmetricDifferenceCases, t)
}

func BenchmarkNewFromSlice1e1(b *testing.B) { bench(1e1, b) }
Copy link
Member

Choose a reason for hiding this comment

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

does the benchmarking not make any sense anymore? or was it jus tmoved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing this was just an accident.

@robphoenix
Copy link
Contributor Author

Honestly, I've not done the exercise yet, and this is a rather opinionated change that has not come from a need to improve the exercise from it's current form. What I think I'll do is change it back to using the String as before, leaving the structure of what a set is open for interpretation, and then that change can be a separate PR if there's a qualified view that such a change needs to be done. Leaving this PR to just the updates from the canonical-data changes.

@robphoenix robphoenix force-pushed the custom-set/update-tests branch 2 times, most recently from cb5c519 to 001f868 Compare January 20, 2017 14:20
@robphoenix
Copy link
Contributor Author

robphoenix commented Jan 20, 2017

ok, I've updated this so that it just reflects the changes in the canonical data now, and rebased on master.

@robphoenix robphoenix force-pushed the custom-set/update-tests branch 2 times, most recently from a2300d1 to 1fb6875 Compare January 20, 2017 15:11
t.Fatalf("%v IsEmpty = %t, want %t", s, got, tc.want)
s.Add(tc.ele)
want := NewFromSlice(tc.want)
if !Equal(s, want) {
Copy link
Member

Choose a reason for hiding this comment

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

There's an interesting ordering quirk now that we've done this. In the previous version, the Equal test came very early. Notice how there is a test that says "With Equal tested, remaining tests use it to judge correctness." Whereas here, a test that uses Equal comes before the actual test of Equal, so it seems that we miss this property. This, I see, was a guiding desire for the reordering done in exercism/problem-specifications#257. Might be better to keep it that way. Unless there is a strong reason to do it this way (I think you made it so that all binary operations happen in sequence, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, yes, I remember what I did now, I had them all in the order suggested, and then forgot while cleaning things up a bit and grouped all the methods together, silly oversight on my part, thanks again for the catch @petertseng

if e == v {
return
// String returns a printable representation of s.
func (s Set) String() string {
Copy link
Member

Choose a reason for hiding this comment

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

I got a distinct sense of deja vu when reading this file, since it appears you made the exact same change in example.go. Then I realized, wow, we have two example files in here, one with []string and one with map[string]bool, and control which one to build with build flags. Though nowhere in Travis do we build with any build flags, so this means this file is never tested, and is susceptible to going out of date. Maybe one day we should test it. But that's a talk for another time. No change needed at this moment

@robphoenix
Copy link
Contributor Author

ok, I think that should be everything now?

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

just one more small thing I think (moving the order of the functions in the comments)

// Subset(s1, s2 Set) bool // return s1 ⊆ s2
// (s Set) IsEmpty() bool
// (s Set) Has(string) bool
// (s Set) Add(string)
Copy link
Member

Choose a reason for hiding this comment

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

Should these be listed in the same order they are tested? I think that was what you were going for. In that case, let's move this one down.

@robphoenix robphoenix merged commit c05964a into exercism:master Jan 21, 2017
@robphoenix robphoenix deleted the custom-set/update-tests branch January 21, 2017 20:59
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.

2 participants