-
Notifications
You must be signed in to change notification settings - Fork 0
Done testing exercise and http exercise by Nacchan #2
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds solutions for the HTTP client exercise and testing exercise by implementing a basic HTTP request flow and range validation functionality.
- Implements a range-checking function with accompanying tests.
- Adds an HTTP client that fetches user and account data from an external API.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
exercise_answers/nacchan/validator/validator.go | Implements a simple range-checking function with clear comments. |
exercise_answers/nacchan/validator/validator_test.go | Provides comprehensive tests for the IsInRange function. |
exercise_answers/nacchan/http_exercise.go | Implements an HTTP client to fetch and display user and account data. |
) | ||
|
||
type UserAttributes struct { | ||
Id int `json:"id"` |
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.
nitpick about naming; in Go we need to use consistent case (all small, or all capitalized) for initialisms and acronyms. For example, ID
, AccountIDs
, id
(for unexported variable), userID
etc.
You can see Go Wiki and the Google Style Guide for more examples!
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.
Google style guide looks very easy to understand! Thanks👏
func main() { | ||
var userId int | ||
reader := bufio.NewReader(os.Stdin) | ||
for { |
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.
nice loop!
There are many ways to read input in Go, including bufio
, fmt.Scanf
, and others. I am not sure which one is the most performant or most common, but your solution seems robust!
resp, err := http.Get(URL + strconv.Itoa(id)) | ||
|
||
if err != nil { | ||
return "", fmt.Errorf("error in making request: %v", err) |
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 error message looks great! It follows the error strings suggestion of using not-capitalized strings, which is correct. There is only one thing that can be improved here, which is using %w
instead of %v
for wrapping the errors.
The %w
verb creates "wrapped" (nested) errors which can be unwrapped using the errors
package. You can check the errors package documentation for more info!
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 see, by wrapping you can use error.Is
later to check it holds specific error or not👀 I need to understand about go error handling more, thanks for letting me know!
return nil, fmt.Errorf("error in parsing json: %v", err) | ||
} | ||
|
||
var accounts []AccountAttributes = []AccountAttributes{} |
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.
There are many ways to initialize slices in Go, such as:
var accounts []AccountAttributes = []AccountAttributes{}
- creates a slice with 0 length/capacity, appending elements will resize the slice
- this approach is generally not recommended in Go (instead, use the next approach)
var accounts []AccountAttributes
- creates a
nil
slice with 0 length/capacity, appending elements will resize the slice - this approach is recommended if you do not know (or cannot estimate) the final size of the slice
- creates a
accounts := make([]AccountAttributes, len(res))
- creates a slice with 0 length, but preconfigured capacity, appending elements will only resize the slice if the capacity is not enough
- this approach is recommended when you already know (or can estimate) the final size of the slice
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.
printAccounts(accounts) | ||
} | ||
|
||
func getUser(id int) (string, error) { |
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.
Great solution!
var res UserResponse | ||
err = json.Unmarshal(body, &res) | ||
|
||
if err != nil { |
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.
This comes down to personal preferences, so other people may have different suggestions on this. But a more idiomatic way of retrieving an error and checking if it's nil, is like this:
if err = json.Unmarshal(body, &res); err != nil {
return "", fmt.Errorf("error in parsing json: %v", err)
}
The main benefit of this approach is that the scope of the err
variable is limited to the if
statement. This style is used whenever the function that you want to check (in this case, json.Unmarshal()
) only returns an error object, and you want to immediately check the error.
Note: your solution is also perfectly valid, since the err
variable has already been declared in a previous statement. The code that I recommended does not actually limit the scope of the outer err
variable, but it's still considered more "go idiomatic", aka it's more common.
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.
Assigning variables at the beginning of an if statement certainly gives it a go-like style!(I never saw this style in other language🧐) While I understand that limiting the variable scope isn't always essential, I agree it’s considered good practice and I'll keep that in mind!
return accounts, nil | ||
} | ||
|
||
func printAccounts(accounts []AccountAttributes) { |
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.
Nice function!
"testing" | ||
) | ||
|
||
func TestIsInRange(t *testing.T) { |
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.
Great solution!!
t.Run(tc.name, func(t *testing.T) { | ||
result := IsInRange(tc.num, tc.min, tc.max) | ||
if result != tc.expected { | ||
t.Errorf("expected %v, got %v", tc.expected, result) |
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 error message is clear, and shows what went wrong in the test!
But some small optional adjustments are:
- Instead of
expected x, got y
, Go usually follows the opposite ordergot x, want y
. This is also explained briefly in the "Useful Test Failures" section of the Go Wiki. - Mentioning the function name is also a common pattern, so in this case the error would be:
t.Errorf("IsInRange() = %v, want %v", result, tt.expected)
Note: in IAA we will probably use the assert package of the testify library, so the error messages will automatically be handled by the library.
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.
FYI this is the auto-generated test from IntelliJ, that follows the general Go conventions:
func TestIsInRange(t *testing.T) {
type args struct {
num int
min int
max int
}
tests := []struct {
name string
args args
want bool
}{
// TODO: Add test cases.
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := IsInRange(tt.args.num, tt.args.min, tt.args.max); got != tt.want {
t.Errorf("IsInRange() = %v, want %v", got, tt.want)
}
})
}
}
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 see—it's a Go convention to include the test's objective in the function name and clearly distinguish between got and want. Thanks for pointing out this helpful reference!
func TestIsInRange(t *testing.T) { | ||
tt := []struct { | ||
name string | ||
num, min, max int |
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.
Your approach is common to separate the input and output parameters of the test! In this case, num, min, max
are all input parameters, so they are placed in the same line 👏
Another way to separate them, if we have a lot of input parameters (maybe 4+), is using an extra struct. Check the IntelliJ-generated code that I pasted in the below comment.
This PR adds answers for the http client exercise and the testing exercise.
Screenshots