hossein1376 / grape

Modern, zero-dependency HTTP library for Go
https://pkg.go.dev/github.com/hossein1376/grape
MIT License
149 stars 5 forks source link

Less complexity #5

Closed MarcelH-Tabeo closed 4 months ago

MarcelH-Tabeo commented 5 months ago

This is what my linter shows me:

grape/serialize.go:53:1 - ReadJson has complexity: 14
  complexity = 1
  + 1 (found 'if' at line: 59, complexity = 2)
  + 1 (found 'if with (35) lines >= 10 ' at line: 59, complexity = 3)
  + 1 (found 'if with (35) lines >= 25 ' at line: 59, complexity = 4)
    + 2 (found 'switch' at line: 64, complexity = 6)
    + 2 (found 'case with (5) lines >= 3 ' at line: 77, complexity = 8)
      + 3 (found 'if' at line: 78, complexity = 11)
    + 2 (found 'case with (3) lines >= 3 ' at line: 86, complexity = 13)
  + 1 (found 'if' at line: 96, complexity = 14)

And by looking at it, I would do an early return around line 59

func (s serializer) ReadJson(w http.ResponseWriter, r *http.Request, dst any) error {
    r.Body = http.MaxBytesReader(w, r.Body, s.maxBytes)
    dec := json.NewDecoder(r.Body)
    dec.DisallowUnknownFields()

    err := dec.Decode(dst)
    if err == nil {
        err = dec.Decode(&struct{}{})
        if err != io.EOF {
            return errors.New("body must only contain a single JSON value")
        }
        return nil
    }

    var syntaxError *json.SyntaxError
    var unmarshalTypeError *json.UnmarshalTypeError
    var invalidUnmarshalError *json.InvalidUnmarshalError

    switch {
    case errors.Is(err, io.EOF):
        return errors.New("body must not be empty")

    case errors.Is(err, io.ErrUnexpectedEOF):
        return errors.New("body contains badly-formed JSON")

    case errors.As(err, &invalidUnmarshalError):
        return err

    case errors.As(err, &syntaxError):
        return fmt.Errorf("body contains badly-formed JSON (at character %d)", syntaxError.Offset)

    case errors.As(err, &unmarshalTypeError):
        if unmarshalTypeError.Field != "" {
            return fmt.Errorf("body contains incorrect JSON type for field %q", unmarshalTypeError.Field)
        }
        return fmt.Errorf("body contains incorrect JSON type (at character %d)", unmarshalTypeError.Offset)

    case err.Error() == "http: request body too large":
        return fmt.Errorf("body must not be larger than %d bytes", s.maxBytes)

    case strings.HasPrefix(err.Error(), "json: unknown field "):
        fieldName := strings.TrimPrefix(err.Error(), "json: unknown field ")
        return fmt.Errorf("body contains unknown key %s", fieldName)

    default:
        return err
    }

    return nil
}

Then this is complexity 10 ! Just my 2 cents ;-)

hossein1376 commented 5 months ago

Hello! Thanks for bringing this up.

The goal of this function is to return a clear and helpful error message (if any). I don't believe the current implementation is complicated. It's a bit verbose, but it serves to have a better error message.

I appreciate your suggestion, but it doesn't follow the happy path pattern. Currently, it's obvious why and when we handle the error.
Using the err == nil expression is rare, and it's possible to be mistaken with the far more common err != nil one. Thus, it may result in confusion.

I encourage you to open a new merge request if you come up with a better or less verbose solution. My regards.