Skip to content

remove files so I can add comments#1

Open
jimmyjames85 wants to merge 1 commit into
blics18:masterfrom
jimmyjames85:branch-for-comments
Open

remove files so I can add comments#1
jimmyjames85 wants to merge 1 commit into
blics18:masterfrom
jimmyjames85:branch-for-comments

Conversation

@jimmyjames85

@jimmyjames85 jimmyjames85 commented Oct 24, 2017

Copy link
Copy Markdown

A pull request (PR) is a git branch with code changes. In github you can only add comments to pull requests. So I forked your repo, created the branch jimmyjames85:branch-for-comments and deleted all the files. This way I could add comments. Since this was the first commit, committing to master branch was the right thing to do. I think for future commits, we should work in a separate branch next time, that way we can add comments directly to your PR. Then when we all agree with the code, we merge the separate branch into master.

Git can be pretty confusing and this helped me out a lot: https://learngitbranching.js.org/

Comment thread README.md
@@ -1,15 +0,0 @@
# SendGrid

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

love love love that you included a README.md

At SendGrid, our readmes often include steps on how to spin up the service, execute tests, setup the environment, etc... These are really nice to have.

Comment thread README.md

#### Files:

- client.go: Generates random user ID and emails and sends to localhost:8081/retrieve

@jimmyjames85 jimmyjames85 Oct 24, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

These two files are in the same directory and the same package main. This can be confusing. For example, while I was able to go run each of them in the terminal, my code inspection complains that main is defined twice. Consider reorganizing by creating two separate folders "server" and "client". Also you can check out https://talks.golang.org/2014/organizeio.slide

Comment thread client.go
userEmails:= MakeRandomEmails(numEmails)

d := User{userID, userEmails}
p := Payload{d}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm curious why Payload is just a wrapper around User. Since it doesn't provide additional information maybe it's not needed? For example you could just send:

{
    "UserID": 5,
    "Emails": [
      "PIDrMFzBov@cLINF.RIJ",
      "lEhUOMNPvZ@oOTSx.TZL",
      "xwbvoqMnec@YrFEB.XTi",
      "FATPMrcKCc@ZXpAk.LEs",
      "rojvvxzmvs@OmXUG.gda",
      "YTbcczirwe@ssrKz.eQi",
      "cuwRQXhWvV@VGesP.hLv",
      "bqpYGiWRXQ@fIGtC.iHS",
      "mCdjUEEDBm@gINuk.Rta",
      "ZXNXPckgmb@KZPUJ.dJU"
    ]
  }

Maybe you have plans to add on to the payload in the future?

{
   "UserData": .... , 
   "RequestTimeout": ... , 
   "BloomFilterMetaData": ...  ,
}

Comment thread client.go
d := User{userID, userEmails}
p := Payload{d}

userJSON, err := json.MarshalIndent(p, "", " ")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

What do you want to do if there is an error? Currently we are not handling them

   if err != nil {
          // handle error here
   }

Comment thread client.go
p := Payload{d}

userJSON, err := json.MarshalIndent(p, "", " ")
req, err := http.NewRequest("GET", "http://localhost:8081/retrieve", bytes.NewBuffer(userJSON))

@jimmyjames85 jimmyjames85 Oct 24, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ditto

Comment thread client.go
client := &http.Client{}
resp, err := client.Do(req)

if err != nil {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

👍

Comment thread client.go
}

// Print the response being sent to the client
// body, err := ioutil.ReadAll(resp.Body)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We usually remove commented code from production ready repos. But also I think this might be usefull (shrug), especially for debugging. Maybe a setting like if Debug { printStuff }

Comment thread server.go
if err != nil {
panic(err)
}

@jimmyjames85 jimmyjames85 Oct 24, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see defer r.Body.Close() is the last line. It's a good thing to always close the body, however "A defer statement defers the execution of a function until the surrounding function returns." So as the last statement the defer keyword is really not needed. If you moved it up to line 46 it would be better utilized. It's generally a good idea from the standpoint of organizing code, because the Close() statement is next to the ReadAll() statement. See: https://gobyexample.com/defer

Comment thread server.go

err = json.Unmarshal(body, &p)
if err != nil {
panic(err)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

When I tried to hit the endpoint from the browser, this panic(err) line got executed and the browser said

This page isn’t working

localhost didn’t send any data.
ERR_EMPTY_RESPONSE

Fortunately go's http.ListenAndServe caught the panic and your server recovered. But a nicer way to handle the error is to write a response code back to the client e.g.

if err != nil {
    w.WriteHeader(http.StatusBadRequest)
    w.Write([]byte("Unable to parse json body"))
    return
}

w.WriteHeader(http.StatusAccepted)
w.Write([]byte("success"))

Comment thread client.go
return email_list
}

func MakeRandomUserID(n int, offset int) int {

@jimmyjames85 jimmyjames85 Oct 24, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Where is this function being used?

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.

1 participant