Extract title with regex

Question

I'm quite new to Go and I feel like this code could be smaller and cleaner.

I would love any suggestions and/or hints about mistakes and conventional go things!

func getBookTitle(client *http.Client) {
    rsp, err := client.Get(bookSite)
    if err != nil {
        panic(err)
    }
    html, _ := ioutil.ReadAll(rsp.Body)

    //Get div with title
    regTitle := regexp.MustCompile("(<div class=\"dotd-title\">)[\n+\\s]*(<h2>)[a-zA-Z–\\-\n\\s:]*(</h2>)[\n+\\sdd]*(</div>)")
    //remove linebreaks regex
    regFormatTitle := regexp.MustCompile("[\r\n]*")
    //apply regex
    title := regFormatTitle.ReplaceAllString(string(regTitle.Find(html)),"")
    //Remove remove html tags and remove whitespaces
    title =  strings.TrimSpace(title[strings.Index(title,"<h2>")+len("<h2>"):strings.Index(title,"</h2>")])

    fmt.Printf("Book title:%s\n",title)

    rsp.Body.Close()
}

Answer 1

Go practices

It's recommended to close things using defer as early as possible, so that you won't forget them later. For example you have:

html, _ := ioutil.ReadAll(rsp.Body)

// some code

defer rsp.Body.Close()

Using defer, you should write like this instead:

html, _ := ioutil.ReadAll(rsp.Body)
defer rsp.Body.Close()

// some code

---

Another thing, it's recommended to not ignore errors. This example ignores the err value returned by ioutil.ReadAll, and that may lead to crashes later in the program.

Single responsibility principle

It's recommended for functions to do just one thing. The getBookTitle function does do many things:

• Download a web page from bookSite (a free variable defined elsewhere)
• Read content from a stream
• Extract title from an HTML string

The name implies only the 3rd task. It would be better if it did only that, and the other tasks should be performed in other functions.

Strange regex

This regex looks strange:

(<div class=\"dotd-title\">)[\n+\\s]*(<h2>)[a-zA-Z–\\-\n\\s:]*(</h2>)[\n+\\sdd]*(</div>)

For several reasons:

• [\n+\\s] is odd. \\s already includes \n. You could write simpler as [+\\s]. But are you really expecting a literal + between the starting <div ...> and <h2>? I seriously doubt it. In which case the expression could be reduced to just \\s.

• [\n+\\sdd] is similarly odd. Again, the \n is unnecessary. Two d are also unnecessary, and if there are literal d between </h2> and </div>, that would be invalid HTML.

• Why the groupings with (...)? Those parentheses don't serve any purpose in this program.

All in all, it seems the regex is mistakenly over-complicated, this simpler regex is probably good enough and much more clear:

<div class=\"dotd-title\">\\s*<h2>[a-zA-Z–\\s:]*</h2>\\s*</div>

Answer 2

janos gave a good answer. I would add one minor thing and one major thing:

1. Minor: don't panic. This is encapsulated in a function (that, indeed, does too many things) so the function should return an error if something bad is likely to happen; and you should let the caller deal with the error. Furthermore, log.Exitf is almost always what you want rather than panic.
2. Major: don't use a regexp to parse HTML. Seriously, don't. Consider using the html Go library instead.