Skip to content
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

Abort vs panic #342

Closed
lilee opened this Issue Jun 14, 2015 · 3 comments

Comments

Projects
None yet
2 participants
@lilee
Copy link

lilee commented Jun 14, 2015

every time I want to return the error, i must write the following code

if err != nil {
    c.AbortWithError(code, err)
    return
}

it's ugly with so many return

I think using the panic maybe the best practice. Like Java's throw Exception

if err != nil {
    panic(err) // maybe err is ErrUserNotFound
}

err is defined as

type HTTPError interface {
    HTTPStatus() int
}

using the catchError middleware to catch the error and abort with the HTTPStatus

func catchError() gin.HandlerFunc {
    return func(c *gin.Context) {
        defer func() {
            if err := recover(); err != nil {
                switch err.(type) {
                case HTTPError:
                    e := err.(*errors.Error)
                    c.JSON(e.HTTPStatus(), e)
                    c.Abort()
                default:
                    c.AbortWithStatus(http.StatusInternalServerError)
                }
            }
        }()
        c.Next()
    }
}

lot's of ugly return are removed

@manucorporat

This comment has been minimized.

Copy link
Contributor

manucorporat commented Jun 14, 2015

@lilee currently the best practice is to do:

AbortWithError()
return;

and then a middleware reads c.Errors in order to do something with the errors.

Using a panic() is a bad idea:

  • It is not Go idiomatic. In Go, a panic() is a strict exception (or an assertion). i.e. something really bad (probably a bug) happened.
  • Calling panic() is slow
  • When panic() is called, most of the middlewares will not work correctly, since a panic() usually means 500.
  • Debugging: you will not be able to differentiate between real crashes (like access no nil) and errors.

Of course, you are free to use whatever you want, but using panic() to control logic normal flow is a anti-pattern in Go.

@manucorporat

This comment has been minimized.

Copy link
Contributor

manucorporat commented Jun 14, 2015

Tip: refactor your code into smaller functions, so instead of repeating:

if err != nil {
    c.AbortWithStatus(code, error)
    return 
}

you do:

func dosomething() error {
    err = dosomething1()
    if err != nil {
        return err
    }
    err = dosomething2()
    if err != nil {
        return err
    }
    err = dosomething3()
    if err != nil {
        return err
    }
}

func handler(c *gin.Context) {
    err := dosomething()
    if err != nil {
        c.AbortWithStatus(400, err)
        return;
    }
    // ...
}
@lilee

This comment has been minimized.

Copy link
Author

lilee commented Jun 15, 2015

Thanks for explaination

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.