Skip to main content
1 of 3
Ismael Miguel
  • 6.1k
  • 2
  • 23
  • 62

Your code is pretty clean, you implement docstrings and it seems easy enough to read.
You do have 1 extra indentation level, which I will assume that it is a copy-paste-format error.

However...

Bugs

I found 2 bugs that are easy to catch.

getpage returns strings

Here's the code in question:

# user defined functions
def getpage(query, url_type):
    """
    A helper function to create a BeautifulSoup object. Input query string and specify the url type.
    The goal is to return html code of a page.
    
    query: str input. Accepts search query by "item name" or "asin"
    
    url_type: specify the url type for the search you're doing. options = "item", "asin"
    """
    if url_type == "item":
        url = BASE_URL + "s?k=" + query
    elif url_type == "asin":
        url = BASE_URL + "product-reviews/" + query
    else:
        return "Error: url type unsupported. Choose from the following 'item', 'asin'"
        
    response = requests.get(url, headers = HEADERS)
    
    if response.status_code == 200:
        return BeautifulSoup(response.text, "html.parser")
    else:
        return "Error: status_code != 200"

The first sentence of the docstring reads:

A helper function to create a BeautifulSoup object.

This is obviously a lie! The 6th and last lines return a string.

Return a BeautifulSoup or consider throwing a "proper" exception.

But why is this bad?

This is bad because you're using the return value directly.

Here's an example taken from get_prod_name:

# [...]
html_raw = getpage(query, "item")
products = html_raw.find_all("span", class_ = "a-size-medium a-color-base a-text-normal")
# [...]

This will throw an exception because you're accessing the method find_all on a string value. This method doesn't exist.


Input handling

More like, the lack of input handling...

You expect that the string is already URL-formatted.

Yeah, you do talk about this in the docstring, but, this is error prone and the reason why I classify it as a bug.
Just imagine trying to find HB #2 pencils with your code, and having to manually encode the space and the pound symbol.

You're putting the "user" in an error-prone situation because of pure lazyness.

All you have to do is to take the input and use requests.utils.quote on it.
I found this on here: https://stackoverflow.com/questions/46783078/uri-encoding-in-python-requests-package#comment126847571_46783596

You just need to go to the functions get_prod_name and get_asin and add this as the first line:

query = requests.utils.quote(query, safe='')

But why add safe=''?

As explained in the linked comment, this function assumes that / is a safe character (as it is just the function urllib.parse.quote).

Here's an example: Example of using the function requests.utils.quote



Leaving the bugs behind, now it's time to talk about ...

Weird things

Strange string usage

Inside the function get_prod_name, you have the following:

query = query + f"&page={str(start)}"

Inside a formatted string, you don't need to use the str function, as it is implicitly converted to string.

And then, you're concatenating it, which is kinda weird too.

You can just do this:

query = f"{query}&page={start}"

It will have the same exact effect.

Repeated code

You have quite a lot of repeated code, for apparently no reason at all...

Lets analyze the code inside the get_reviews function:

# asin: list, start = 1, end = None
# [...]

if end == None:
    query = list(map(lambda x: x + f"?pageNumber={str(start)}", asin))
    html_raw = list(map(lambda x: getpage(x, "asin"), query))
    html_tag = list(map(lambda x: x.find_all("span", attrs = {"data-hook": "review-body"}), html_raw))
    html_tag = reduce(operator.iconcat, html_tag) # flatten the ResultSet
    reviews = list(map(lambda x: x.text, html_tag))
    reviews = [r.strip("\n") for r in reviews]

    return reviews

else:
    pages = range(start, end + 1)
    queries = []
            
    for p in pages:
        for id in asin:
            query = id + f"?pageNumber={str(p)}"
            queries.append(query)
    
    html_raw = list(map(lambda x: getpage(x, "asin"), queries))
    html_tag = list(map(lambda x: x.find_all("span", attrs = {"data-hook": "review-body"}), html_raw))
    html_tag = reduce(operator.iconcat, html_tag) # flattent the ResultSet
    reviews = list(map(lambda x: x.text, html_tag))
    reviews = [r.strip("\n") for r in reviews]
    
    return reviews

So, what's the difference between the code in the ifs?

I compared both, and this is the result: Result of comparing both ifs

  • The way the queries are built changes
  • The way you call getpage
  • The spelling of "flatten" (vs. "flattent")

All the other lines are REPEATED, exactly byte by byte.

You can just write this:

if end == None:
    html_raw = list(map(lambda x: getpage(f"{x}?pageNumber={start}", "asin"), asin))
else:
    pages = range(start, end + 1)
    queries = []
            
    for p in pages:
        for id in asin:
            queries.append(f"{id}?pageNumber={p}")

    html_raw = list(map(lambda x: getpage(x, "asin"), queries))

html_tag = list(map(lambda x: x.find_all("span", attrs = {"data-hook": "review-body"}), html_raw))
html_tag = reduce(operator.iconcat, html_tag) # flatten the ResultSet
reviews = list(map(lambda x: x.text, html_tag))
reviews = [r.strip("\n") for r in reviews]

return reviews

Yes, this can be better written, but this is just to give you an idea.

I also removed some (kinda) useless maps, but if you want to add them back, feel free to do so.

The strange pick of maps everywhere

On the above code, you do use the pattern list(map(lambda x: x)) a lot.

Try using list comprehensions, like this:

# old: html_raw = list(map(lambda x: getpage(x, "asin"), queries))

html_raw = [getpage(x, "asin") for x in queries]

Or, a better example:

# old: reviews = list(map(lambda x: x.text, html_tag))
#      reviews = [r.strip("\n") for r in reviews]

reviews = [x.text.strip("\n") for x in html_tag]

This should work with BeautifulSoup, as the documentation contains the following example, in the first page:

One common task is extracting all the URLs found within a page’s tags:

for link in soup.find_all('a'):
    print(link.get('href'))
# http://example.com/elsie
# http://example.com/lacie
# http://example.com/tillie

The name choices

There are some names that are extremely puzzling:

  • getpage - gives you a BeautifulSoup object or a string
  • html_raw - it's either a BeautifulSoup object or a string (no HTML here)
  • html_tag - has a list of HTML elements, not tags
  • products - it's either a list of HTML elements or a list of product names

Please, try to come up with better names.



There's a lot more to say about the code, but, I don't feel comfortable talking about those points.

I do have a limited python knowledge, and I just hope I've helped enough for others to pick and carry on from here (or even see any mistakes I've made and point them out to me as well).

Any questions, just throw them in the comments.

Ismael Miguel
  • 6.1k
  • 2
  • 23
  • 62