Reorganize data-fetching code / add tests #4

reagent · 2017-11-07T04:37:11Z

This is a rather big change -- my main goal was to introduce a Client interface to allow for dependency injection when testing. In addition, I reorganized the data structures a bit and was able to remove some duplicate code.

* Added a new `Client` interface to facilitate testing * Removed duplication when providing meetup names * Pushed event fetching and sorting to new Schedule struct * Simplified templates by rendering events inline * Added tests for `data` package

jasonkeene

There was a number of issues with this PR. The main one being the deadlock in Poll. I was thinking perhaps we can make these changes together in a recorded pairing session that gets posted on youtube/twitch. Let me know if that is something that you might be interested in doing.

jasonkeene · 2017-11-25T15:47:59Z

Makefile

+	go test ./...
+
+fmt:
+	find . -type f -name '*.go' | xargs gofmt -w


You can use goimports -w . to do the same thing here.

jasonkeene · 2017-11-25T16:04:01Z

data/data.go

-	s.mu.Lock()
-	defer s.mu.Unlock()
-	return s.meetupSchedule
+	return data, err


Line 29 and 31 can be merged.

return ioutil.ReadAll(resp.Body)

vs

data, err = ioutil.ReadAll(resp.Body) return data, err

derp -- good call.

jasonkeene · 2017-11-25T16:22:48Z

data/data.go

