5
\$\begingroup\$

The following code is part of a big application. I am new to Go, and appreciate feedback on the best practices and improvements. Please consider styling and comments as well.

Every 30 minutes, this will fetch news from HackerNews and store it in a database:

What fetches the news:

package services

import (
    "fmt"
    "net/http"
    "log"
    "io/ioutil"
    "github.com/demas/cowl-go/pkg/quzx-crawler"
    "encoding/json"
    "github.com/demas/cowl-go/pkg/postgres"
    "time"
    "strconv"
)

// represent an implementation of quzx_crawler.HackerNewsService
type HackerNewsService struct {
}

const idsUrl = "https://hacker-news.firebaseio.com/v0/topstories.json?print=pretty"
const newsUrl = "https://hacker-news.firebaseio.com/v0/item/%d.json?print=pretty"
const syncInterval = 60 * 30 // 30 minutes

func (s *HackerNewsService) Fetch() {

    var lastSyncTime int64
    var err error

    lastSyncTimeStr := (&postgres.SettingsRepository{}).GetSettings("lastHackerNewsSyncTime")
    if lastSyncTimeStr == "" {
        lastSyncTime = time.Now().Unix() - syncInterval - 1
    } else {
        lastSyncTime, err = strconv.ParseInt(lastSyncTimeStr, 10, 64)
        if err != nil {
            log.Fatal(err)
        }
    }

    currentTime := time.Now().Unix()

    if lastSyncTime + syncInterval > currentTime {
        return
    }

    // get list of messages
    res, err := http.Get(idsUrl)
    if err != nil {
        log.Fatal(err)
    }

    jsn, err := ioutil.ReadAll(res.Body)
    res.Body.Close()
    if err != nil {
        log.Fatal(err)
    }

    // decode
    var ids []int64
    err = json.Unmarshal(jsn, &ids)

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

        for _, id := range ids {

            if !(&postgres.HackerNewsRepository{}).NewsExists(id) {

                // fetch each news
                log.Println("hacker news: " + fmt.Sprintf(newsUrl, id))
                res, err := http.Get(fmt.Sprintf(newsUrl, id))
                if err != nil {
                    log.Fatal(err)
                }

                jsn, err := ioutil.ReadAll(res.Body)
                res.Body.Close()
                if err != nil {
                    log.Fatal(err)
                }

                // decode
                var news quzx_crawler.HackerNews
                err = json.Unmarshal(jsn, &news)

                if err != nil {
                    log.Fatal(err)
                } else {
                    (&postgres.HackerNewsRepository{}).InsertNews(news)
                }
            }
        }
    }

    (&postgres.SettingsRepository{}).SetSettings("lastHackerNewsSyncTime",  strconv.FormatInt(currentTime, 10))
}

What stores the data:

package postgres

import (
    "log"
    "github.com/demas/cowl-go/pkg/quzx-crawler"
)

// represent a PostgreSQL implementation of quzx_crawler.HackerNewsService
type HackerNewsRepository struct {
}

func (r *HackerNewsRepository) NewsExists(id int64) bool {

    var cnt int
    err := db.Get(&cnt, "SELECT count(*) FROM HackerNews WHERE Id = $1", id)

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

    return cnt != 0
}

func (r *HackerNewsRepository) InsertNews(n quzx_crawler.HackerNews) {

    tx := db.MustBegin()

    insertQuery := `INSERT INTO HackerNews(Id, By, Score, Time, Title, Type, Url, Readed)
                VALUES($1, $2, $3, $4, $5, $6, $7, $8)`

    _, err := tx.Exec(insertQuery,
              n.Id, n.By, n.Score, n.Time, n.Title, n.Type, n.Url, 0)

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

    tx.Commit()
}
\$\endgroup\$
  • \$\begingroup\$ If this code is part of a bigger application, then why not just post the whole application for review? Don't be afraid about page size. \$\endgroup\$ – T145 Mar 18 '17 at 21:05
  • \$\begingroup\$ Here is a full application github.com/demas/quzx-crawler \$\endgroup\$ – demas Mar 19 '17 at 4:08
2
\$\begingroup\$
  1. newsUrlnewsURL, same for idsUrl.
  2. const syncInterval = 30 * time.Minute is more readable. You can also convert everything that represents a time into a time.Duration or a time.Time, rather than deal with Unix timestamps. There are functions in the time package that do what you want to do (time.Date, time.Since, etc.).
  3. About that, why does Fetch need to do complicated things with times, checking when was the last time it was called, etc.? Shouldn't the caller take care of this with a simple time.Ticker instead? It feels like this complexity shouldn't be there.
  4. HackerNewsService is a weird name for an implementation. HackerNewsFetcher maybe?
  5. Your postgres package isn't very idiomatic, which makes the callers do weird things to use it. For example, why do you have a SettingsRepository struct that doesn't contain anything? Why not simply have GetSettings as a package-level function? Same for the other empty structs of this package (HackerNewsRepository…).
  6. It's more idiomatic to test for len(myString) == 0 rather than myString == "".
  7. I'm not exactly sure what you're doing with lastSyncTime (this is pretty complex). Maybe add a comment for the line lastSyncTime = time.Now().Unix() - syncInterval - 1 or refactor this part?
  8. Talking about refactor, Fetch() is way too long. Split it in sub-functions.
  9. No need for an else after log.Fatal.
  10. Rather than:

    // fetch each news
    log.Println("hacker news: " + fmt.Sprintf(newsUrl, id))
    

    why not: log.Println("fetching from hacker news", fmt.Sprintf(newsUrl, i))

  11. Using log.Fatal everywhere makes your code not very resilient. Maybe it's worth looking at "what can fail for transient reasons" (typically, database transactions…), and re-try the thing that failed a few times with exponential backoff.
  12. There's lots of whitespace in the "what stores the data" part, which isn't very idiomatic. Empty lines in functions aren't really frequent.
\$\endgroup\$
  • \$\begingroup\$ Thank you. I have one additional question regarding 5. I have the same functions (for example GetNews) for different sites (for example, Twitter and HackerNews). I can not have two functions with the same name in one package, so I use structs to split package namespace to different 'areas'. What is the better solution in this case ? \$\endgroup\$ – demas Mar 21 '17 at 13:40
  • \$\begingroup\$ Number 7 - it is 30 minutes and 1 second ago in the unix time. Can I do it more idiomatic way ? \$\endgroup\$ – demas Mar 21 '17 at 13:43
  • 1
    \$\begingroup\$ Regarding 5: maybe have each function take an argument to specify which website you're talking about? You can use the Go equivalent of enums to represent different websites, and use a switch in the main function that calls the relevant (unexported) function for each website. \$\endgroup\$ – Ted Mar 21 '17 at 14:25
  • 1
    \$\begingroup\$ Regarding 7: time.Now() - 30*time.Minute - 1*time.Second is more idiomatic… but my point was that you shouldn't bother with these calculations at all. Why are you storing the last sync time in your SQL database? Just trust your software, and have your caller call it every 30 minutes. Using [time.Tick](,golang.org/pkg/time/#Tick) is the idiomatic way to do so in Go. \$\endgroup\$ – Ted Mar 21 '17 at 14:29
  • \$\begingroup\$ Great points, though I'd stick with initial-caps for acronyms as the rule is much easier to apply consistently. Here's a great example of why: should it be IDsURL, iDsURL or idsURL? Ugh! \$\endgroup\$ – David Harkness Mar 23 '17 at 0:23

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.