3
\$\begingroup\$

How could one write in a more elegant way that if error is null, the result is true? I really need a bool as returned value.

func existsFile(pPath string) bool {
    _, errStat := os.Stat(pPath)

    if errStat != nil {
        return false
    }
    return true
}
\$\endgroup\$

closed as primarily opinion-based by dfhwze, Peter Taylor, t3chb0t, esote, 1201ProgramAlarm Jul 6 at 3:33

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ I'm not sure what do you mean by "more elegant" but you can return both your value and original error \$\endgroup\$ – Bohdan Stupak Jul 1 at 12:06
  • \$\begingroup\$ One line. return os.Stat(pPath) != nil. But it is questionable whether you should do this at all. Usually you're about to open it for I/O, in which case you should just do that and handle the failure as it actually occurs, rather than trying to foretell the future and risk all the timing-window problems that can arise when you do so. \$\endgroup\$ – user207421 Jul 2 at 4:32
  • \$\begingroup\$ @user207421 os.Stat returns two values. you need to pick one to compare. \$\endgroup\$ – Tudor Jul 2 at 8:09
  • \$\begingroup\$ This post and most answers ignore that fact that ost.Stat returning a non-nil error does not equate to file non-existence. There are many other possible reasons os.Stat might fail (e.g. permissions). The errors should be tested with os.IsNotExist. \$\endgroup\$ – Dave C Jul 7 at 11:57
10
\$\begingroup\$

Instead of checking with an if statement if errStat is null and then returning false:

if errStat != nil {
    return false
}
return true

you can return a Boolean expression:

return errStat == nil
\$\endgroup\$
  • \$\begingroup\$ As per my answer, without knowing what the OP means by elegant, I can understand why you'd answer with a shorter version. However, I would argue idiomatic golang and human-readable code is more important than brevity. Elegance is code that only requires you to read it once, to know what it does. \$\endgroup\$ – Elias Van Ootegem Jul 1 at 13:48
  • 3
    \$\begingroup\$ @EliasVanOotegem: Using a Boolian expression as an assignment in cases like this (were one variable depends on a Boolian expression) is very common and (well, in my opinion and probably many more software programmers) is cleaner and more human readable. Some refactoring services actually offer similar changes. Shorter can be more readable. If you write this function for a "a violent psychopath" you may need a book... \$\endgroup\$ – Eliahu Aaron Jul 1 at 14:06
  • 1
    \$\begingroup\$ :D Personal preference/taste I suppose. Not trying to argue your response wasn't valid mate... relax. I know bool expressions are very common, in my experience, though, bugs caused by something like return x >= y where it should've been return y >= x or return x < y are also very common, hence my avoiding them. \$\endgroup\$ – Elias Van Ootegem Jul 1 at 14:09
5
\$\begingroup\$

Elegance is the wrong criterion. The key criterion is correctness, which implies readability and maintainability.

As you read text, you notice misspelled words. Does that mean that you laboriously spell-check each word, letter-by-letter? In fact, most likely, you quickly and unconsciously scan the shape of the words. Only if the shape of a word seems odd do you pause and slowly check the spelling letter-by-letter.

Since readablity is so important, we choose a few idiomatic, instantly recognizable code forms. For example,

if err != nil {
    // ...
    return ...
}
// ...
return ...

Even if, in a particular case, we could write

return err == ...

The first, general form always works. The second, specialized form may not.

The second form is less maintainable.

If we find that

return err == ...

is insufficient, we probably have to replace it with the first form

if err != nil {
    // ...
    return ... 
}
// ...
return ...

To summarize, prefer a single, easily recognizable code form

if err != nil {
    // ...
    return ...
}
// ...
return ...

in particular

if err != nil {
    return false
}
return true

Idiomatic Go

Go wiki: Go Code Review Comments

This page collects common comments made during reviews of Go code, so that a single detailed explanation can be referred to by shorthands.

Indent Error Flow

Try to keep the normal code path at a minimal indentation, and indent the error handling, dealing with it first. This improves the readability of the code by permitting visually scanning the normal path quickly. For instance, don't write:

if err != nil {
  // error handling
} else {
  // normal code
}

Instead, write:

if err != nil {
  // error handling
  return // or continue, etc.
}
// normal code

If the if statement has an initialization statement, such as:

if x, err := f(); err != nil {
  // error handling
  return
} else {
  // use x
}

then this may require moving the short variable declaration to its own line:

x, err := f()
if err != nil {
  // error handling
  return
}
// use x

For readability, don't be idiosyncratic. Conform to the familiar Go standard library os package names. For example,

func Stat(name string) (FileInfo, error)

func IsExist(err error) bool

For your function,

func isFileExist(name string) bool {
    _, err := os.Stat(name)
    if err != nil {
        return false
    }
    return true
}
\$\endgroup\$
  • 2
    \$\begingroup\$ You start out well but your reasons for preferring five lines of code over one are entirely subjective. \$\endgroup\$ – user207421 Jul 2 at 4:31
  • \$\begingroup\$ "The first, general form always works. The second, specialized form may not." I'm not clear in which case the second form would not be equal to the first. Not arguing, just curious. \$\endgroup\$ – CoolAJ86 Jul 2 at 16:20
3
\$\begingroup\$

Elegant is hard to define, especially given the small snippet of code you've provided. I'll go down the list of different ways to write it. Note: Not all of them are what I'd call "elegant", though:

// as short as possible (NOT ELEGANT)
func fileExists(path string) (ok bool) {
    if _, err := os.Stat(path); err == nil {
        ok = true
    }
    return
}

This works by creating a variable for the return value. A bool defaults to the false value. Next, I stat the path, and if Stat returns no errors, I set the return variable to true. Return and only if Stat returned no errors, the function will return true.

// boolean assignment
if fileExists(path string) bool {
    _, err := os.Stat(path)
    return err == nil
}

This is shorter, because we're returning the boolean value resulting from the comparison of err. If no error is returned, the function will return true.

All things considered, this is not elegant

It's important to remember: code is written by humans, for humans to read, and maintain. The compiler is there to translate the human readable code to machine instructions. If you try to write something as short as possible, it's going to be harder for others to maintain/understand. There's 2 very well known quotes about this:

Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it? -- Brian Kernighan in The Elements of Programming Style

And

Always code as if the person who ends up maintaining your code is a violent psychopath who knows where you live. From Code for the maintainer

Keeping these things in mind, I'd say the code you have is easy to read, easy to maintain, and therefore elegant. There is one small change I'd suggest to have it be more "idiomatic":

func fileExists(path string) bool {
    if _, err := os.Stat(path); err != nil {
        return false
    }
    return true
}

The if <err-assigning-expression>; err != nil is the de facto standard way of checking error returns in golang, if you don't need the return values other than checking them. In this case, all you need the err for is to check whether it was a nil value of not, so assign & check in one if statement is what I'd recommend.

\$\endgroup\$

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