snippetgoMinor
Create a new unique file
Viewed 0 times
uniquefilenewcreate
Problem
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.
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)Solution
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:
You should use
Functions
You need more functions... still. I would create a function to control the file creation process (note the use of
Result
OK, now your code looks like:
Error handling
You have
Alternative
If it were me, I would do two things.....
First up, create a directory for the actual process you are running as.
then, ensure you use that in your path:
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:
And a matching function:
Now, you can simplify the whole thing (by modifying
and your whole code then becomes:
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")
}filepathYou 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 uint64And 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)
.....Code Snippets
func buildFileName() string {
return time.Now().Format("20060102150405")
}import (
"path/filepath"
)
filenamePrefix := filepath.Join(exeDir, "debug", buildFileName())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)
}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)myid := fmt.SPrintf("Process_%d", os.Getpid())Context
StackExchange Code Review Q#132025, answer score: 7
Revisions (0)
No revisions yet.