<p>So, I have read a lot of articles and a few ebooks on Golang. I finally wrote my first program a few days ago. I would like to get some comments on what I am doing wrong, or how I can make this better. It is a simple program that takes a filename and encrypts/decrypts it with a password. I am not concerned about the encryption part(whether it is secure), but rather would like to understand the language more. I have not been able to include any structs, pointers or goroutines, which I am hoping you guys can help me with.</p>
<p>Here is the link to a repo:
<a href="https://github.com/r3s/Go-Crypter">https://github.com/r3s/Go-Crypter</a></p>
<p>Thanks</p>
<hr/>**评论：**<br/><br/>StabbyCutyou: <pre><p>As a general tip, you should really throw up a very basic rundown of what the project is / does on the readme. A slightly more formalized version of what you wrote here about the file encryption is likely good enough to get started.</p>
<p>Right off the bat I can see you're actively commenting your code, so thats a huge plus.</p>
<p>You mention that you weren't able to use any structs or goroutines, but that doesn't make it a bad thing. Those things are here for when we need them, and you don't always need them.</p>
<p>However, one idea for you, as practice for using goroutines as a way to do work concurrently, what if the user gives several files on the command line, or a directory - you could take your current approach, and call a goroutine for each file provided / each file in the directory. That way you could do more work at once, and not need any significant refactors.</p>
<p>You'll have to learn a bit about WaitGroups to do it correctly (since you'll need the program itself to wait until all files are encrypted before it exits, and WaitGroups are how you'd accomplish that), but it'd be a good extension of your current approach as a learning tool for more advanced go concepts.</p>
<p>Good luck!</p></pre>p765: <pre><p>Thanks for your input. I was thinking the same way for using goroutines. Also, I was thinking of rigging up a small GUI with go-qml. </p></pre>neoasterisk: <pre><p>In my opinion the function HandleError() is bad practice as it helps you not thinking about actually handling the errors. I am pretty sure that panicking has it's uses but should you really be using panic() after every error? Handle the errors normally instead.</p>
<p>I also believe that HandleError() makes code less readable. I had to navigate to the helpers.go file (which is fine) but only to discover (in horror) that it uses panic inside to handle every error. Again personal opinion!</p>
<p>As a last note, I've noticed some <a href="https://www.reddit.com/r/golang/comments/41c6q0/show_file_finder_first_go_project_would_love/cz1a8hf">good pointers</a> of what could be improved on a small command line program on that other thread. I am pretty sure some of those pointers apply to your situation as well.</p></pre>dchapes: <pre><p>Indeed, using <code>panic</code> for expected errors such as file not found or I/O errors is just plain wrong. <code>HandleError</code> in no way is handling the error.</p>
<p>Functions/methods that don't want-to/need-to/can't do something with an error itself, should return the <code>error</code> to their caller. For a simple command line utility you can have your <code>main</code> function then call <code>log.Fatal</code> to report any unrecoverable errors to the user (to make <code>log</code> output nicer you could call <code>log.SetPrefix("programname: "); log.SetFlags(0)</code> at the start of <code>main</code>). Note that <code>log</code> and <code>log.Fatal</code> will also be default write to <code>os.Stderr</code> which is where error messages belong; do not write error messages to <code>os.Stdout</code>.</p></pre>p765: <pre><p>So I should return the error from the function and check if there is any error in main and log it right. Please correct me if I have understood wrong.</p></pre>weberc2: <pre><p>That's correct, but it's important to understand that we do this because it decouples the responsibility of handling errors from the responsibility of encrypting or decrypting a file. As your program grows, you might find that you want to encrypt a file, but this time you want to handle the error differently (maybe you want to print a user-friendly string, or perhaps you'd like to retry the encryption). This sort of growth is so frequent that we try to hard-code as few things as possible. Often, it makes sense to let the error bubble all the way up to your <code>main()</code> function, who can decide how to handle the error.</p>
<p>Similarly, your encrypt and decrypt file functions could be <em>even more</em> decoupled if you had them take an <code>io.Reader</code> instead of a filepath string. This means your function doesn't care where the data comes from or where the encrypted data goes to--it could come from a file, a socket, a bunch of bytes on disk, etc or be written to any of those. This is how I would rewrite your code to decouple the file stuff from the encryption stuff:</p>
<pre><code>// Since we're no longer dealing with files, we'll rename this to "encrypt()"
func encrypt(src io.Reader, dst io.Writer, key string) error {
// TODO
}
func main() {
srcFile, err := os.Open("/source_file.txt")
if err != nil {
panic(err)
}
defer srcFile.Close()
// encryptFile() doesn't need to decide on the file naming conventions.
// now it can be reused for encrypting files whose extension are not
// aes!
dstFile, err := os.Create("/destination_file.aes")
if err != nil {
panic(err)
}
defer dstFile.Close()
if err := encrypt(srcFile, dstFile, "key"); err != nil {
panic(err)
}
}
</code></pre>
<p>Because we've decoupled the file i/o stuff from the encryption (and decryption), we can read a file, encrypt it to a byte buffer, and send the data over HTTP:</p>
<pre><code>func main() {
srcFile, err := os.Open("/source_file.txt")
if err != nil {
panic(err)
}
defer srcFile.Close()
// Write the encrypted data to a buffer
buf := &bytes.Buffer{}
if err := encrypt(srcFile, buf, "key"); err != nil {
panic(err)
}
// Then send that data to Google
rsp, err := http.Post("http://google.com/", "application/octet-stream", buf)
if err != nil {
panic(err)
}
// Make sure Google didn't have a problem with our data
if rsp.StatusCode < 200 || rsp.StatusCode > 299 {
panic("HTTP problem: " + rsp.Status)
}
}
</code></pre>
<p>To demonstrate the <code>decryptFile</code> side of things:</p>
<pre><code>func decrypt(src io.Reader, dst io.Writer, key string) error {
// TODO
}
func isBadData(err error) bool {
// TODO
}
// This program reads encrypted data from HTTP requests, decrypts it, and stores
// it in files. For example, an HTTP POST request to
// http://localhost:8080/my_file.aes will attempt to decode the data in the
// request body and write it to the my_file.aes.
func main() {
http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
defer r.Body.Close()
path := filepath.Join("/tmp", r.URL.Path)
file, err := os.Create(path)
if err != nil {
// log the error and write the response to the http client
fmt.Fprintf(os.Stderr, "Error opening file '%s': %v\n", path, err)
code := http.StatusInternalServerError
w.WriteHeader(code)
fmt.Fprintln(w, code, http.StatusText(code))
return
}
if err := decrypt(r.Body, file, "key"); err != nil {
// if the error is because of bad data, we'll need to let the client
// know that their data was bad. If it was some other error (for
// example, an I/O error), then we'll just let the user know that
// the problem was on our end.
if isBadData(err) {
code := http.StatusBadRequest
w.WriteHeader(code)
fmt.Fprintln(w, code, http.StatusText(code))
return
}
fmt.Fprintf(os.Stderr, "Error opening file '%s': %v\n", path, err)
code := http.StatusInternalServerError
w.WriteHeader(code)
fmt.Fprintln(w, code, http.StatusText(code))
return
}
})
// This starts up a server, listening on port 8080
if err := http.ListenAndServe(":8080", nil); err != nil {
panic(err)
}
}
</code></pre></pre>p765: <pre><p>I've made some of the suggestions you pointed out. I see that you mentioned decoupling by using a io.Reader which I will try to include. Thank you for all your comments</p></pre>p765: <pre><p>Thanks, I'll look up ways to handle errors properly and modify the code. Thank you for the link also</p></pre>xrstf: <pre><p><code>go fmt</code> is your friend.</p></pre>p765: <pre><p>Thanks. I actually thought Visual Studio Code automatically ran <code>go fmt</code> on saving. But seems like it doesn't. I've already updated the repo after running <code>go fmt</code></p></pre>ergotayours: <pre><p>Crypto code review is not my strong suit, so I'll just stick to formatting things.</p>
<ul>
<li><p>Documentation comments for functions should start with the name of the function. For encryptFile, instead of "Function to encrypt a file", you'd want to write something like "encryptFile encrypts the named file using the key". Also, it's typical to use <code>file</code> as a name for os.File variables; you might want to call the input parameter <code>path</code> or <code>name</code> instead.</p></li>
<li><p>In general, Go code doesn't have multi-line breaks inside functions or structs. <code>gofmt</code> will automatically format your code to the standard style.</p></li>
<li><p>You don't need so many comments for obvious operations. For instance, it is obvious to anyone reading your code that <code>fout, err := os.OpenFile(...)</code> opens an output file, so you don't need a comment that just says "Open output file". Rob Pike, the co-creator of Go, <a href="https://www.lysator.liu.se/c/pikestyle.html" rel="nofollow">says</a>:</p>
<blockquote>
<p>I tend to err on the side of eliminating comments, for several reasons. First, if the code is clear, and uses good type names and variable names, it should explain itself. Second, comments aren't checked by the compiler, so there is no guarantee they're right, especially after the code is modified. A misleading comment can be very confusing. Third, the issue of typography: comments clutter code.</p>
</blockquote>
<p>I tend to agree with him; I only comment hackish or tricky code and top-level variables and types.</p></li>
<li><p>Don't put helper functions in a separate file; put them near to where they are used. Putting them in a separate file just means anyone reading your code must spend time digging around to find a certain definition. Separate files by functionality instead.</p></li>
<li><p>Panicking is <em>not</em> an acceptable way to handle errors. If a function returns an error, it is meant to be interpreted (otherwise the function itself would just call <code>panic()</code>). Usually, this will just mean returning an error from the function you're writing; if an error ever reaches <code>main()</code>, the program can then <code>panic</code> (or better, print the error and exit gracefully).</p></li>
<li><p>You use <code>fmt.Println</code> many times to print the current status of the encryption. There is a package called <code>log</code> which is similar to <code>fmt</code> but is specifically designed for this sort of logging (for instance, it outputs a timestamp next to messages).</p></li>
</ul></pre>p765: <pre><p>I have modified the code taking in your comments. Thank you for the help</p></pre>ergotayours: <pre><p>No problem, I'm glad to help!</p></pre>lapingvino: <pre><p><a href="https://xkcd.com/138/" rel="nofollow">https://xkcd.com/138/</a> ;)</p></pre>xkcd_transcriber: <pre><p><a href="http://imgs.xkcd.com/comics/pointers.png" rel="nofollow">Image</a></p>
<p><a href="http://m.xkcd.com/138/" rel="nofollow">Mobile</a></p>
<p><strong>Title:</strong> Pointers</p>
<p><strong>Title-text:</strong> Every computer, at the unreachable memory address 0x-1, stores a secret. I found it, and it is that all humans ar-- SEGMENTATION FAULT.</p>
<p><a href="http://www.explainxkcd.com/wiki/index.php/138#Explanation" rel="nofollow">Comic Explanation</a></p>
<p><strong>Stats:</strong> This comic has been referenced 99 times, representing 0.1020% of referenced xkcds.</p>
<hr/>
<p><sup><a href="http://www.xkcd.com" rel="nofollow">xkcd.com</a></sup> <sup>|</sup> <sup><a href="http://www.reddit.com/r/xkcd/" rel="nofollow">xkcd sub</a></sup> <sup>|</sup> <sup><a href="http://www.reddit.com/r/xkcd_transcriber/" rel="nofollow">Problems/Bugs?</a></sup> <sup>|</sup> <sup><a href="http://xkcdref.info/statistics/" rel="nofollow">Statistics</a></sup> <sup>|</sup> <sup><a href="http://reddit.com/message/compose/?to=xkcd_transcriber&subject=ignore%20me&message=ignore%20me" rel="nofollow">Stop Replying</a></sup> <sup>|</sup> <sup><a href="http://reddit.com/message/compose/?to=xkcd_transcriber&subject=delete&message=delete%20t1_czaz9bz" rel="nofollow">Delete</a></sup></p></pre>p765: <pre><p>lol thanks</p></pre>whizack: <pre><p>Any time i see a file named 'helpers' i get scared. </p>
<p>Rightfully so, you have methods that would better be represented in a decorator type (which might even make the encrypt/decrypt code unit testable), and a method you don't even use in there (Abs).</p>
<p>You also have a bunch of magic numbers in your code. Typically you'd declare these as constants with names that make sense for what they represent.</p></pre>iio7: <pre><p>IMHO the HandleError() is really annoying. Go has such a clean way of handling errors and there is no need for the added layer of abstraction here.</p>
<p>I agree 100% with neoasterisk as the first thing I did was also to locate the HandleError() in helpers.go and then see a panic being called every time.</p>
<p>I suggest you just use the Go way and handle the errors normally.</p></pre>driusan: <pre><p>As other people pointed out, I don't think the HandleError is worthwhile. If you <em>are</em> going to panic after every error, you should defer the Close() call immediately after Open, before the HandleError, not after, to ensure that the cleanup happens even if the program panics. I also don't see any reason HandleError (or any of your other helper functions) need to be exported. Especially since it seems that functions which make more sense to export (ie. encryptFile and decryptFile) are unexported (not that it makes a big difference, since it's in a main package and not a library..)</p>
<p>The more idiomatic way to handle an error in Go is to use an initalizer in an if statement that checks the error and either return multiple values including an error, or return or a value that's obviously an error. For instance, instead of:</p>
<pre><code>func FileSize(file string) int64 {
res, err := os.Stat(file)
HandleError(err, "Stat file")
return res.Size()
}
</code></pre>
<p>you could write:</p>
<pre><code>// Returns the size of the file and an error.
func FileSize(file string) (int64, error) {
// This can't go in the initializer because we need a reference
// to res outside of the if statement
res, err := os.Stat(file);
if err != nil {
return 0, err // It would be better to return a known error type
// instead of blindly propagating it, but for
// now this illustrates how not to panic..
}
return res.Size(), nil
}
</code></pre>
<p>or better yet:</p>
<pre><code>// Returns the size of file or -1 if there's an error.
func FileSize(file string) int64 {
if res, err := os.Stat(file); err == nil {
return res.Size()
}
return -1
}
</code></pre>
<p>(edit: formatting..)</p></pre>driusan: <pre><p>And what I might about the defer is don't do this:</p>
<pre><code>// Open input file to be encrypted
fin, err := os.Open(file)
HandleError(err, "open input file")
defer fin.Close()
</code></pre>
<p>because fin.Close() won't be called if there's an error, since HandleError panics before it gets there. Do this:</p>
<pre><code>// Open input file to be encrypted
fin, err := os.Open(file)
defer fin.Close()
HandleError(err, "open input file")
</code></pre></pre>dchapes: <pre><p>This is wrong. If <code>os.Open</code> returns a non-nil error value then it's first argument is not to be used (it will be nil and <del>calling <code>Close</code> on a nil <code>*os.File</code> will panic</del>)[edit: using a <code>nil</code> or undefined result is at <em>best</em> a bad idea].</p>
<p>Unless the specific API says otherwise, you always check the error result before looking at other returns. An example of where the API says otherwise is <code>io.Reader</code> which might return something like <code>4, io.EOF</code> meaning it read and filled in four bytes (that you should deal with) and then got the mentioned error.</p></pre>driusan: <pre><p>Calling Close on a nil *os.File won't panic. Go isn't C or Java. It will return an os.ErrInvalid.</p>
<p>The implementation is <a href="https://golang.org/src/os/file_unix.go#L114" rel="nofollow">here</a>:</p>
<pre><code>func (f *File) Close() error {
if f == nil {
return ErrInvalid
}
return f.file.close()
}
</code></pre></pre>dchapes: <pre><p>True, I've edited by previous reply. However, many <code>io.Closers</code> are not as forgiving (e.g. the return from <code>net.Pipe</code>, <code>io.PipeWriter</code>, etc). It's at <em>best</em> a really really bad idea to do anything with an undefined or <code>nil</code> value when a function tells you it failed (i.e. open is telling you it didn't open the file so you know there is nothing to close).</p></pre>p765: <pre><p>Thanks. I've changed the function to the first version you mentioned. Seemed a bit better with the inclusion of error</p></pre>weberc2: <pre><p>Make sure you're handling all of your errors. On line 26 in your <code>encryptFile</code> function, you're capturing the error in a variable, but you're immediately overwriting it on the next line:</p>
<pre><code>_, err := rand.Read(iv)
// Define a new AES cipher with our generated key
block, err := aes.NewCipher(cipherKey)
</code></pre></pre>p765: <pre><p>Whoops, thanks for noticing. I've updated this</p></pre>weberc2: <pre><p><code>4<<20</code> is not <code>32000</code> (it's <code>4194304</code>). If you need a constant, it's clearer just to write 32000 rather than trying to do bit shifting.</p>
<pre><code>// If file size is greater than 32KB, make a byte buffer of 32KB
// Otherwise, create a buffer of file size
var buf []byte
if size > (4 << 20) {
buf = make([]byte, 32768)
} else {
buf = make([]byte, size)
}
</code></pre></pre>weberc2: <pre><p>Also, it looks like Go's <code>crypto/cipher</code> package has <a href="https://godoc.org/crypto/cipher#StreamReader" rel="nofollow"><code>StreamReader</code></a> (<a href="http://play.golang.org/p/uMiGSl9RQb" rel="nofollow">example</a>) and <a href="https://godoc.org/crypto/cipher#StreamWriter" rel="nofollow"><code>StreamWriter</code></a> (<a href="http://play.golang.org/p/YFlsPAY-yr" rel="nofollow">example</a>) objects that could replace your large <code>for{}</code> loop in your <code>encryptFile</code> and <code>decryptFile</code> functions.</p></pre>p765: <pre><p>I tried with the StreamReader and StreamWriter but when I time it, the for loop seems faster. Well, it was faster or encryption but the times for decryption were fairly similar</p></pre>weberc2: <pre><p>Sure, my advice was to produce more readable code. If you observed a performance difference, and your use-case justifies trading off readability and maintenance cost for performance, then absolutely roll your own version.</p>
<p>I would still recommend putting it behind a function or an object instead of inlining it in this function (this should get you performance without trading off too much readability).</p>
<p>I'm intrigued about the performance difference though. Did you try varying the size of the buffer you passed into the Write() method?</p></pre>
