2
\$\begingroup\$

I wrote a simple daily horoscope in Python using Beautiful Soup as scraper. It works fine but the code can use some sort of a... direction if you want to put it that way. I have used underscores in my variable names which is a folly. Please point out how can I correct this and also, please tell me how I can expand this. Thanks you.

import requests
from bs4 import BeautifulSoup

def horoscope(url):
    url_ = url
    page = requests.get(url_)

    soup = BeautifulSoup(page.text, "html.parser")

    horoscope_ = soup.findAll("div", {"class": "block-horoscope-text f16 l20"}, text=True)[0].text

    return horoscope_

if __name__ == '__main__':
    rinput = input("Enter your sign: ")
    sign = str.lower(rinput)
    sign = str.strip(sign)
    url = ''

    signs = ['aries', 'taurus', 'gemini', 'cancer', 'leo', 'virgo', 'libra', 'scorpio', 'sagitarius', 'capricorn',
             'aquarius', 'pisces']

    for index, sign_ in enumerate(signs):
        if sign == sign_:
            url = "https://www.horoscope.com/us/horoscopes/general/horoscope-general-daily-today.aspx?sign={0}".format(
                index + 1
            )

print (horoscope(url))
\$\endgroup\$
  • \$\begingroup\$ It does in python3 \$\endgroup\$ – Graipher Aug 17 '16 at 11:21
  • \$\begingroup\$ Tags are important :-) \$\endgroup\$ – Mast Aug 17 '16 at 12:55
  • 1
    \$\begingroup\$ Very bad, none of the things it told me actualy happened. Needs much improvement. \$\endgroup\$ – Tomáš Zato Aug 17 '16 at 13:24
  • \$\begingroup\$ Be sure not to commit this grave sin... stackoverflow.com/a/1732454/5167161 \$\endgroup\$ – Rohan Jhunjhunwala Aug 17 '16 at 17:14
4
\$\begingroup\$

I don't see an underscore variable name that is needed. I don't understand why you did url_ = url in horoscope(). Why not just use url directly? The only time you would want to do a straight var1 = var2 is for when one of them will be assigned to something else, and you want to remember the original value. In this case, neither one changes, so they are still both the original by the end of the function. I also don't see why you said horoscope_ = ... instead of just horoscope = ... I think it may be because the function itself is named horoscope, but why should that make a difference? Yes, conceivably you might have recursion later that would be surprised when horoscope means something else, but ... wait, why would you have recursion? Since you won't, it doesn't change anything inside the function to have the same name, and since it isn't a global variable, it doesn't effect anything else. As for sign_, I'll get to that later.


I am happy to see you using if __name__ == "__main__":. That is an excellent piece of code that many people leave out. I do wonder one thing, though: why is the print(...) outside of it? Since url isn't defined unless the if __name__ == "__main__": block executes, you will have an error if you try to import this module.


The way you use str.lower(...) and str.strip(...) is definitely not conventional. You very rarely need to access instance methods by using the class method with the instance as the first argument. Just use it like an instance method:

sign = rinput.lower()
sign = sign.strip()

I would also suggest merging them:

sign = rinput.strip().lower()

rinput seems to be a strange name for user input. I would do something more like this:

sign = input("Enter your sign: ").strip().lower()

There really isn't a reason I can see to create more than one variable.


You first define url as blank. I'm not sure why you do that, because a blank url causes a requests.exceptions.MissingSchema error in horoscope, whereas no url at all causes a NameError. As far as I can tell, it doesn't make a difference to the user which error is shown. I believe the error shouldn't be shown. You should have error checking, and ask the user again for input. (More later)


You don't need to use a for loop to get the index. The list class already has a very handy method for it: .index(). I would suggest that you use it:

sign = signs.index(sign) + 1
url = "https://www.horoscope.com/us/horoscopes/general/horoscope-general-daily-today.aspx?sign={0}".format(sign)

This will throw an IndexError if the user types the wrong thing, so I would do something like this:

sign = ''
while True:
    sign = input("Enter your sign: ").lower.strip()
    if sign in signs:
        break
    print("That sign is not available.  The available signs are:")
    print("\n".join(signs)

That way, you make sure that the sign is valid before you try to get an index.

The requests.get function cat take a params argument for giving variables in the url. I would suggest that you use that instead of putting the variables right into the url. To do that, horoscope could be defined like this:

def horoscope(sign):
    url = "https://www.horoscope.com/us/horoscopes/general/horoscope-general-daily-today.aspx"
    page = requests.get(url, params={'sign': sign})

You can then call it with horoscope(sign) (where sign is the index + 1).

\$\endgroup\$
3
\$\begingroup\$

PEP8, the official python styleguide recommends a maximum linewidth of 80 characters for readability. For the url, which is longer than that, you could use line continuation (\ at the end of the line), but I personally prefer to keep the url unbroken. In all other places intermediate variables can help to shorten the lines.

You should not have too much code under the if __name__ == "__main__": gaurd, rather put it into a main() method and just call that.

Both the base url and the signs are constants of your code, so I put them to the beginning (in CAPITAL_LETTERS, as, again, recommended by PEP8).

Python already has a function implemented to find the (first) index at which a given value appears in a list, list.index. Use that.

You should always validate user input. Your code fails with a cryptic error, if the user enters foo as their sign, for example. Or if he just makes a typo and inputs gemeni. Incidentally, you have a typo in sagitarius. Correct is sagittarius.

You should also poll the user until he submits a valid choice and tell him what his choices are.

You should probably strip whitespace from the end of the text you retrieve. It is indented quite a lot.

import requests
from bs4 import BeautifulSoup

SIGNS = ['aries', 'taurus', 'gemini', 'cancer', 'leo', 'virgo', 'libra',
     'scorpio', 'sagitarius', 'capricorn', 'aquarius', 'pisces']
URL = "https://www.horoscope.com/us/horoscopes/general/horoscope-general-daily-today.aspx?sign={0}"


def horoscope(url):
    soup = BeautifulSoup(requests.get(url).text, "html.parser")
    selector = {"class": "block-horoscope-text f16 l20"}
    divs = soup.findAll("div", selector, text=True)
    return divs[0].text.strip()


def main():
    print("The signs are: {}".format(" ".join(SIGNS)))
    sign = ""
    while sign not in SIGNS:
        sign = input("Enter your sign: ").lower().strip()
    url = URL.format(SIGNS.index(sign) + 1)
    print("Your horoscope for {}:". format(sign))
    print(horoscope(url))

if __name__ == "__main__":
    main()
\$\endgroup\$

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.