Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
run:
go run main.go

test:
go test ./...

fmt:
find . -type f -name '*.go' | xargs gofmt -w
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆒 TIL


166 changes: 78 additions & 88 deletions data/data.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,78 +3,34 @@ package data
import (
"encoding/json"
"fmt"
"io/ioutil"
"log"
"net/http"
"sort"
"sync"
"time"
)

const apiTemplate = "https://api.meetup.com/%s/events?status=upcoming"

var (
meetupNames = []string{
"Boulder-Gophers",
"Denver-Go-Language-User-Group",
"Denver-Go-Programming-Language-Meetup",
}
)

// Store contains data for the site.
type Store struct {
pollingInterval time.Duration

mu sync.Mutex
meetupSchedule *MeetupSchedule
type Client interface {
Get(string) ([]byte, error)
}

// NewStore creates a new store initialized with a polling interval.
func NewStore(i time.Duration) *Store {
return &Store{
pollingInterval: i,
}
}
type MeetupClient struct{}
Copy link
Member

Choose a reason for hiding this comment

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

MeetupClient is not covered by tests. Testing this might be low value but the cost of testing it is fairly low as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll skip this altogether based on your other comments.


// Poll runs forever, polling the meetup API for event data and updating the
// internal cache.
func (s *Store) Poll() {
for {
events := s.poll()
s.updateCache(events)
time.Sleep(s.pollingInterval)
func (c MeetupClient) Get(url string) (data []byte, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

resp, err := http.Get(url)

if err != nil {
return data, err
}
}

func (s *Store) updateCache(schedule *MeetupSchedule) {
s.mu.Lock()
defer s.mu.Unlock()
s.meetupSchedule = schedule
}
defer resp.Body.Close()

func (s *Store) poll() *MeetupSchedule {
schedule := NewMeetupSchedule()
for _, meetup := range meetupNames {
eds, err := events(meetup)
if err != nil {
log.Printf("error fetching events for %s: %s", meetup, err)
continue
}
sort.Slice(eds, func(i, j int) bool {
return eds[i].Time < eds[j].Time
})
schedule.Add(meetup, eds)
}
return schedule
}
data, err = ioutil.ReadAll(resp.Body)

// AllEvents returns the current meetup events in CO.
func (s *Store) AllEvents() *MeetupSchedule {
s.mu.Lock()
defer s.mu.Unlock()
return s.meetupSchedule
return data, err
Copy link
Member

Choose a reason for hiding this comment

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

Line 29 and 31 can be merged.

return ioutil.ReadAll(resp.Body)

vs

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

return data, err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

derp -- good call.

}

// Event contains information about a meetup event.
type Event struct {
ID string `json:"id"`
Name string `json:"name"`
Expand All @@ -86,52 +42,86 @@ func (e Event) HumanTime() string {
return time.Unix(e.Time/1000, 0).Format(time.RFC1123)
}

func NewMeetupSchedule() *MeetupSchedule {
return &MeetupSchedule{
events: make(map[string][]Event),
}
}
type Schedule struct {
key string

type MeetupSchedule struct {
events map[string][]Event
Label string
Events []Event
}

func (m *MeetupSchedule) Add(name string, events []Event) {
m.events[name] = events
}
type Schedules []*Schedule

func (s *Schedule) FetchEvents(client Client) (err error) {
url := fmt.Sprintf("https://api.meetup.com/%s/events?status=upcoming", s.key)

data, err := client.Get(url)

func (m *MeetupSchedule) BoulderEvents() []Event {
return nextThree(m.events["Boulder-Gophers"])
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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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!

return s.Events[i].Time < s.Events[j].Time
})

return err
}

func (m *MeetupSchedule) DenverEvents() []Event {
return nextThree(m.events["Denver-Go-Language-User-Group"])
func (s *Schedule) Next(count int) []Event {
Copy link
Member

Choose a reason for hiding this comment

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

Next and FetchEvent are called from separate goroutines and their access to s.Events is not synchronized. this will cause data races.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I think a solution here would be to not modify s.Events inside of FetchEvents and move more closely to the original implementation. I really need to check out those screencasts you did to get a better understanding of the context when looking at this code.

This may be something best suited for reviewing during a pairing session.

if len(s.Events) > count {
return s.Events[0:count]
Copy link
Member

Choose a reason for hiding this comment

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

This can be return s.Events[:count]. The 0 is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😎

}

return s.Events
}

func (m *MeetupSchedule) DTCEvents() []Event {
return nextThree(m.events["Denver-Go-Programming-Language-Meetup"])
// Store contains data for the site.
type Store struct {
pollingInterval time.Duration
mu sync.Mutex

Schedules Schedules
}

func nextThree(events []Event) []Event {
if len(events) < 3 {
return events
// NewStore creates a new store initialized with a polling interval.
func NewStore(i time.Duration) *Store {
return &Store{
pollingInterval: i,
Schedules: Schedules{
&Schedule{key: "Boulder-Gophers", Label: "Boulder"},
&Schedule{key: "Denver-Go-Language-User-Group", Label: "Denver"},
&Schedule{key: "Denver-Go-Programming-Language-Meetup", Label: "Denver Tech Center"},
},
}

return events[0:3]
}

func events(name string) ([]Event, error) {
resp, err := http.Get(fmt.Sprintf(apiTemplate, name))
if err != nil {
log.Fatal(err)
// Poll runs forever, polling the meetup API for event data and updating the
// internal cache.
func (s *Store) Poll() {
Copy link
Member

Choose a reason for hiding this comment

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

Poll is not covered in tests. This would have exposed the deadlock below.

for {
s.mu.Lock()
defer s.mu.Unlock()
Copy link
Member

Choose a reason for hiding this comment

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

This has a few problems:

  1. 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.
  2. 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
  3. If this did do what you intended, sleeping with a lock acquired should be avoided.
  4. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

It's a very common mistake.

s.refresh()

time.Sleep(s.pollingInterval)
}
defer resp.Body.Close()
}

decoder := json.NewDecoder(resp.Body)
var data []Event
err = decoder.Decode(&data)
if err != nil {
return nil, err
func (s *Store) refresh() {
client := MeetupClient{}

for _, s := range s.Schedules {
err := s.FetchEvents(client)

if err != nil {
log.Printf("error fetching events for %s: %s", s.key, err)
continue
}
}
return data, nil
}
132 changes: 132 additions & 0 deletions data/data_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
package data
Copy link
Member

Choose a reason for hiding this comment

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

Using package data_test prevents collisions with test fixtures. For instance if NewClient was defined here as well as in data.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense -- I have done this when using Ginkgo, but I wasn't sure if it was a general practice.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer it. Using package data_test makes it to where you have to test only what is exported by the package. I only use something like package data when I explicitly want to test a function/type that is not exported by the package. In that situation I often consider just creating an internal package. See: https://docs.google.com/document/d/1e8kOo3r51b2BWtTs_1uADIA5djfXhPT36s6eHVRIvaU/edit


import (
"errors"
"reflect"
"testing"
)

type TestClient struct {
Copy link
Member

Choose a reason for hiding this comment

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

Fixture types and functions should be lowercase. They won't be exported if they are uppercase but keeping them lowercase reduces confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coupled with package data in this file, that was an unintended side-effect.

*MeetupClient
Copy link
Member

Choose a reason for hiding this comment

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

Embedding *MeetupClient here is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must have been a vestige of a previous approach, this is moot now anyway.


data []byte
err error
}

func NewClient(data string, err error) TestClient {
return TestClient{
data: []byte(data),
err: err,
}
}

func (c TestClient) Get(key string) ([]byte, error) {
if c.err != nil {
return []byte{}, c.err
}

return c.data, c.err
}

func TestScheduleHasNoEvents(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

This test isn't useful, it covers no code in data.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can delete.

s := Schedule{}
if len(s.Events) != 0 {
t.Fail()
}
}

func TestFetchEventsStoresEvents(t *testing.T) {
s := Schedule{key: "Boulder-Gophers"}

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

s.FetchEvents(c)
Copy link
Member

Choose a reason for hiding this comment

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

You should assert that the error returned her is nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


expected := []Event{Event{ID: "id", Name: "Event", Time: 400}}

if !reflect.DeepEqual(s.Events, expected) {
t.Fail()
}
}

func TestFetchEventsReturnsErrorWhenRequestFails(t *testing.T) {
s := Schedule{key: "Boulder-Gophers"}
c := NewClient(``, errors.New("Failed to connect"))

err := s.FetchEvents(c)

if err == nil {
t.Fail()
}
}

func TestFetchEventsReturnsErrorWhenUnmarshalFails(t *testing.T) {
s := Schedule{key: "Boulder-Gophers"}
c := NewClient(`<>`, nil)

err := s.FetchEvents(c)

if err == nil {
t.Fail()
}
}

func TestFetchEventsReturnsEventsInOrderOfTime(t *testing.T) {
s := Schedule{key: "Boulder-Gophers"}
c := NewClient(`
[
{"id":"two","name":"Two","time":2},
{"id":"one","name":"One","time":1}
]
`, nil)

s.FetchEvents(c)
Copy link
Member

Choose a reason for hiding this comment

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

You should assert that the error returned her is nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


expected := []Event{
Event{ID: "one", Name: "One", Time: 1},
Event{ID: "two", Name: "Two", Time: 2},
}

if !reflect.DeepEqual(s.Events, expected) {
t.Fail()
}
}

func TestNextReturnsSubsetOfEvents(t *testing.T) {
s := Schedule{Events: []Event{
Event{ID: "one", Name: "One", Time: 1},
Event{ID: "two", Name: "Two", Time: 2},
Event{ID: "three", Name: "Three", Time: 3},
}}

expected := []Event{
Event{ID: "one", Name: "One", Time: 1},
Event{ID: "two", Name: "Two", Time: 2},
}

if !reflect.DeepEqual(s.Next(2), expected) {
t.Fail()
}
}

func TestNextReturnsAllWhenLimitGreaterThanLen(t *testing.T) {
s := Schedule{Events: []Event{
Event{ID: "one", Name: "One", Time: 1},
}}

expected := []Event{
Event{ID: "one", Name: "One", Time: 1},
}

if !reflect.DeepEqual(s.Next(2), expected) {
t.Fail()
}
}

func TestHumanTimeReturnsTimeInProperFormat(t *testing.T) {
e := Event{Time: 1533121600000}

if e.HumanTime() != "Wed, 01 Aug 2018 05:06:40 MDT" {
t.Fail()
}
}
17 changes: 9 additions & 8 deletions server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import (
"github.com/milehighgophers/website/ui"
)

type Store interface {
AllEvents() *data.MeetupSchedule
}
// type Store interface {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// AllEvents() *data.MeetupSchedule
// }

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

log.Printf("listening on %s", addr)

mux := http.NewServeMux()
Expand All @@ -22,17 +22,18 @@ func Start(addr string, s Store) error {
}

type IndexHandler struct {
store Store
store data.Store
}

func NewIndexHandler(s Store) *IndexHandler {
func NewIndexHandler(s *data.Store) *IndexHandler {
return &IndexHandler{
store: s,
store: *s,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at all, will need to fix this.

}
}

func (h *IndexHandler) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
html := ui.Render(h.store.AllEvents())
html := ui.Render(h.store.Schedules)

_, err := rw.Write(html)
if err != nil {
log.Print("error occured with /:", err)
Expand Down
Loading