Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimizations #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

dominikbraun
Copy link

@dominikbraun dominikbraun commented Aug 3, 2019

This PR contains:

  • optimizations regarding code style
  • optimizations regarding concurrency style
  • optimizations regarding performance

@@ -17,97 +18,97 @@ func main() {
directory := flag.String("directory", "", "the absolute directorypath to search for duplicates")
flag.Parse()

fmt.Println("DupsFindr started with: ", *directory)
if len(*directory) == 0 {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the length and then panicking increases readability and reduces indentations.

wg.Add(1)
// recieve from files and add to slice
go readFiles(files, &allFilesWithoutDuplicates, &counter, &wg)
counter := 0
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason counter is created but never used in this function?

// is this okay, or should it be closed after the last directory was read?
close(files)
wg.Wait()
// is this okay, or should it be closed after the last directory was read?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is best practice to call close() only from the sending goroutine, but that won't work here since there are multiple sending goroutines.

}

func readDirectory(directory string, files chan<- string, wg *sync.WaitGroup) {
wg.Add(1)
Copy link
Author

@dominikbraun dominikbraun Aug 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved Add() into the function so that a call to readDirectory() can be used without having to call Add() yourself.

}

info, err := f.Stat()
defer f.Close()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defering the call to Close() only if err == nil

}

for _, directoryInfo := range directoryInfos {
path := fmt.Sprintf("%s/%s", directory, directoryInfo.Name())
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introducing a new variable that holds the path since it will be used two times. It is common practice to use fmt.Sprintf instead of classical string concatenation.

go readDirectory(*directory, files, &wg)
var wg sync.WaitGroup
// read the directory and send all files to the channel
go readDirectory(*directory, files, &wg)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Könnte man optimieren auch wenns für den Regelfall wohl nicht relevant ist.

@dominikbraun
Copy link
Author

Wunderbar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant