-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement users controller #9
Conversation
In this change I created a controller to handle the /users/{userId} endpoint, where userId could be a numeric ID or a user name. I changed the naming convention for parameters from snake_case (user_id) to camelCase (userId) because our client is written in Javascript and it uses camelCase. For now our database is just a file with some users in JSON format. This change is a needed to implement the "Has user X solved problem Y" because for that we need users first.
|
||
problemsController := problems.NewController(deps.ProblemsDataProvider) | ||
r.HandleFunc("/v1/problems/sets", problemsController.SetIndex) | ||
r.GET("/v1/problems/sets", problemsController.SetIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it a bit Confused to mix GET and SET verbs here? I understand that SET has a different meaning here but is still confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should probably change SetIndex to something else, but I would do that in a future change as this was not added in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, lets keep this in mind for future changes.
"os" | ||
) | ||
|
||
var ErrNotFound = errors.New("User not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a good idea to have constant Errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The language does not allow to have constants of pointers, and errors.New()
returns a pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation.
} | ||
|
||
for _, user := range users { | ||
if user.UserName == userName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please describe what is your criteria to use variable, varName or VarName I am noticing the threes styles in this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course.
Variables: simple camelCase. In this snippet, user
is a User
, userName
is a user name.
Fields (in structs): exported fields (visible from outside the package) start with an uppercase letter, otherwise it will be visible only within this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, we can add this to README just to be clear about the naming convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a lot, that's part of the language and there's already a README for that: https://github.com/golang/go/wiki/CodeReviewComments.
We could add that link to the README though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to complain about this 😏 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me.
In this change I created a controller to handle the /users/{userId}
endpoint, where userId could be a numeric ID or a user name.
I changed the naming convention for parameters from snake_case (user_id)
to camelCase (userId) because our client is written in Javascript and it
uses camelCase.
For now our database is just a file with some users in JSON format.
This change is a needed to implement the "Has user X solved problem Y"
because for that we need users first.