3
\$\begingroup\$

I am new to Go and wrote program to download and save bulk URLs concurrently. It is working correctly, but I would like to make it more efficient and follow best practices.

package main

import (...)

// Connection String
const connectionString = "connectionString"

// Declare Channels
var wg sync.WaitGroup
var ch = make(chan *item) // For success item(s)
var errItems = make(chan *item) // For error item(s)

type item struct {
    id      int
    url     string
    path    string
    content []byte
    err     error
}

func main() {
    start := time.Now()
    var basePath = os.Args[1]
    var from, to = os.Args[2], os.Args[3]
    db, err := sql.Open("sqlserver", connectionString)
    if err != nil {
        log.Fatal(err.Error())
    }
    rows, err := db.Query("SELECT [ImageID],[AccommodationlID],[Link],[FileName] FROM [dbo].[AccomodationImage] where AccommodationlID between @from and @to and FileName not like '%NoHotel.png%'", sql.Named("from", from), sql.Named("to", to))
    if err != nil {
        log.Fatal(err.Error())
    }
    defer db.Close()
    for rows.Next() {
        var item = item{}
        var accId, name string
        _ = rows.Scan(&item.id, &accId, &item.url, &name)
        item.path = fmt.Sprintf("%s\\%s\\%s", basePath, accId, name)
        wg.Add(1)
        go downloadFile(&item)
        go func() {
            select {
            case done := <-ch:
                wg.Add(1)
                go saveAndUpdateFile(db, done)
            case errorItem := <-errItems:
                wg.Add(1)
                go printResult(errorItem)
            }
        }()
    }
    wg.Wait()
    fmt.Printf("%.2fs elapsed\n", time.Since(start).Seconds())
}
func downloadFile(item *item) {
    defer wg.Done()
    resp, err := http.Get(item.url)
    if err != nil {
        item.content, item.err = nil, err
    } else if resp.StatusCode != http.StatusOK {
        item.content, item.err = nil, errors.New(resp.Status)
    } else {
        item.content, item.err = ioutil.ReadAll(resp.Body)
    }
    if item.err != nil {
        errItems <- item
    } else {
        ch <- item
    }
}
func saveAndUpdateFile(db *sql.DB, item *item) {
    defer wg.Done()
    if item.content == nil {
        item.err = errors.New("Content is empty.")
    } else {
        dir := filepath.Dir(item.path)
        err := os.MkdirAll(dir, os.ModePerm)
        if err != nil {
            item.err = err
        } else {
            item.err = ioutil.WriteFile(item.path, item.content, 0644)
        }
    }
    if item.err == nil {
        result, err := db.Exec("UPDATE [dbo].[AccomodationImage] SET IsRead = 1 WHERE ImageID = @id", sql.Named("id", item.id))
        if rows, _ := result.RowsAffected(); rows <= 0 || err != nil {
            item.err = errors.New("Update status failed.")
        }
    }
    if item.err != nil {
        errItems <- item
    }
}
func printResult(item *item) {
    defer wg.Done()
    fmt.Println(item.toString())
}

I use a select statement to implement event so that when errItems or ch channels receive, save or print items. I also surrounded select statement with goroutine to prevent blocking.

\$\endgroup\$
3
\$\begingroup\$

Full code

import (...)

Please include your full code next time. This should instead be:

import (
    "database/sql"
    "errors"
    "fmt"
    "io/ioutil"
    "log"
    "net/http"
    "os"
    "path/filepath"
    "sync"
    "time"
)

You also have item.toString(), but this is nowhere defined. I changed it to:

fmt.Println(item)

Scope

connectionString doesn't need to be in the global scope, whether it's const or not. The same could be argued about wg, ch, and errItems -- but moving those to a lower scope would involve adding more function arguments, which is a choice you can make.

In terms of security, I recommend against hard-coding the connection string in the source code. Instead, as one of many options, you can have the connection credentials as a separate file that you read.

Simplify query

You use named arguments in your query. But here, the query is so short that it's clear from the context. You can split the query across multiple lines to make it easier to read. You also have some keywords in all uppercase and some in all lowercase.

q := `
SELECT [ImageID],
       [AccommodationlID],
       [Link],
       [FileName]
FROM [dbo]. [AccomodationImage]
WHERE AccommodationlID BETWEEN ? AND ?
      AND FileName NOT LIKE '%NoHotel.png%'`

rows, err := db.Query(q, from, to)

(Notice that you also have a misspelling in AccomodationImage.)

This kind of formatting for query strings is also how the Go documentation does it.

Vertical spacing

All of the code is compressed together and has no room to breathe. I recommend adding the occasional empty lines, such as between if-statements, for loops, goroutines, etc.

Validate os.Args

Your code assumes the program will always have three arguments. While this makes sense if only you plan to use it, it's generally not good practice.

Without bounds checking, you will get a runtime panic "index out of range" -- no a particularly useful message for those running the program.

if len(os.Args) < 4 {
    log.Fatal("missing arguments")
}

You can also use short variable declaration when declaring basePath, from, and to.

basePath := os.Args[1]
from, to := os.Args[2], os.Args[3]

Default values

var item = item{}

This is redundant. You should be able to simply use var item -- please correct my if I'm wrong here and Rows.Scan() produces an error if item is nil. Since we do not need to initialize item, we can combine our declarations. Notice I rename the variable item to avoid confusion with the type item.

var (
    i     item
    accID string
    name  string
)

Conclusion

