Code review with changes#7
Open
akshith271 wants to merge 1 commit into
Open
Conversation
akshith271
commented
Jan 18, 2024
Comment on lines
-12
to
-13
| name := r.FormValue("name") | ||
| email := r.FormValue("email") |
| func setupJsonApi() { | ||
| http.HandleFunc("/createUser", func(w http.ResponseWriter, r *http.Request) { | ||
| // create mysql connection | ||
| conn := createConnection() |
Author
There was a problem hiding this comment.
Creating a common createConnection() is preferable
| email := r.FormValue("email") | ||
| query := "INSERT INTO users (name, email) VALUES (" + name + ", " + email + ")" | ||
| result, err := conn.Exec(query) | ||
| fmt.Println("result ", result, " err ", err.Error()) |
|
|
||
| func main() { | ||
| setupJsonApi() | ||
| http.ListenAndServe(":80", nil) |
Author
There was a problem hiding this comment.
there is no error handling for http.ListenAndServe
|
|
||
| // createConnection creates a connection to mysql database | ||
| func createConnection() *sql.DB { | ||
| db, err := sql.Open("mysql", "root:password@tcp(127.0.0.1:3306)/test") |
Author
There was a problem hiding this comment.
Using constants and encrypted environment variables is more secure.
| email := r.FormValue("email") | ||
| query := "Update users set name=" + name + ", email=" + email + " where id=" + r.FormValue("id") | ||
| result, err := conn.Exec(query) | ||
| fmt.Println("result ", result, " err ", err.Error()) |
Author
There was a problem hiding this comment.
returning HTTP errors makes it easy to understand and debug.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here are some issues I found:
1. SQL Injection Vulnerability:
❌The code directly concatenates user input into the SQL query string. That is not ideal.
✔Use parameterized queries to bind values securely
2. Error Handling:
❌The code does not properly handle potential errors during operations.
✔Add error handling to handle errors effectively, such as returning an error response to the client.
3.Code Duplication:
❌The code for creating a database connection is duplicated in both the createUser and updateUser handlers.
✔ It's better to create a reusable function for establishing a database connection.
4. Global Database Connections:
❌It's generally not recommended to use a global database connection.
✔Passing the database connection as a parameter to functions or using a struct to manage the database connection within a specific scope is better.
5. Lack of defer statements :
❌It is better to close the database connections after the function exits. We have to use defer keyword provided by go
✔We have to use defer keyword provided by go.
Here are some suggestions:
1. Use logging specific packages:
Instead of fmt, log is recommended because we can include additional information like timestamp, log level, source file information. fmt is more general purpose formatting package.
2. Use HTTP status codes:
Using HTTP status codes helps us in providing more meaningful error messages and helps us in debugging.
3. Use constants over direct values.
4. Add validations to the user inputs.
5. Use environment variables.
6. Be specific with the error messages.
Authored By :
Akshith Bharadwaj
[email protected]
+91-9347620298