3 ms·

Thanks for the feedback. Yeah the Python part is definitely a bit hackish. Do you have any other recommendations on other problems? I'm still learning Golang an

Thanks for the feedback. Yeah the Python part is definitely a bit hackish. Do you have any other recommendations on other problems? I'm still learning Golang and would like to improve. Thanks!

TheDong 6y ago
It took me a few to respond, but I'll make some recommendations now: 1. Use filepath.Glob if you want to glob Most of [0] can be replaced with filepath.Glob. You should probably also respect TMPDIR if it's set, which ioutil.TempDir etc will do. 2. ExitErr shouldn't be used, especially not within nested functions. You have 'defer os.RemoveAll(tmpDir)'. defers won't get called if you `os.Exit(1)` as you do in ExitErr. You should be instead doing `return nil, err` all the way back up to `err := RunApp()`, and printing the error there. That will let defers run. In general, using os.Exit anywhere other than in the main function is an antipattern. 3. Your parallel processing should be using a waitgroup or errgroup. A slice of channels is super fragile. In [1], do something like the following: // before the for loop var wg sync.WaitGroup // in each iteration of the for loop wg.Add(1) go func(name string) { defer wg.Done() cli.ImageRoutine(fileName) }(fileName) // at the end, to wait wg.Wait() Use an errgroup if you need to cancel it. The channel thing you're doing now is more fragile than a waitgroup, and harder to reason about. 4. use of exported functions / unexported functions is all over the place and inconsistent. There's a few other things, but that's what I've got offhand. I can't actually run the code because I only use linux, and it obv doesn't run on linux. Oh, I guess that's another thing, you want some build tags to make it more clear it won't run on linux. Hopefully something in there was helpful [0]: https://github.com/rootVIII/pdfinverter/blob/5fe9f505779bb9d75b8b3edc879c18065bfd0a3c/utils.go#L53-L62 https://github.com/rootVIII/pdfinverter/blob/5fe9f505779bb9d... [1]: https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc72545142247c859073fd02e952e73b10/cli.go#L19-L20 https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc7254514...
- rootVIII 6y ago
  okay have some of these fixes in progress. Thanks again for taking the time to look at my code! Very appreciative
- rootVIII 6y ago
  Hi TheDong. I put most of the changes you recommended and I am much happier with the overall project. I decided to remove the Python part though and went back to the Imagick bindings. There's probably still some things to fix but thanks again for taking the time to look and advise.

Thanks for the feedback. Yeah the Python part is definitely a bit hackish. Do you have any other recommendations on other problems? I'm still learning Golang and would like to improve. Thanks!

TheDong 6y ago

It took me a few to respond, but I'll make some recommendations now: 1. Use filepath.Glob if you want to glob Most of [0] can be replaced with filepath.Glob. You should probably also respect TMPDIR if it's set, which ioutil.TempDir etc will do. 2. ExitErr shouldn't be used, especially not within nested functions. You have 'defer os.RemoveAll(tmpDir)'. defers won't get called if you `os.Exit(1)` as you do in ExitErr. You should be instead doing `return nil, err` all the way back up to `err := RunApp()`, and printing the error there. That will let defers run. In general, using os.Exit anywhere other than in the main function is an antipattern. 3. Your parallel processing should be using a waitgroup or errgroup. A slice of channels is super fragile. In [1], do something like the following: // before the for loop var wg sync.WaitGroup // in each iteration of the for loop wg.Add(1) go func(name string) { defer wg.Done() cli.ImageRoutine(fileName) }(fileName) // at the end, to wait wg.Wait() Use an errgroup if you need to cancel it. The channel thing you're doing now is more fragile than a waitgroup, and harder to reason about. 4. use of exported functions / unexported functions is all over the place and inconsistent. There's a few other things, but that's what I've got offhand. I can't actually run the code because I only use linux, and it obv doesn't run on linux. Oh, I guess that's another thing, you want some build tags to make it more clear it won't run on linux. Hopefully something in there was helpful [0]: https://github.com/rootVIII/pdfinverter/blob/5fe9f505779bb9d75b8b3edc879c18065bfd0a3c/utils.go#L53-L62 https://github.com/rootVIII/pdfinverter/blob/5fe9f505779bb9d... [1]: https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc72545142247c859073fd02e952e73b10/cli.go#L19-L20 https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc7254514...

rootVIII 6y ago
okay have some of these fixes in progress. Thanks again for taking the time to look at my code! Very appreciative
rootVIII 6y ago
Hi TheDong. I put most of the changes you recommended and I am much happier with the overall project. I decided to remove the Python part though and went back to the Imagick bindings. There's probably still some things to fix but thanks again for taking the time to look and advise.