Couple of changes to exercise 2

[benprew]

Hey,

Thanks for putting these exercise together, they're fun!

I wanted to expand on the exercise a little bit by adding some more detail to the problem and also adding an artificial slowdown to the mock database call to better simulate a call to a database. It also highlights the effects of different solutions (ie locking the entire function vs locking just the map/list access)

Finally I was noticed in the comments that it mentioned that it was an LRU cache, but it's implemented as a FIFO cache, so I included some changes to implement an LRU cache.

Let me know if there's any changes you'd like me to make, I'm happy to make any fixes.

Thanks!

[loong]

Hey Ben,

Thank you so much for the PR! Looks great so far. Will give it a closer view over the weekend! 🙏

Greetings from Singapore, Long

[benprew]

Long,

No problem, thanks for providing the exercises!

On Fri, Aug 21, 2020 at 12:31 AM Long Hoang notifications@github.com wrote:

Hey Ben,

Thank you so much for the PR! Looks great so far. Will give it a closer view over the weekend! 🙏

Greetings from Singapore, Long

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/loong/go-concurrency-exercises/pull/12#issuecomment-678091452, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAJHV5LIW367BYSVPUFD5DSBYPGJANCNFSM4P7D3L2Q .

[benprew]

Great, thanks for the feedback, I'll make those changes and update the PR.

On Mon, Aug 24, 2020 at 6:07 AM Long Hoang notifications@github.com wrote:

@loong requested changes on this pull request.

Hey @benprew https://github.com/benprew!

Thank you so much for the PR! Especially catching my mistake with the LRU. I hope no one used my implementation as a template for their own LRU cache implementation 😅.

I ran golint and got some errors. Better to solve them or my friend @ironzeb will give us a bad GoReportCard https://goreportcard.com/report/github.com/loong/go-concurrency-exercises !

~/g/s/g/l/g/2-race-in-cache ❯❯❯ golint .

main.go:22:6: exported type Page should have comment or be unexported

main.go:47:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

So, I only have a these small nitpicks. Once those are resolved, we can merge!

Greetings from tropical Singapore!

In 2-race-in-cache/check_test.go https://github.com/loong/go-concurrency-exercises/pull/12#discussion_r475580240 :

@@ -20,3 +23,24 @@ func TestMain(t *testing.T) {

  t.Errorf("Incorrect pages size %v", pagesLen)

}

}

+

+func TestLRU(t *testing.T) {

• loader := Loader{

• DB: GetMockDB(),

• }

• cache := New(&loader)

• for i := 0; i < 100; i++ {

• cache.Get("Test " + strconv.Itoa(i))

⬇️ Suggested change

• cache.Get("Test " + strconv.Itoa(i))

• cache.Get("Test" + strconv.Itoa(i))

I'm really nitpicking here, sorry 🙇 ! So, in the mock server, there isn't a space after Test:

https://github.com/loong/go-concurrency-exercises/blob/master/2-race-in-cache/mockserver.go#L29

It's all correct though 👍

In 2-race-in-cache/check_test.go https://github.com/loong/go-concurrency-exercises/pull/12#discussion_r475580670 :

@@ -20,3 +23,24 @@ func TestMain(t *testing.T) {

  t.Errorf("Incorrect pages size %v", pagesLen)

}

}

+

+func TestLRU(t *testing.T) {

Test case! 🤩

In 2-race-in-cache/main.go https://github.com/loong/go-concurrency-exercises/pull/12#discussion_r475584044 :

@@ -19,9 +19,14 @@ type KeyStoreCacheLoader interface {

Load(string) string

}

+type Page struct {

Page is an exported value but has no comment!

From linter: main.go:22:6: exported type Page should have comment or be unexported

In 2-race-in-cache/main.go https://github.com/loong/go-concurrency-exercises/pull/12#discussion_r475584732 :

• if e, ok := k.cache[key]; ok {

• k.pages.MoveToFront(e)

• return e.Value.(Page).Value

• } else {

• // Miss - load from database and save it in cache

• page := Page{key, k.load(key)}

  // if cache is full remove the least used item

• if len(k.cache) > CacheSize {

• delete(k.cache, k.pages.Back().Value.(string))

• k.pages.Remove(k.pages.Back())

• if len(k.cache) >= CacheSize {

• end := k.pages.Back()

• // remove from map

• delete(k.cache, end.Value.(Page).Key)

• // remove from list

• k.pages.Remove(end)

  }

• k.pages.PushFront(page)

• k.cache[key] = k.pages.Front()

• return page.Value

The if block ends with a return, so else block is not needed.

From linter: main.go:47:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/loong/go-concurrency-exercises/pull/12#pullrequestreview-473437962, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAJHV22N6OZZ274NZC7K5TSCJQ2LANCNFSM4P7D3L2Q .

[benprew]

Hey @loong thanks for the feedback, I think my changes address your feedback and the golint warnings. Let me know if I missed anything.

How's the weather in Singapore? I'm in Oregon, USA, where it's currently 70F (21C) and sunny, pretty perfect out right now.

[loong]

Well, it's an eternal summer here with at least 30°C (86°F) during the day. I grew up in Germany, so this is hot for me (especially with the humidity!). 😅

Went for a hike today!

EDIT: The picture above is totally necessary for this PR

[benprew]

Thanks @loong, I'll send another PR if I think of some exercises 👍 . Also, I don't think I can merge this PR unless you grant me write access to your repo.

PS. that's a great picture, it's so beautiful and serene, more PRs could use pictures like that :)