Problem
I’ve written the following function which reads an authorization header, compares it and returns whether it is valid or not.
The function works correctly:
func Auth(req *http.Request) bool{
sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 2)
if sHeader[0] != "" {
fmt.Printf("sHeader: ", sHeader[1])
devEnv := os.Getenv("authorization")
if (devEnv == sHeader[1]) {
fmt.Println("valid")
return true
} else {
fmt.Println("Not valid: " + sHeader[1])
return false
}
} else {
log.Fatal("Missing Authorization Header")
return false
}
}
What can I improve?
Solution
Here are my notes that I would bring to the code review.
package main
import (
"fmt"
"log"
"net/http"
"os"
"strings"
)
func AuthCodeReview(req *http.Request) bool { // why bool? why not error
sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 3) // 3 for fields after 0 & 1?
if len(sHeader) == 0 || sHeader[0] == "" { // check index in range
log.Print("Missing Authorization Header") // never Fatal!
return false
}
if len(sHeader) > 1 { // why 1? why ignore 0? check index in range
fmt.Printf("sHeader: ", sHeader[1]) // debug?
devEnv := os.Getenv("authorization") // LookupEnv?
if devEnv == sHeader[1] {
fmt.Println("valid") // debug?
return true
}
}
fmt.Println("Not valid: " + sHeader[1]) // debug?
return false
}
func Auth(req *http.Request) bool {
sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 2)
if sHeader[0] != "" {
fmt.Printf("sHeader: ", sHeader[1])
devEnv := os.Getenv("authorization")
if devEnv == sHeader[1] {
fmt.Println("valid")
return true
} else {
fmt.Println("Not valid: " + sHeader[1])
return false
}
} else {
log.Fatal("Missing Authorization Header")
return false
}
}
func main() {}
Playground: https://play.golang.org/p/_MwBV80vWx
log.Fatal
is equivalent tolog.Print()
followed by a call to
os.Exit(1)
.
os.Exit
causes the current program to exit with the given status code.
The program terminates immediately; deferred functions are not run.
Your program is running 100,000 client connections using goroutines. One connection has a problem, so you crash the program. We have 100,000 very upset customers. Handle errors and fail gracefully; return errors; don’t panic
or Exit
.
See Errors.
fmt.Println("valid") // debug?
Your sysout
is a massive clutter of messages. That may make sense for debugging, but it doesn’t make sense for production. What is the purpose of this and similar statements.
fmt.Println("Not valid: " + sHeader[1]) // debug?
If this message is not for debugging, it should be logged (log
not fmt
).
strings.SplitN
The count determines the number of substrings to
return: n > 0: at most n substrings; the last substring will be the
unsplit remainder.
// 3 for fields after 0 & 1?
sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 3)
If you are expecting two fields SplitN
(2) will return the first field in index 0
and any remainder in index 1
. SplitN
(3) will return the first field in index 0
, the second field in index 1
and any remainder in index 2
. Having a remainder may or may not be valid.
Playground: https://play.golang.org/p/IQA8iqIdN5
sHeader[1] // why 1? why ignore 0?
You don’t say what the format of req.Header.Get("authorization")
is. What does sHeader[1]
represent. Add a comment to define sHeader
and sHeader[1]
.
I reorganized your code.
Don’t write code as a heavily indented stream of consciousness. First, write the normal flow. The first step in a code review is to read and verify the normal flow, it should be obvious. Second, add exception handling. Third, add error handling. Sensibly minimize indentation.
See Indent Error Flow.
For example, a simple first draft,
func AuthFirstDraft(req *http.Request) error {
// TODO: improve error detection and handling
sHeader := strings.SplitN(req.Header.Get("authorization"), " ", 3)
devEnv := os.Getenv("authorization")
if len(sHeader) > 1 && devEnv == sHeader[1] {
return nil
}
return fmt.Errorf("error")
}
Code review is an iterative process. If there are significant changes, open a new question with the revised code.