Flag fall through logic review

[Leam Hall]

I'm re-learning Go and building a program to create characters for role-playing games. The program uses text files to build lists, and I'd like to let the user change the lists. For example, a modern day Earth game would only have human characters, but a Star Trek (TNG) game might have other species options, and a Star Wars game might have an even different set.

The logic is:
1. If run as provided, the program uses a default directory and default text files.
2. A user can modify one of the default text files to suit their needs.
3. A user can specify a custom directory for text files.
3.1. If a file of the same name as a default file exists in the custom directory, the
program will use the custom file.
3.2. If a custom file does not exist, the program will use the default.

How would you feel about that, as a user?
How can the code and logic be improved?

Thanks!

Leam

###
1 // Package cli_fallthrough tests setting various vars.
2
3 package main
4
5 import (
6 "errors"
7 "flag"
8 "fmt"
9 "io/fs"
10 "os"
11 fp "path/filepath"
12 )
13
14 func exists(filepath string) bool {
15 if _, err := os.Stat(filepath); errors.Is(err, fs.ErrNotExist) {
16 return false
17 }
18 return true
19 }
20
21 func isDir(filepath string) bool {
22 // Need better logic here. What to do if there is an err?
23 dir, _ := os.Stat(filepath)
24 if dir.IsDir() {
25 return true
26 }
27 return false
28 }
29
30 func main() {
31 customData := false
32 datadir := "data"
33 datafiles := map[string]string{
34 "species": "species.txt",
35 "conflicts": "conflicts.txt",
36 "positives": "positives.txt",
37 }
38
39 custom_datadir := flag.String("custom", "custom", "custom directory for data")
40 flag.Parse()
41
42 if exists(*custom_datadir) {
43 customData = isDir(*custom_datadir)
44 }
45
46 for k, v := range datafiles {
47 datafiles[k] = fp.Join(datadir, v)
48 if customData {
49 filepath := fp.Join(*custom_datadir, v)
50 if exists(filepath) && !isDir(filepath) {
51 datafiles[k] = filepath
52 }
53 }
54 _, err := os.Stat(datafiles[k])
55 if os.IsNotExist(err) {
56 fmt.Println(err)
57 }
58 fmt.Printf("using %s.\n", datafiles[k])
59 }
60 }

###
--
Systems Programmer (reuel.net/resume)
Scribe: The Domici War (domiciwar.net)
General Ne'er-do-well (github.com/LeamHall)

[Brian Candler]

(BTW, sharing your code on play.golang.org makes it easier to format and read)

On Tuesday, 16 November 2021 at 14:11:54 UTC leam...@gmail.com wrote:

14 func exists(filepath string) bool {
15 if _, err := os.Stat(filepath); errors.Is(err, fs.ErrNotExist) {
16 return false
17 }
18 return true
19 }

You are silently ignoring all errors here, apart from fs.ErrNotExist.  (You already identified that problem in your isDir function).

Basically there are three possible results: file exists, file doesn't exist, or something went wrong.  The traditional way to return that would be as a pair of values (bool, err)

Alternatively, it might be reasonable to panic(err) on any other non-nil error here.

 

42 if exists(*custom_datadir) {
43 customData = isDir(*custom_datadir)
44 }

Screams "pointer not checked for nil" to me.

[Leam Hall]

Brian, thanks! Here's the playground link: https://play.golang.org/p/Ef8D4CF-kKD

For the errors in lines 14-19, what other failures do you see needing to be handled? I'm still not comprehending how to check for "isReadable()", but that would be a useful next step. Of course, when the file is ingested there will need to be error checking there, too.

On lines 42-44, wouldn't the nil pointer issue be that the directory didn't exist? So far the code seems to handle that, but I'm probably missing a few things.

Leam

[Sean Liao]

I would go for something more like: https://play.golang.org/p/MyorlUwOL9s

and not bother with the intermediate checks, just read directly and report any issues you encounter

[Brian Candler]

On Tuesday, 16 November 2021 at 14:49:54 UTC leam...@gmail.com wrote:

For the errors in lines 14-19, what other failures do you see needing to be handled?

IMO, programs should handle *all* errors - especially the ones you don't anticipate.

But if the filesystem is starting to return EIO or whatever, then the system is toast anyway.  I think it's reasonable for the program to panic() in that case ("unexpected error: %v", err)

 

I'm still not comprehending how to check for "isReadable()", but that would be a useful next step. Of course, when the file is ingested there will need to be error checking there, too.

Nah: just read it, and if it fails, report the error.

 

On lines 42-44, wouldn't the nil pointer issue be that the directory didn't exist? So far the code seems to handle that, but I'm probably missing a few things.

The issue is that the expression "*custom_datadir" will panic if custom_datadir is nil.