Ggthjy

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 elapsedn", 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.

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 elapsedn", 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!