2
\$\begingroup\$

I'm currently writing a little tool to get into Go. As I'm not familiar with the language I'm especially looking for

  1. Conventional go stuff.
  2. utility.go feels wrong.Should I wrap the client and email/password into a struct in utility? What would you advice.

Every critique on anything is very well welcome!

Small info:

  • 2 Files
  • main.go and utility.go
  • utility.go has it's own package

main.go

package main

import (
    w "./webactions"
    "flag"
    "fmt"
    "net/http"
    "net/http/cookiejar"
    "os"
)

func main() {
    if len(os.Args) < 3 {
        fmt.Println("Not enough arguments provided")
        return
    }

    format := flag.String("dl", "", "Set download format")
    flag.Parse()

    cookie, _ := cookiejar.New(nil)
    client := &http.Client{Jar: cookie}

    if w.Login(client, flag.Args()[0], flag.Args()[1]) {
        fmt.Println("Loggin sucessfull")
        if *format != "" {
            if w.DownloadBooks(client, *format) {
                fmt.Println("Downloaded all new Books as", *format)
            } else {
                fmt.Println("Failed to download any books")
            }
        }
        if w.Logout(client) {
            fmt.Println("Logout sucessfull")
        }

    }

}

utility.go

package webactions

type book struct {
    nid         string
    title       string
    format      string
    downloadURL string
}

/*
    Utility function for this file
*/
func getHTMLBytes(client *http.Client, urlString string) []byte {
    rsp, err := client.Get(urlString)
    if err != nil {
        panic(err)
        return nil
    }
    defer rsp.Body.Close()
    htmlBytes, err := ioutil.ReadAll(rsp.Body)
    if err != nil {
        panic(err)
        return nil
    }
    return htmlBytes
}

func Login(client *http.Client, email string, password string) bool {
    form := url.Values{
        "op":       {"Login"},
        "email":    {email},
        "password": {password},
        "form_id":  {"user_login_form"},
    }
    rsp, err := client.PostForm(requestUrl, form)
    defer rsp.Body.Close()
    if err != nil {
        panic(err)
        return false
    }
    return rsp.Request.URL.String() == requestUrl+"index"
}

func Logout(client *http.Client) bool {
    _, err := client.Get(requestUrl + "logout")
    if err != nil {
        panic(err)
        return false
    }
    return true
}

func DownloadBooks(client *http.Client, format string) bool {
    htmlBytes := getHTMLBytes(client, myBooksURL)
    if htmlBytes == nil {
        return false
    }

    var wg sync.WaitGroup
    //Every book has a div like <div nid="" title="">
    books := regexp.MustCompile("nid=\"[0-9]+\" title=\"[\\w:\\[\\]'\\-\\s]+\"")
    //Create dir
    os.MkdirAll("BookDownloads/"+format+"/", 0666)
    for _, e := range books.FindAllString(string(htmlBytes), -1) {
        title := regexp.MustCompile("[\\w:\\[\\]'\\-\\s]+\\[eBook\\]")
        nid := regexp.MustCompile("[0-9]+")

        wg.Add(1)
        go downloadBookJob(
            client,
            &wg,
            &book{
                nid.FindString(e),
                title.FindString(e),
                format,
                requestUrl + "ebook_download/" + nid.FindString(e) + "/" + format,
            },
        )
    }
    wg.Wait()
    return true
}

func downloadBookJob(client *http.Client, wg *sync.WaitGroup, book *book) {
//out = *file   
if out := prepareDownload(book); out != nil {
        defer out.Close()

        if !downloadBook(client, book.downloadURL, out) {
            os.Remove(out.Name())
            wg.Done()
            return
        }
        fmt.Println("Downloaded:", book.title)
    }
    wg.Done()
}

func prepareDownload(book *book) *os.File {

    reg := regexp.MustCompile("[/:;?*<>|]")
    fileName := "BookDownloads/" + book.format + "/" + reg.ReplaceAllString(book.title, "_") + "." + book.format

    out, err := os.OpenFile(fileName, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
    if os.IsExist(err) {
        return nil
    }
    return out
}

func downloadBook(client *http.Client, bookDownloadURL string, file *os.File) bool {

    rsp, err := client.Get(bookDownloadURL)
    defer rsp.Body.Close()
    if err != nil {
        fmt.Println(bookDownloadURL)
        panic(err)
    }
    if rsp.Header.Get("Content-Length") == "0" {
        return false
    }

    _, err = io.Copy(file, rsp.Body)
    if err != nil {
        fmt.Println("Error while downloading", bookDownloadURL, "-", err)
        return false
    }
    return true

}
\$\endgroup\$
1
\$\begingroup\$

A few suggestions:

  1. Relative Imports:

    • From personal experience, relative imports are a pain to work with. There is a very good discussion around this here. In short, as posted by Andrew Gerrand, the only reason the go tool supports relative imports is to run the compiler tests, which predate the go tool and the concept of GOPATH.
  2. Do not Panic:

    • Return an appropriate error instead of panicking so that caller can handle the error gracefully. The idiomatic way of returning an error is the last return value. I would suggest reading effective go.
  3. Always check errors:

    • Almost all the functions return an error. Check those before proceeding. You have deferred close of response body before checking the error which can lead to a panic if the HTTP request fails.
\$\endgroup\$
  • \$\begingroup\$ This provides some decent feed back, but it doesn't explain why. Is it possible to add explanations? \$\endgroup\$ – pacmaninbw Jun 7 '17 at 13:03

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.