overridding Gopath for custom build

[sethgrid]

I did not want to submit a PR because you may be ideologically opposed to my suggestions. I think it is good to provide good example code. I feel there are some big tweaks that should happen in this example service. This is a brief code review and I did not dig in too deep. There could be more application level issues. I just took note of the things I noticed relatively quickly.

This application is not go get-able and your shell scripts override the GOPATH. While it might "work," I would not consider this idiomatic Go code. Let developers manage their own GOPATH and let other get your packages with go get.

You have an odd import path here: https://github.com/alioygur/gocart/blob/master/src/app/usecases/facebook.go#L11. The lack of a namespace from github or where ever could lead a newer-to-Go developer to think that it is part of the standard lib. I think this is a fragment of you taking over the GOPATH.

Consider changing this to move all of the src/app code to the root directory and creating a bin/ directory where the majority of your of you current root files (the executable ones at least) live. Where I work, we have scripts like bin/build and bin/test that our build systems leverage. These can have build tags and standard command line arguments like making sure tests use the -race flag.

You have no tests :(. I'm interested in the testing path you would take. You could leverage httptest. You could have integration style tests that test integration with the DB (I usually find mocking that to be not worth it).

You have IDatabase as an interface. It is more of a Java paradigm to preface with i. Usually, Go interfaces have a suffix or -er or -or which you use earlier in that same file.

You have Amount float32 - you should use int for money to avoid unexpected rounding issues.

Named returns typically hurt readability: https://github.com/alioygur/gocart/blob/master/src/app/order.go#L102

JWTs should be short lived as you can't invalidate them. If something were to be exploited, you would be allowing that exploited token to do what ever they wish for a whole week: https://github.com/alioygur/gocart/blob/master/src/app/user.go#L51

Your parseDBURL is scary because of the url parsing suggests that someone could accidentally use it in a place that would be exposed to an end user's tampering. I see that it actually comes from an Env Var. Why not just have the Domain Name String as the Env Var? Or have separate DB Env Vars that you read (user, pw, etc)?

Here (https://github.com/alioygur/gocart/blob/master/src/app/infra/cloudinary.go#L64) and in other locations, you are not checking for errors (many foo, _ := ...). The linked one is particularly insidious as someone not familiar with that method would not realize that an error was not checked. I believe there is a package called errCheck that would catch things like that. I think GoMetaLinter runs that be default.

You override the global rand seed here: https://github.com/alioygur/gocart/blob/master/src/app/infra/utils.go#L9. If someone were to import that code, they would lose control of the rand seed which they may be using for deterministic results (which is something I would do, for instance, in a test suit with a -seed flag). Instead, you should instantiate a new rand value with its own seed.

In your gorm interface directory, you have interface{} everywhere, losing any sense of type safety. Looks to be the same in the bolt files. interface{} tells the developer (and the compiler) nothing.

Consider avoiding raw ints for status code responses as you do here: https://github.com/alioygur/gocart/blob/master/src/app/interfaces/handlers/account.go#L35. Consider http.StatusOK.

What does https://github.com/alioygur/gocart/blob/master/src/app/interfaces/handlers/auth.go#L121 get you? Why not just have a token struct? Then you don't have the interface{} return value that tells a developer nothing.

Consider changing decodeR to decodeReq or some similar name expansion. After jumping around looking at the usage and the definition, I see what it does. It would nice if the name was less cryptic with the R.

You check for user existence with userExistByEmail; unfortunately that is a bit naive. If your service gets popular, you will be faced with the use case where a husband and wife (or similar) share an email account but do not want to share an account on your system. Crazy? Yes. Happens? Yes.

Why the label here? https://github.com/alioygur/gocart/blob/master/src/app/usecases/facebook.go#L36

I don't see you closing this Get request: https://github.com/alioygur/gocart/blob/master/src/app/usecases/facebook.go#L46. This will lead to leaked file descriptors and eventually the inability to make connections.

You use an ORM - to each their own. I do not care for them. I particularly don't care for GORM as it takes a code-first perspective, when, in production systems, you usually need to take a database first perspective.

I don't see any consistent logging.

There is no instrumentation / metrics.

I hope to only provide some constructive criticism. Cheers and good luck.

[aliuygur]

I did not want to submit a PR because you may be ideologically opposed to my suggestions. I think it is good to provide good example code. I feel there are some big tweaks that should happen in this example service. This is a brief code review and I did not dig in too deep. There could be more application level issues. I just took note of the things I noticed relatively quickly.

This application is not go get-able and your shell scripts override the GOPATH. While it might "work," I would not consider this idiomatic Go code. Let developers manage their own GOPATH and let other get your packages with go get.

You have an odd import path here: https://github.com/alioygur/gocart/blob/master/src/app/usecases/facebook.go#L11. The lack of a namespace from github or where ever could lead a newer-to-Go developer to think that it is part of the standard lib. I think this is a fragment of you taking over the GOPATH.

Consider changing this to move all of the src/app code to the root directory and creating a bin/ directory where the majority of your of you current root files (the executable ones at least) live. Where I work, we have scripts like bin/build and bin/test that our build systems leverage. These can have build tags and standard command line arguments like making sure tests use the -race flag.

Answer We will discuss these later.

You have no tests :(. I'm interested in the testing path you would take. You could leverage httptest. You could have integration style tests that test integration with the DB (I usually find mocking that to be not worth it).

Answer I will do it.

You have IDatabase as an interface. It is more of a Java paradigm to preface with i. Usually, Go interfaces have a suffix or -er or -or which you use earlier in that same file.

Answer Fixed, 1fde4fe

You have Amount float32 - you should use int for money to avoid unexpected rounding issues.

Answer Then how to use values like these 0.1, 0.5

Named returns typically hurt readability: https://github.com/alioygur/gocart/blob/master/src/app/order.go#L102

Answer Fixed, e3ee39ec27916905634a43a0df6e7df61637caa8

JWTs should be short lived as you can't invalidate them. If something were to be exploited, you would be allowing that exploited token to do what ever they wish for a whole week: https://github.com/alioygur/gocart/blob/master/src/app/user.go#L51

Answer Fixed, a2275fe13ae7f53e75e4cbbad7bbf47f76df25a4

Your parseDBURL is scary because of the url parsing suggests that someone could accidentally use it in a place that would be exposed to an end user's tampering. I see that it actually comes from an Env Var. Why not just have the Domain Name String as the Env Var? Or have separate DB Env Vars that you read (user, pw, etc)?

Answer I have used like this because the SaaS services like Amazon, Heroku pass database credentials like this. do you have any suggestions?

Here (https://github.com/alioygur/gocart/blob/master/src/app/infra/cloudinary.go#L64) and in other locations, you are not checking for errors (many foo, _ := ...). The linked one is particularly insidious as someone not familiar with that method would not realize that an error was not checked. I believe there is a package called errCheck that would catch things like that. I think GoMetaLinter runs that be default.

Answer I will check these.

You override the global rand seed here: https://github.com/alioygur/gocart/blob/master/src/app/infra/utils.go#L9. If someone were to import that code, they would lose control of the rand seed which they may be using for deterministic results (which is something I would do, for instance, in a test suit with a -seed flag). Instead, you should instantiate a new rand value with its own seed.

Answer Do you make a pull request for this ?

In your gorm interface directory, you have interface{} everywhere, losing any sense of type safety. Looks to be the same in the bolt files. interface{} tells the developer (and the compiler) nothing.

Answer I think so.

Consider avoiding raw ints for status code responses as you do here: https://github.com/alioygur/gocart/blob/master/src/app/interfaces/handlers/account.go#L35. Consider http.StatusOK.

Answer Fixed, c16246af92d00bb755959730cd4045b1fa0b3c0f

What does https://github.com/alioygur/gocart/blob/master/src/app/interfaces/handlers/auth.go#L121 get you? Why not just have a token struct? Then you don't have the interface{} return value that tells a developer nothing.

Answer Fixed, 5f8b8eff48a889c44bc9bff632b050626a7cd9b0

Consider changing decodeR to decodeReq or some similar name expansion. After jumping around looking at the usage and the definition, I see what it does. It would nice if the name was less cryptic with the R.

Answer Fixed, f7261d6fcec62d957d5095fb38d23d17efce36f6

You check for user existence with userExistByEmail; unfortunately that is a bit naive. If your service gets popular, you will be faced with the use case where a husband and wife (or similar) share an email account but do not want to share an account on your system. Crazy? Yes. Happens? Yes.

Answer Sorry I didn't understand what did you mean. do you say that e-mail addresses shouldn't be unique?

Why the label here? https://github.com/alioygur/gocart/blob/master/src/app/usecases/facebook.go#L36

Answer Fixed, 561500fca42d78e0e6917fa6af8fe7b59b71dd3a

I don't see you closing this Get request: https://github.com/alioygur/gocart/blob/master/src/app/usecases/facebook.go#L46. This will lead to leaked file descriptors and eventually the inability to make connections.

Answer Fixed, 7805c9bb3dd172dcac1a006ccc721aea727ae840

You use an ORM - to each their own. I do not care for them. I particularly don't care for GORM as it takes a code-first perspective, when, in production systems, you usually need to take a database first perspective.

Answer I think so. It should be replaced another one

I don't see any consistent logging.

Answer I will do.

There is no instrumentation / metrics. Answer

I will do.

I hope to only provide some constructive criticism. Cheers and good luck.

Thank you so much for your feedback

[sethgrid]

Then how to use values like these 0.1, 0.5

You can store the value as an int (in USD, you may want to use cents or 10ths of a cent). In this case you would store cents := 10 and/or cents := 50. See https://www.noelherrick.com/blog/always-use-decimal-for-money as an example of why floats can be bad.

I have used like this because the SaaS services like Amazon, Heroku pass database credentials like this. do you have any suggestions?

If it works, keep doing it. I've not built systems outside our company's data center so I have no experience for what is best / easiest for Amazon nor Heroku. :)

Sorry I didn't understand what did you mean. do you say that e-mail addresses shouldn't be unique?

For small services, you can assume unique email addresses. When you become larger and have either business customers or a married couple as users, they may have only one email address but may want multiple accounts; in this case, you will have to allow duplicate email addresses but unique user names. It is an edge case. We did not get that edge case until we had many thousands of customers, and only a few customers needed this support.

Do you make a pull request for this ?

https://github.com/alioygur/gocart/pull/2

Cheers!