3
\$\begingroup\$

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()
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

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>
\$\endgroup\$
1
\$\begingroup\$

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.
\$\endgroup\$
3
  • \$\begingroup\$ About 2. Even though I don't want to reconstruct any thing? I know how it is build so shouldn't it have the same error chance like when I check for a div with the id "dotd-title"? I'm basically matching in the first place. \$\endgroup\$
    – Nordiii
    Apr 1, 2017 at 12:05
  • \$\begingroup\$ Here is stated that it's sometimes appropriate to parse a limited, known set of HTML which would apply on this case or am I wrong? \$\endgroup\$
    – Nordiii
    Apr 1, 2017 at 12:16
  • 1
    \$\begingroup\$ The important bit in that answer is "This was a limited, one-time job." If that's your case, and you're going to throw this code away afterwards and never use it again, then whatever does the job works =) But since you wanted your code reviewed, and encapsulated this logic in a function, it doesn't look like it's your case. \$\endgroup\$
    – Ted
    Apr 1, 2017 at 20:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.