4
\$\begingroup\$

For debugging purposes I am storing requests into files. This code is supposed to create a new file for each request.

Somehow it doesn't look very nice, can it be improved? Additionally it would be nice if it could be made atomic.

filenamePrefix := exeDir + "/debug/" + time.Now().Format("20060102150405")
var filename string

for i := 0 ; ; i++ {
    if (i > 100) {
        log.Print("Could not find free filename")
        return
    }

    filename = filenamePrefix + "-" + strconv.Itoa(i)
    _, err := os.Stat(filename);
        if os.IsNotExist(err) {
        break
    }
}

fd, err := os.Create(filename)
if err != nil {
    log.Print(err.Error())
    return
}
defer fd.Close()

io.Copy(fd, request.Body)
\$\endgroup\$
5
\$\begingroup\$

Your code has a few items in it which concern me in a few small ways, and, in general, while I think your code is OK functionally, it could be done better (I am biased, I have done similar things, and ended up doing it differently).

First up, my concerns...

Timestamp function

while it is a 1-liner, I would still have a method to encapsulate the time formatting:

func buildFileName() string {
    return time.Now().Format("20060102150405")
}

filepath

You should use path/filepath to manage your path names:

import (
  "path/filepath"
)

filenamePrefix := filepath.Join(exeDir, "debug", buildFileName())

Functions

You need more functions... still. I would create a function to control the file creation process (note the use of %d instead of string.Itoa...):

func pickFileName(prefix string) (string, error) {
    for i := 0; i < 100; i++ {
        fname := fmt.Sprintf("%s_%d", prefix, i)
        if _, err := os.Stat(fname); os.IsNotExist(err) {
            return fname, nil
        }
    }
    return "", fmt.Errorf("Unable to create a unique file with the prefix %v in 100 tries", prefix)
}

Result

OK, now your code looks like:

filenamePrefix := filepath.Join(exeDir, "debug", buildFileName())
filename, err := pickFileName(filenamePrefix)
if err != nil {
    log.Print(err)
    return
}

fd, err := os.Create(filename)
if err != nil {
    log.Print(err)
    return
}
defer fd.Close()

io.Copy(fd, request.Body)

Error handling

You have log.Print(err.Error()) but log.Print(err) is fine. Still, you should probably be returning an error, instead of just logging it....

Alternative

If it were me, I would do two things.....

First up, create a directory for the actual process you are running as.

myid := fmt.SPrintf("Process_%d", os.Getpid())

then, ensure you use that in your path:

filenamePrefix := filepath.Join(exeDir, "debug", myid, buildFileName())

Now, you have a folder dedicated to the current process (and collisions with other running instances of your process are impossible - at the same time - though there may be an old instance of that same folder for an older process - that's OK).

Then, create a counter (a "global") too:

var debugCounter uint64

And a matching function:

func nextDebugId() string {
    return fmt.Sprintf("%d", atomic.AddUint64(&debugCounter, 1))
}

Now, you can simplify the whole thing (by modifying buildFileName) to:

func buildFileName() string {
    return time.Now().Format("20060102150405") + "_" + nextDebugId()
}

and your whole code then becomes:

fullpath := filepath.Join(exeDir, "debug", myid, buildFileName())

fd, err := os.Create(fullpath)
.....
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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