Here's the code I ended up with:

package main

import (
    "database/sql"
    "errors"
    "fmt"
    "io/ioutil"
    "log"
    "net/http"
    "os"
    "path/filepath"
    "sync"
    "time"
)

type item struct {
    id      int
    url     string
    path    string
    content []byte
    err     error
}

var wg sync.WaitGroup
var ch = make(chan *item)       // For success items
var errItems = make(chan *item) // For error items

func main() {
    const connectionString = "connectionString"

    if len(os.Args) < 4 {
        log.Fatal("missing arguments")
    }

    start := time.Now()

    basePath := os.Args[1]
    from, to := os.Args[2], os.Args[3]

    db, err := sql.Open("sqlserver", connectionString)

    if err != nil {
        log.Fatal(err.Error())
    }

    q := `
SELECT [ImageID],
       [AccommodationlID],
       [Link],
       [FileName]
FROM [dbo]. [AccomodationImage]
WHERE AccommodationlID BETWEEN ? AND ?
      AND FileName NOT LIKE '%NoHotel.png%'`

    rows, err := db.Query(q, from, to)

    if err != nil {
        log.Fatal(err.Error())
    }

    defer db.Close()

    for rows.Next() {
        var (
            i     item
            accID string
            name  string
        )

        _ = rows.Scan(&i.id, &accID, &i.url, &name)

        i.path = fmt.Sprintf("%s\\%s\\%s", basePath, accID, name)

        wg.Add(1)

        go downloadFile(&i)

        go func() {
            select {
            case done := <-ch:
                wg.Add(1)
                go saveAndUpdateFile(db, done)
            case errorItem := <-errItems:
                wg.Add(1)
                go printResult(errorItem)
            }
        }()
    }

    wg.Wait()

    fmt.Printf("%.2fs elapsed\n", time.Since(start).Seconds())
}

func downloadFile(i *item) {
    defer wg.Done()

    resp, err := http.Get(i.url)

    if err != nil {
        i.content, i.err = nil, err
    } else if resp.StatusCode != http.StatusOK {
        i.content, i.err = nil, errors.New(resp.Status)
    } else {
        i.content, i.err = ioutil.ReadAll(resp.Body)
    }

    if i.err != nil {
        errItems <- i
    } else {
        ch <- i
    }
}

func saveAndUpdateFile(db *sql.DB, i *item) {
    defer wg.Done()

    if i.content == nil {
        i.err = errors.New("Content is empty.")
    } else {
        dir := filepath.Dir(i.path)
        err := os.MkdirAll(dir, os.ModePerm)

        if err != nil {
            i.err = err
        } else {
            i.err = ioutil.WriteFile(i.path, i.content, 0644)
        }
    }

    q := `
UPDATE [dbo].[AccomodationImage]
SET    IsRead = 1
WHERE  ImageID = ?`

    if i.err == nil {
        result, err := db.Exec(q, i.id)

        if rows, _ := result.RowsAffected(); rows <= 0 || err != nil {
            i.err = errors.New("Update status failed.")
        }
    }

    if i.err != nil {
        errItems <- i
    }
}

func printResult(i *item) {
    defer wg.Done()

    fmt.Println(i)
}

Unfortunately I cannot test this code and suggest algorithmic or more in-depth changes.

Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ "Double-spacing" code so that it can "breathe" severely limits readablity. Readability is the paramount concern. \$\endgroup\$ – peterSO Mar 15 at 12:55
  • \$\begingroup\$ We don't need the full code for imports. Simply run the goimports command. \$\endgroup\$ – peterSO Mar 15 at 13:03
  • \$\begingroup\$ @peterSO And goimports is what I ran in this case. It is still invalid Go code nonetheless. In my opinion adding spacing improves readability, but this change is of little consequence. \$\endgroup\$ – esote Mar 15 at 13:40
  • 1
    \$\begingroup\$ Thank you very much @esote. your tips are very helpful. i didn't write imports because of summarization. generally my emphasis is on concurrent approach and implementing goroutines and select statement in for loop. \$\endgroup\$ – MatinMoezi Mar 15 at 20:12
2
\$\begingroup\$

A couple of generic good practices are

  • Try to avoid using global variables(You can pass wg, ch & errItems as arguments to function)
  • Single Responsibility function(A function should do only once task. Makes it easy to test.)
  • As far as possible pass dependencies as variables or using dependency injection etc.

Specific to this snippet you can:

  • Pass connectionString as either flag variables or args. Usually secrets like (db user, password) etc should not be hardcoded. It also makes it easier to run in multiple environments like test, production etc.

  • Try to break the main function into smaller functions. (like fetchRows, ProcessRows etc) Will make it testing easier and reusable.

  • saveAndUpdateFile can be made more generic. You shouldn't pass *sql.DB instead try passing a io.Reader object or just the data that needs to be written to the file.

  • Add fetchDBAccomodationFunction and updateDBAccomodation function and pass necessary information as arguments to the functions.

  • Check if rows.Next() keeps a connection open to DB. If yes you should first read all the rows, close the connection and then make http calls to download files. It is better not to keep db connections open for a long time.

  • I'm not sure if you need two separate channels for done & errorItem. Since you are passing error within item you have one channel and based on error make a decision on how to proceed.

    select {
        case item := <-ch:
        wg.Add(1)
        if item.err != nil {
            go printResult(errorItem)
        }else{
            go saveAndUpdateFile(db, done)   
        }
     }


\$\endgroup\$

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.