<p>Hi guys. Looking for feedback on a program I've put together as an exercise. I got some of this code from SocketLoop website to get me started.</p>
<p>Currently it's a simple find and replace tool that works in the directory program resides. Variable naming is terrible at the moment, I think. Apologies, can't find a way to highlight code.</p>
<p>Any design advice or coding critique is welcome. </p>
<p>thanks</p>
<p><begin></p>
<p>package main</p>
<p>import (
"bytes"
"flag"
"fmt"
"io/ioutil"
"os"
"path/filepath"
)</p>
<p>func main() {</p>
<pre><code>var fileExtension string
var fileOutputMod string
var textInput string
var textOutput string
flag.StringVar(&fileExtension, "e", "nullExtension", "Default extension")
flag.StringVar(&fileOutputMod, "m", "nullPrefix", "Default prefix")
flag.StringVar(&textInput, "i", "nulltextInput", "Default input text")
flag.StringVar(&fileOutputMod, "o", "nulltextOutput", "Default output text")
flag.Parse()
fmt.Println("\n")
if fileExtension == "nullExtension" {
fmt.Println("Set file extension for target files using the -e switch then the file extension. Example: -e .txt\n")
} else {
fmt.Println("Setting file extension for target files to", fileExtension, "\n")
}
if fileOutputMod == "nullPrefix" {
fmt.Println("Set prefix for output file name by using the -m switch then the prefix name. Example: -m mod_\n")
} else {
fmt.Println("Setting all modified output files with a prefix of", fileOutputMod, "\n")
}
i := -1
dirname := "." + string(filepath.Separator)
d, err := os.Open(dirname)
if err != nil {
fmt.Println(err)
os.Exit(1)
}
defer d.Close()
files, err := d.Readdir(-1)
if err != nil {
fmt.Println(err)
os.Exit(1)
}
fileList := make([]string, 0)
for _, file := range files {
if file.Mode().IsRegular() {
if filepath.Ext(file.Name()) == fileExtension {
i++
fileList = append(fileList, file.Name())
}
}
}
//fmt.Println("File List:", fileList)
sliceCount := i
fmt.Println("\n")
fmt.Println(sliceCount+1, "files processed")
for i := sliceCount; i > sliceCount; i-- {
//fmt.Println("Value of i is now:", i)
}
for j := 0; j < i+1; j++ {
eachFile := (fileList[j])
input, err := ioutil.ReadFile(eachFile)
if err != nil {
fmt.Println(err)
os.Exit(1)
}
output := bytes.Replace(input, []byte("e"), []byte("z"), -1)
eachFileOut := eachFile
modFileOut := fileOutputMod + eachFileOut
if err = ioutil.WriteFile(modFileOut, output, 0666); err != nil {
fmt.Println(err)
os.Exit(1)
}
}
</code></pre>
<p>}</p>
<p><end></p>
<hr/>**评论：**<br/><br/>nathankerr: <pre><p>It's hard to critique your code when I don't really know what you are trying to do. You say:</p>
<blockquote>
<p>Currently it's a simple find and replace tool that works in the directory program resides.</p>
</blockquote>
<p>This is vague. From reading your code and making some guesses, I think you are trying to:</p>
<blockquote>
<p>Make a copy of all files with the extension specified by <code>-e</code>, prefixed with the string specified by <code>-m</code>, replacing all occurrences of the string specified with <code>-i</code> with that specified by <code>-o</code>.</p>
</blockquote>
<p>The code you provided does not do this, but it looks like what you intend to do. Often, the hardest part of programming is figuring out what the program needs to do. Only after knowing what to do can you figure out how to do it.</p>
<p>I put your code on the <a href="https://play.golang.org/p/tRL1HEPIvH" rel="nofollow">playground</a>, and use those line numbers for reference. My version is <a href="https://play.golang.org/p/ubOL-fk2Wl" rel="nofollow">https://play.golang.org/p/ubOL-fk2Wl</a></p>
<h2>Flag Variables</h2>
<p>I think the main reason to use flag variants like <code>flag.StringVar</code> is when you need to associate the flag with a variable outside of the current scope. In this case, it is better to use <code>flag.String</code> because it reduces the number of lines by two and reduces the risk of referencing the wrong variable. Line 21, for example, uses <code>fileOutputMod</code> when it should use <code>textOutput</code>.</p>
<p>I would change lines 13-21 to:</p>
<pre><code>fileExtension := flag.String("e", "nullExtension", "Default extension")
fileOutputMod := flag.String("m", "nullPrefix", "Default prefix")
textInput := flag.String("i", "nulltextInput", "Default input text")
textOutput := flag.String("o", "nulltextOutput", "Default output text")
</code></pre>
<p>While this forces dereferencing to get the values, it reminds me to separate the configuration and actual functionality of the program. Therefore I split <code>main</code> after line 38 (where the command line processing ends) into a separate function. Having a second function focuses <code>main</code> on the problem of dealing with the user (e.g., parsing and validating input) and the new function on the problem the program is intended to solve. To maintain this separation, I also move the declaration of <code>dirname</code> (line 41) to <code>main</code> and pass it to the new function. Keeping these separate helps in testing.</p>
<p>Changing the variable declarations also results in some compile errors:</p>
<pre><code># command-line-arguments
./findr.go:15: textInput declared and not used
./findr.go:16: textOutput declared and not used
</code></pre>
<p>In this case those variables are not used or validated. The previous code did not have this compile error because the way the variables were set used them. Since I am trying to improve, but not change the functionality of your code, I comment out these two lines.</p>
<p>Also, the descriptions and default values for these flag variables are not helpful. You can check if a string is empty by comparing it to <code>""</code>. This avoids magic strings like <code>"nullExtension"</code>, and lets you validate what actually matters, e.g., that <code>fileExtension</code> is not empty. I don't make any changes related to this, even though it is really annoying, because I am trying to not change the functionality of your code.</p>
<h2>i, Looping Over <code>fileList</code></h2>
<ul>
<li>defined on line 39</li>
<li>first used on line 60</li>
<li>weird machinations in lines 68-73</li>
<li>ends up being (line 75) the length of <code>fileList</code> + 1</li>
</ul>
<p>It seems as if you don't know how to get the length of a slice or how to use range to loop over a slice.</p>
<ul>
<li>Get the length with <code>len(fileList)</code></li>
<li>Loop over the slice with <code>for _, eachFile := range fileList</code></li>
</ul>
<p>If you actually needed the structure you used for <code>i</code> then:</p>
<ul>
<li>declare the variable as close to its first use as possible, in this case right before line 56</li>
<li>have a better name</li>
<li>if the name is not sufficient to indicate the use, write a comment</li>
<li>weird machinations should always have a comment describing what they intend to accomplish</li>
</ul>
<p>Keeping these in mind will either simplify the code or justify it's complexity.</p>
<h2>Other Notes</h2>
<ul>
<li>It might be better to not assume that <code>dirname</code> is always the current directory.</li>
<li>Line 70 prints the number of files process before the processing takes place. If this output is not needed or can be done after processing, then the loops on lines 57 and 75 can be combined, removing the need (and the allocations for) <code>fileList</code></li>
</ul></pre>korilius: <pre><p>Thanks for taking the time to reply. I'm going to re-read your post and digest as best as I can.</p>
<p>Given what you've seen and what you know, is there a better design way to implement a find and replace tool? If so, it would certainly help me be more efficient with any future programs I practice writing.</p>
<p>When you say length of slice, does that mean how many elements (if that is right word) are in said slice?</p>
<p>Line 70 gets the number of files with the specified file extension in current directory. I haven't figured out / done the part to list only files that contain the text or phrase to be searched and replaced. </p>
<p>Thanks again!</p></pre>Terudax: <pre><p>I recommend posting your code on <a href="https://gist.github.com/" rel="nofollow">https://gist.github.com/</a> and linking us. </p></pre>ChristophBerger: <pre><p>Another option is <a href="https://play.golang.org" rel="nofollow">https://play.golang.org</a> </p></pre>
