2
\$\begingroup\$

Can someone take a look if this code piece is ok. I'm most interested whether the goroutines are used correctly, but also general Go best practices.

package main

import (
    "fmt"
    "io/ioutil"
    "os"
    "path/filepath"
    "regexp"
)

type Endings struct {
    crlf, lf uint
}

func check(e error) {
    if e != nil {
        panic(e)
    }
}

func getFileList(searchDir string) []string {
    fileList := []string{}
    filepath.Walk(searchDir, func(path string, f os.FileInfo, err error) error {
        if !f.IsDir() {
            fileList = append(fileList, path)
        }
        return nil
    })

    return fileList
}

func getFileEndings(filename string, c chan Endings) {
    fileContent, err := ioutil.ReadFile(filename)
    check(err)
    c <- countEndings(string(fileContent))
}

func countEndings(s string) Endings {
    crlf := regexp.MustCompile("\r\n")
    lf := regexp.MustCompile("\n")

    x := len(crlf.FindAllStringIndex(s, -1))
    y := len(lf.FindAllStringIndex(s, -1))

    return Endings{uint(x), uint(y - x)}
}

func splitIntoChunks(arr []string, chunkSize int) [][]string {
    if chunkSize <= 0 {
        panic("chunk size too small")
    }

    if len(arr) <= chunkSize {
        return [][]string{arr}
    }

    numChunks := int(len(arr)/chunkSize) + 1
    chunks := make([][]string, numChunks)

    for i := 0; i < numChunks; i++ {
        l := i * chunkSize
        u := l + chunkSize

        if u > len(arr) {
            u = len(arr)
        }

        chunks[i] = arr[l:u]
    }

    return chunks
}

func main() {
    searchDir := os.Args[1]
    c := make(chan Endings)
    chunkSize := 1000

    fileList := getFileList(searchDir)

    count := Endings{0, 0}
    for _, chunk := range splitIntoChunks(fileList, chunkSize) {
        for _, file := range chunk {
            go getFileEndings(file, c)
        }

        for _ = range chunk {
            result := <-c

            count.crlf += result.crlf
            count.lf += result.lf
        }
    }

    fmt.Printf("crlf: %d, lf: %d\n", count.crlf, count.lf)
}
\$\endgroup\$
0
1
\$\begingroup\$

Your program works fine and you are correctly using goroutines, but the code isn't very idiomatic.

Producer/Consumer architecture

What we have here is a typical producer/consumer scenario. The producer is filepath.Walk() wich returns a list of files, and the consumers are goroutines executing regex on file content.

So instead of the splitIntoChunks() method, we could just have a chan string: the producer sends file names to the channel, and the consumers loop over it and parse the file content.

This have two main advantages:

  • code is easier to read
  • producer and consumer can work in parallel

So the code would looks like

results := make(chan Endings, 2) // chan to get the results of each worker
var wg sync.WaitGroup
wg.Add(2)
files := make(chan string, 1000) // channel containing file list
for i := 0; i < 2; i++ {
    go run(files, results, &wg) // start the producers 
}
filepath.Walk(searchDir, func(path string, f os.FileInfo, err error) error {
    if !f.IsDir() {
        files <- path // feed the channel
    }
    return nil
})
close(files)
wg.Wait()
close(results)

Work on []byte rather than on string

This is a general advice regarding performances: always prefer working on []byte instead of working on string to avoid extra allocations. The regex package has methods to work on string or on byte slice, so instead of

fileContent, err := ioutil.ReadFile(filename)
c <- countEndings(string(fileContent)) 
...
x := len(crlf.FindAllStringIndex(s, -1))

we can just use

fileContent, err := ioutil.ReadFile(fileName)
...
x := len(crlf.FindAllIndex(fileContent, -1))

Don't panic

The code shouldn't panic on every error. For example, if the user running the program don't have the permission to read a file in searchDir, the program should not crash but rather log the error

so avoid method like this:

func check(e error) {
    if e != nil {
        panic(e)
    }
}

instead handle the error localy:

fileContent, err := ioutil.ReadFile(fileName)
if err != nil {
    fmt.Printf("fail to read file %s: %v\n", fileName, err)
}

Other consideration

It's better to define regex as global var:

var (
    crlf = regexp.MustCompile("\r\n")
    lf   = regexp.MustCompile("\n")
)

It's also better to use the flag package to parse arguments instead of relying on os.Args

Here is the final version of the code:

package main

import (
    "flag"
    "fmt"
    "io/ioutil"
    "os"
    "path/filepath"
    "regexp"
    "sync"
)

var (
    crlf = regexp.MustCompile("\r\n")
    lf   = regexp.MustCompile("\n")
)

type Endings struct {
    crlf, lf uint
}

func (e *Endings) countEndings(fileName string) {
    c, err := ioutil.ReadFile(fileName)
    if err != nil {
        fmt.Printf("fail to read file %s: %v\n", fileName, err)
    }
    x := len(crlf.FindAllIndex(c, -1))
    y := len(lf.FindAllIndex(c, -1))

    e.crlf += uint(x)
    e.lf += uint(y - x)
}

func run(files <-chan string, results chan<- Endings, wg *sync.WaitGroup) {
    e := &Endings{}
    for f := range files {
        e.countEndings(f)
    }
    results <- *e
    wg.Done()
}

func main() {

    searchDir := flag.String("dir", "dir", "directory to search")
    nWorker := flag.Int("n", 2, "number of worker")
    flag.Parse()

    results := make(chan Endings, *nWorker)

    var wg sync.WaitGroup
    wg.Add(*nWorker)
    files := make(chan string, 1000)
    for i := 0; i < *nWorker; i++ {
        go run(files, results, &wg)
    }
    filepath.Walk(*searchDir, func(path string, f os.FileInfo, err error) error {
        if !f.IsDir() {
            files <- path
        }
        return nil
    })
    close(files)
    wg.Wait()
    close(results)

    count := &Endings{}
    for e := range results {
        count.crlf += e.crlf
        count.lf += e.lf
    }
    fmt.Printf("crlf: %d, lf: %d\n", count.crlf, count.lf)
}

Performance

The new code is slightly faster:

old:

$ time ./test ~/go/src/golang.org          
crlf: 6231, lf: 1589462
./test ~/go/src/golang.org  1,78s user 0,23s system 167% cpu 1,201 total

new:

$ time ./test -dir ~/go/src/golang.org -n 6 
crlf: 6231, lf: 1589462
./test -dir ~/go/src/golang.org -n 6  1,48s user 0,18s system 181% cpu 0,911 total
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.