3
\$\begingroup\$

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?

\$\endgroup\$
2
  • \$\begingroup\$ "(I see lot of view's but no comments ;-) )" Nina watcher. Really! You only posted this about two hours ago. We are unpaid volunteers. \$\endgroup\$
    – peterSO
    Nov 22, 2017 at 13:32
  • \$\begingroup\$ @peterSO - sorry I just suspect that my code is perferct which is obviously not :) \$\endgroup\$ Nov 22, 2017 at 13:45

1 Answer 1

4
\$\begingroup\$

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 to log.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.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks a lot 1+ few clarification which I need in order to understand :) 1 . you write never fatal! why? 2. you write debug ? how should I change it to be printed only on debug, this is just trace… ? 3. you change the number req.Header …. 3) to 3 which is working, why it works with 2 either ? 4. I use bool that the caller can split with simple if to get the value , you suggest error , how should I adopt it to error if it’s better ? Thanks a lot, this is what I want in order to learn!!!! \$\endgroup\$ Nov 22, 2017 at 13:43
  • \$\begingroup\$ i've close the question but it will be great if you can still answer my questions , I want to learn from you. thanks! \$\endgroup\$ Nov 22, 2017 at 14:46
  • \$\begingroup\$ @Ninawatcher: See my revised answer. \$\endgroup\$
    – peterSO
    Nov 22, 2017 at 15:45
  • \$\begingroup\$ Thanks a lot , just last question, if I use the AuthFirstDraft should I use try catch block ?or can you provide example of the two caller when it success and if not how its recommended to catch the error ? thank a lot for this explanation!!!! \$\endgroup\$ Nov 22, 2017 at 16:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.