Let’s start with a puzzle. You’ve implemented a function to reverse text:

type Reverse string func (r Reverse) String() string { s := []rune(r); slices.Reverse(s); return string(s) } func reverse(s string) (string, Reverse) { r := Reverse(s); return r.String(), r } func main1() { h, w := reverse("olleh") fmt.Println(h, w) } 

And it works fine, printing hello hello. Good. You expand it to a “hello world” program:

func main2() { h, w := reverse("olleh") b, w := "beautiful", "dlrow" if b != "" { fmt.Println(b, w) } fmt.Println(h, w) } 

And it works, printing:

beautiful world hello world 

Great.

After a (long) while you come back and realize a staticcheck warning on the first short declaration: this value of w is never used (SA4006).

Okay, you’ll try to pull the declaration into the if:

func main3() { h, w := reverse("olleh") if b, w := "beautiful", "dlrow"; b != "" { fmt.Println(b, w) } fmt.Println(h, w) } 

But this produces different output. So you try again, simply eliminating the unused variable:

func main4() { h, _ := reverse("olleh") b, w := "beautiful", "dlrow" if b != "" { fmt.Println(b, w) } fmt.Println(h, w) } 

This also fails? Try it on the Go Playground.

You obviously understood all of this, so take the “you” in a metaphorical sense.

The Point

The point I’m trying to make here is that variables in the same scope can have subtle interactions that make (justified) refactoring tricky.

In my opinion, using the if statement's initializer pattern (as done in main3) is the clearest approach, ensuring variables only exist in the scope where they're needed. You should start from there. The mistake in main3 stems not from a wrong technique, but from the subtle variable interactions in the code you're refactoring. Obviously, this is a style issue, so your different opinion is justified.

I’ve written the static Go analyzer scopeguard to point out places where a tighter scope may be beneficial to code readability - and, as mentioned above, it’s still a personal style question.

I ran it on my personal projects and was surprised by the opportunities, especially in tests where I find

 if got, want := s[i], byte('b'); got != want { t.Errorf("Expected %q, got %q", want, got) } i++ 

is much more readable than:

 got := s[i] i++ if got != byte('b') { t.Errorf("Expected %q, got %q", byte('b'), got) } } 

In the first example, got only lives inside the if's scope. This locality makes the code easier to reason about, as you can be sure got isn't used or its calculation influenced elsewhere. In the second example got is no longer s[i].

Try scopeguard on your codebase and see what you think. I appreciate constructive feedback, even when you don’t want to run the static analyzer.

submitted by /u/___oe
[link] [comments]