-		events := s.poll()
-		s.updateCache(events)
-		time.Sleep(s.pollingInterval)
+func (c MeetupClient) Get(url string) (data []byte, err error) {


This type only has one method and the method does not use the receiver for anything. This causes me to think if this could just be a function. Passing a function vs an interface into FetchEvents would make mocking easier and is actually more performant.

It might be worth thinking about this: https://www.youtube.com/watch?v=o9pEzgHorH0&t=2m10s

Also, the implementation here has nothing to do with the meetup API. It just makes a GET request and returns the body as a []byte. At the very least a better name should be chosen.

I think I'm bringing too much Ruby over to Go, so creating an interface was my first hammer. I think this is what you're suggesting:

// data.go type DataFetcher func(string) ([]byte, error) func FetchData(url string) (data []byte, err error) { resp, err := http.Get(url) if resp != nil { defer resp.Body.Close() } if err != nil { return data, err } data, err = ioutil.ReadAll(resp.Body) return data, err } func (s *Schedule) FetchEvents(fetcher DataFetcher) (err error) { url := fmt.Sprintf("https://api.meetup.com/%s/events?status=upcoming", s.key) data, err := fetcher(url) if err != nil { return err } err = json.Unmarshal(data, &s.Events) if err != nil { return err } sort.SliceStable(s.Events, func(i, j int) bool { return s.Events[i].Time < s.Events[j].Time }) return err }

Which simplifies the testing side as you mentioned:

// data_test.go func TestFetchEventsStoresEvents(t *testing.T) { s := Schedule{key: "Boulder-Gophers"} s.FetchEvents(func(url string) (data []byte, error) { return []byte(`[{"id":"id","name":"Event","time":400}]`), nil }) expected := []Event{Event{ID: "id", Name: "Event", Time: 400}} if !reflect.DeepEqual(s.Events, expected) { t.Fail() } }

I like it.

jasonkeene · 2017-11-25T16:36:46Z

data/data.go

+func (s *Store) Poll() {
+	for {
+		s.mu.Lock()
+		defer s.mu.Unlock()


This has a few problems:

This deadlocks. The mutex is acquired, then it's unlock is deferred. The defer will only run when the function returns or panics. Since this is in an infinite loop this will never occur so only one iteration of this loop will occur.

Using defer inside a loop like this is a common mistake that often leads to a defer leak. This won't leak since only one iteration can occur but that is a bug blocking a bug. See: http://devs.cloudimmunity.com/gotchas-and-common-mistakes-in-go-golang/#anamedeferred_call_exeadeferredfunctioncallexecution

If this did do what you intended, sleeping with a lock acquired should be avoided.

Holding a lock open while doing I/O can cause problems. The code here uses the default http client which can block forever causing a deadlock. Even if that wasn't the case doing I/O with a lock open should be avoided.

The link you provided describes exactly why I implemented it this way -- I think this is another instance of me making changes w/o understanding the underlying design decisions.

It's a very common mistake.

jasonkeene · 2017-11-25T17:00:53Z

data/data.go

+		return err
+	}
+
+	sort.SliceStable(s.Events, func(i, j int) bool {


I'm curious as to why you chose to change to a stable sort. It is not needed here and has a performance cost.

I ran some benchmarks to demonstrate this: https://gist.github.com/jasonkeene/3c4d20275ca5c131e63c5f34f0a03815#file-results-txt

I looked up sort.Slice to learn about it and saw the comment "The sort is not guaranteed to be stable. For a stable sort, use SliceStable.". I reached for SliceStable thinking that in the case where 2 events had the same time, it would preserve the order that they were returned by the API. I didn't know about the performance cost and the possibility of 2 events with the same timestamp seems pretty slim. I'm fine with reverting this.

Stable sort sounds like a good idea then. From what I recall the meetup API does not return very precise timestamps. They are rounded significantly. This increases the chance of timestamps being the same. If that is the case preserving order makes sense.

Good catch!

jasonkeene · 2017-11-26T02:44:24Z

data/data_test.go

+
+	c := NewClient(`[{"id":"id","name":"Event","time":400}]`, nil)
+
+	s.FetchEvents(c)


You should assert that the error returned her is nil.

jasonkeene · 2017-11-26T02:45:20Z

data/data_test.go

+		]
+	`, nil)
+
+	s.FetchEvents(c)


You should assert that the error returned her is nil.

jasonkeene · 2017-11-26T02:47:26Z

server/server.go

-type Store interface {
-	AllEvents() *data.MeetupSchedule
-}
+// type Store interface {


I wouldn't commit code that is commented out. If it isn't needed just delete it. It is in version control so if it is needed in the future it can be brought back.

I think I just missed this when I committed -- I can rebase this out.

jasonkeene · 2017-11-26T02:48:40Z

server/server.go


-func Start(addr string, s Store) error {
+func Start(addr string, s *data.Store) error {


I'm wondering why *data.Store was chosen over Store. Do you need the concrete type?

I didn't see the advantage of an interface in this instance -- what's the best practice here?

Typically, I receive small interfaces. This decouples your code, allows for callers to provide alternative implementations which is good for testing.

That said, passing concrete types is more performant. I only cross that bridge when it is necessary tho.

jasonkeene · 2017-11-26T02:49:15Z

server/server.go

 	return &IndexHandler{
-		store: s,
+		store: *s,


Dereferencing here makes a copy of the Store. Likely this is not what is intended.

Not at all, will need to fix this.

reagent · 2017-11-26T17:07:24Z

Thanks for spending the time doing a thorough review of this PR -- this is exactly what I was hoping for by submitting a larger (but hopefully not overwhelming) change. I'll go back through your comments tomorrow when I have time and respond to each.

reagent · 2017-11-28T05:08:51Z

I'd like to pair on these changes if you have time. I'm not really into the idea of recording it, but I'm open to it if you feel strongly about it. My email is on my profile if you want to coordinate something this week or next. Let me know.

jasonkeene · 2017-11-29T14:40:55Z

Cool, we can pair w/o recording. These shouldn't take long. I'll email you in to coordinate.

reagent added 2 commits November 6, 2017 21:27

Add Makefile for convenience

7159731

jasonkeene requested changes Nov 26, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize data-fetching code / add tests #4

Reorganize data-fetching code / add tests #4

reagent commented Nov 7, 2017

jasonkeene left a comment

jasonkeene Nov 25, 2017

reagent Nov 28, 2017

jasonkeene Nov 25, 2017

reagent Nov 28, 2017

jasonkeene Nov 25, 2017

reagent Nov 28, 2017

jasonkeene Nov 25, 2017

reagent Nov 28, 2017

jasonkeene Nov 29, 2017

jasonkeene Nov 25, 2017

reagent Nov 28, 2017

jasonkeene Nov 29, 2017

jasonkeene Nov 26, 2017

reagent Nov 28, 2017

jasonkeene Nov 26, 2017

reagent Nov 28, 2017

jasonkeene Nov 26, 2017

reagent Nov 28, 2017

jasonkeene Nov 26, 2017

reagent Nov 28, 2017

jasonkeene Nov 29, 2017

jasonkeene Nov 26, 2017

reagent Nov 28, 2017

reagent commented Nov 26, 2017

reagent commented Nov 28, 2017

jasonkeene commented Nov 29, 2017


		c := NewClient(`[{"id":"id","name":"Event","time":400}]`, nil)

		s.FetchEvents(c)


		func Start(addr string, s Store) error {
		func Start(addr string, s *data.Store) error {

Reorganize data-fetching code / add tests #4

Are you sure you want to change the base?

Reorganize data-fetching code / add tests #4

Conversation

reagent commented Nov 7, 2017

jasonkeene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reagent commented Nov 26, 2017

reagent commented Nov 28, 2017

jasonkeene commented Nov 29, 2017