<p>So I was making a function that interfaces with a database and has errors implemented like this:</p>
<pre><code> // A function to create a new post
func CreatePost(title string, content string) (error) {
// Open a db connection and check errors
db, dbErr := InitDb()
defer db.Close()
if dbErr != nil {
return errors.New("post creation failed")
}
// Migrate the schema
db.AutoMigrate(&Post{})
// Create a new post
createErr := db.Create(&Post{Title: title, Content: content}).Error
if createErr == nil {
return nil
} else {
return errors.New("post creation failed")
}
}
</code></pre>
<p>Is better way of doing this? Problem with this is I would have check for return value every time I call the function which is good in the sense that it forces me to check and handle any errors. But I was wondering if this can be written in a way where I can discard the return value.</p>
<hr/>**评论：**<br/><br/>mcandre: <pre><p>Return a non-nil error value.</p></pre>zeus-man: <pre><p>Could you please elaborate on that?</p></pre>jrwren: <pre><p>the language should have been named "if err not nil"</p>
<p>on every call to something which may err write this:
<code>
if err != nil {
return err
}
</code></p>
<p>your code will be littered with them. it will look like tons of noise, then, after a bit, you won't even notice the litter. you'll live in squalor and not care about your captor.</p></pre>zeus-man: <pre><p>Look what you made me do.</p>
<pre><code>// Update an existing post
func UpdatePost(db *gorm.DB, title string, content string, postId string, author string) (error) {
if db.HasTable(Post{}) != true {
db.AutoMigrate(Post{})
}
// SELECT * FROM post WHERE id = postId;
var posts Post
err := db.First(&posts, postId).Error
if err != nil {
return fmt.Errorf("post update failed: %v", err)
}
// Update existing post with new information if no errors
err = db.Model(&posts).Where("id = ?", postId).Update("Title", title).Error
if err != nil {
return fmt.Errorf("post update failed: %v", err)
}
err = db.Model(&posts).Where("id = ?", postId).Update("Content", content).Error
if err != nil {
return fmt.Errorf("post update failed: %v", err)
}
err = db.Model(&posts).Where("id = ?", postId).Update("Date", time.Now()).Error
if err != nil {
return fmt.Errorf("post update failed: %v", err)
}
return nil
}
</code></pre>
<p>They're everywhere!</p></pre>metamatic: <pre><p>You should probably wrap the DB error rather than hiding what actually happened. Also, don't defer closing the database until you know it opened without an error. So:</p>
<pre><code>db, err := InitDb()
if err != nil {
return fmt.Errorf("post creation failed: %v", err)
}
defer db.Close()
</code></pre>
<p><a href="https://blog.golang.org/error-handling-and-go" rel="nofollow">https://blog.golang.org/error-handling-and-go</a></p>
<p>Similarly for the second part, and removing the unnecessary else clause:</p>
<pre><code>// Create a new post
err := db.Create(&Post{Title: title, Content: content}).Error
if err != nil {
return fmt.Errorf("post creation failed: %v", err)
}
return nil
</code></pre>
<blockquote>
<p>But I was wondering if this can be written in a way where I can discard the return value.</p>
</blockquote>
<p>I don't think I understand what you're asking for. Discard which return value? The error value returned? That's what tells you what actually happened, so why would you want to discard it?</p></pre>zeus-man: <pre><p>The dB.Create() method has an additional property(not sure what it’s called) you can access ‘Error’. So I can do err := dB.Create.Error if i want the error or just dB.Create if I don’t. How does that work?</p></pre>metamatic: <pre><p>Oh, I see. You can put an underscore in the appropriate place to ignore any return value you don't want. For example, if a function returns (int, error) but you only want the error, you might do <code>_, err := fn()</code> instead of <code>x, err := fn()</code></p>
<p>And if you're really bad, you can do <code>x, _ := fn()</code> to ignore the error.</p></pre>zeus-man: <pre><p>Any suggestions on how I should handles errors in a function that returns a boolean? Is this a bad idea:</p>
<pre><code>// Given a email check if user exists!
func DoesUserExist() (bool, error) {
// Open a db connection
db, dbErr := InitDb()
defer db.Close()
if dbErr != nil {
return false, errors.New("db initialization failed during user existence check")
}
// Check if user table exists
var user User
if db.HasTable(user) {
return true, nil
} else {
return false, nil
}
}
</code></pre></pre>shovelpost: <pre><p>Why do you keep doing <code>InitDb()</code> in all of your functions? Initialization and dependency injection should ideally happen once when your program starts. </p>
<p>Some tips:</p>
<ul>
<li><p>Read: <a href="http://www.alexedwards.net/blog/organising-database-access" rel="nofollow">Practical Persistence in Go: Organising Database Access</a></p></li>
<li><p>Use defer after you have checked for error.</p></li>
<li><p>No need to name each <code>err</code> variable differently unless you are shadowing it. </p></li>
<li><p>To check for error shadowing use <code>go vet -shadow</code> or <a href="https://github.com/alecthomas/gometalinter" rel="nofollow"><code>gometalinter</code></a>.</p></li>
</ul>
<p>Use the <a href="https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow" rel="nofollow">"happy path"</a>:</p>
<pre><code>if db.HasTable(user) {
return true, nil
}
return false, nil
</code></pre></pre>zeus-man: <pre><p>Thanks for the suggestion, I was thinking about the reinitiaztion of the db as well. My InitDb is defined like this:</p>
<p>package database</p>
<pre><code>import (
"github.com/jinzhu/gorm"
"errors"
)
func InitDb() (*gorm.DB, error){
// Opening db
db, err := gorm.Open("sqlite3", "./data.db")
// Error
if err != nil {
return nil, errors.New("failed to open/create db")
}
return db, nil
}
</code></pre>
<p>Should I do it this way:</p>
<pre><code>// Given a email check if user exists!
func DoesUserExist(db *gorm.DB) (bool) {
defer db.Close()
// Check if user table exists
var user User
if db.HasTable(user) {
return true
}
return false
}
</code></pre>
<p>And then in my main:</p>
<pre><code>package main
import (
"twain/database"
)
func main() {
// Open a db connection and check for any errors
db, err := InitDb()
if err != nil {
fmt.Errorf("db initialization failed: %v", err)
}
defer db.Close()
doesExist := DoesUserExist(db)
print("User exists: ", doesExist)
}
</code></pre>
<p>Or is there a better way of doing this?</p></pre>shovelpost: <pre><p>This is much better yes but I would go one step further. </p>
<p>Wrap <code>*gorm.DB</code> in your own type and define methods to it:</p>
<pre><code>type DB struct {
orm *gorm.DB
}
// NewDB should be called when program starts. Caller is
// responsible for closing.
func NewDB(driver, dataSource string) (*DB, error) {
// call InitDB here
}
func (db *DB) UserExists(email string) bool { ... }
</code></pre>
<p>This has the big advantage of abstracting the database code. If in the future you decide that you want to get rid of Gorm and use <code>database/sql</code>, the structure of your program will stay exactly the same and you only have to change the internals:</p>
<pre><code>type DB struct {
db *sql.DB
}
</code></pre>
<p>Another big advantage is that you can use the power of interfaces to make your code more flexible and testable. For example if you write a function that needs to check if a user exists, you can define a "throwaway" interface (<a href="https://github.com/golang/go/wiki/CodeReviewComments#interfaces" rel="nofollow">in the consumer not the producer!</a>):</p>
<pre><code>type userExister interface{
UserExists() bool
}
</code></pre>
<p>And use that as an argument in your function:</p>
<pre><code>func Foo(ue userExister) {
// call ue.UserExists()
}
</code></pre>
<p>And the real magic happens when you pass your own type DB (which can have dozens of other methods) to Foo because it satisfies the throwaway interface implicitly:</p>
<pre><code>func main() {
db, err := NewDB("sqlite3", "./data.db")
if err != nil {
log.Errorf("db initialization failed: %v", err)
}
defer db.Close()
Foo(db) // Works because db satisfies userExister
}
</code></pre>
<p>This has the additional advantage of reducing tight coupling between packages.</p>
<p>Suggested material:<a href="https://dave.cheney.net/2016/08/20/solid-go-design" rel="nofollow"> SOLID Go Design</a></p></pre>metamatic: <pre><p>Well, in general your HasTable method will also possibly return an error value, so you'll need to deal with that. I'd be inclined to have a variable for the boolean you're going to return, and initialize it to whatever you want the fail case to be (say, false). Then all your return statements are either <code>return result, nil</code> or <code>return result, fmt.Errorf(...)</code>.</p>
<p>And again, don't defer db.Close() until you know it's open. For some databases, closing a closed database will cause an error which may obscure the details of what actually happened.</p></pre>jabbrwcky: <pre><p>If you don't want to have error handling all over the place, I can recommend this talk: <a href="https://youtu.be/1B71SL6Y0kA" rel="nofollow">https://youtu.be/1B71SL6Y0kA</a></p>
<p>It demonstrates using lifting from the functional world to reduce clutter.</p>
<p>Takes a bit getting used to, but can make things a lot nicer IMO.</p>
<p>Apart from that, the is no "throwing" of errors like in Java, you just handle them or pass them on as-is or wrapped.</p></pre>csos95: <pre><p>I rewrote some of your code and added comments explaining.</p>
<pre><code> // A function to create a new post
func CreatePost(title string, content string) error {
// Open a db connection and check errors
db, err := InitDb()
if err != nil {
// use errors.Wrap to wrap the error with a useful message
return errors.Wrap(err, "post creation failed")
}
// defer after you have checked for errors opening the database
defer db.Close()
// Migrate the schema
db.AutoMigrate(&Post{})
// Create a new post
// reuse err variable
err = db.Create(&Post{Title: title, Content: content}).Error
if err != nil {
return errors.Wrap(err, "post creation failed")
}
return nil
}
</code></pre></pre>Justinsaccount: <pre><p>errors.wrap of nil is nil, so the last few lines can just be</p>
<pre><code>err = db.Create(&Post{Title: title, Content: content}).Error
return errors.Wrap(err, "post creation failed")
</code></pre></pre>
