Scraping lunar/solar eclipse data with BeautifulSoup Followup

Question

I decided to refactor my code based on this review.

Eclipse:

class Eclipse():

    def __init__(self, eclipse_date, eclipse_type, eclipse_location):
        self._validate_str_parameters(eclipse_date, "Eclipse date")
        self._validate_str_parameters(eclipse_type, "Eclipse type")
        self._validate_lst_parameters(eclipse_location, "Location")

        self._eclipse_date = eclipse_date
        self._eclipse_type = eclipse_type
        self._eclipse_location = eclipse_location


    def _validate_str_parameters(self, str_to_validate, parameter_type):
       if not isinstance(str_to_validate, str):
           raise TypeError("{0} is the incorrect type".format(parameter_type))
       if not str_to_validate:
           raise ValueError("{0} is empty".format(parameter_type))     

    def _validate_lst_parameters(self, lst_to_validate, parameter_type):
        if not isinstance(lst_to_validate, list):
            raise TypeError("{0} type is incorrect".format(parameter_type))
        if len(lst_to_validate) == 0:
            raise ValueError("{0} is invalid. You must have at least one.".format(parameter_type))

    @property
    def get_eclipse_date(self):
        return self._eclipse_date

    @property
    def get_eclipse_type(self):
        return self._eclipse_type

    @property
    def get_eclipse_location(self):
        return self._eclipse_location

    def date_and_type(self):
        return "{0}\n{1}\n".format(self._eclipse_date, self._eclipse_type)

    def month_and_year_is(self, eclipse_month, eclipse_year):
        self._validate_str_parameters(eclipse_month, "Month")
        self._validate_str_parameters(eclipse_year, "Year")
        return eclipse_month and eclipse_year in self._eclipse_date

    def takesplacein(self, country):
       self._validate_str_parameters(country, "Country")

       country_formatted = country.strip().title()
       muchof = "Much of {0}".format(country_formatted)
       return self._is_muchof_in_list(muchof) or self._is_country_in_list(country_formatted)

    def _is_muchof_in_list(self, muchofcountry):
        return muchofcountry in self._eclipse_location

    def _is_country_in_list(self, countryformatted):
        return countryformatted in self._eclipse_location

    def __str__(self):
        return "Date: {0}\nType: {1}\nLocation: {2}\n" .format(self._eclipse_date,self._eclipse_type,self._eclipse_location)

Explanation: I created this class to create objects based on the data I scraped from this eclipse site.

Example Data after scraping:

Date: July 13, 2018 
Type: Partial Solar Eclipse
Location: ['South in Australia', ' Pacific', ' Indian Ocean']

Date: July 27, 2018 
Type: Total Lunar Eclipse
Location: ['Much of Europe', ' Much of Asia', ' Australia', ' Africa', ' South in North America', ' South America', ' Pacific', ' Atlantic', ' Indian Ocean', ' Antarctica']

The method takesplacein(self, country) takes the country (for example, europe) and using muchof = "Much of {0}".format(country_formatted), creates the phrase "Much of Europe."

The site uses that phrase when not all of the country will see it, without testing for "Much of insert country here" we would just test the country name, but it will return False, even though parts of the country will see it.

The method tests for both "Much of insert country here" or the Country itself, because there can only be one occurrence of the country. It will never say Europe and Much of Europe.

Eclipse concerns:

1. Are the method names takesplacein(self, country) and month_and_year_is(self, month, year) considered clean? I personally like the names because then I can write code that almost sounds like a sentence, for example:

   if eclipse.takesplacein("europe"):
       print(eclipse.date_and_type())
   
   if eclipse.month_and_year_is("July", "2018"):
       print(eclipse.date_and_type())
   

2. Is the use of isinstance() justified here? I'm aware the use of that method or keyword is considered code smell, but without it, I can pass a string as the location when the code uses location as a list

Main:

import requests
from bs4 import BeautifulSoup
from Eclipse import Eclipse
from collections import namedtuple


def generate_eclipse_data(eclipse_info):

    eclipse_list = []

    for info in eclipse_info:
        retrieved_date_and_type = parse_date_and_type(info.find('a').text)
        retrieved_location_data = parse_location(info.find('p').text)
        eclipse_data = Eclipse(retrieved_date_and_type.date,retrieved_date_and_type.type, retrieved_location_data.location)
        eclipse_list.append(eclipse_data)
    return eclipse_list

def parse_location(info_to_clean):
    clean_location = namedtuple("Clean_Location",["location"])
    cleanup_location_and_new_features = info_to_clean.split(" New Features")
    cleanup_location, *rest, new_features = cleanup_location_and_new_features
    location_list = cleanup_location.split(",")
    location_data = clean_location(location_list)
    return location_data

def parse_date_and_type(info_to_split):
    date_and_type = namedtuple("Date_Type", ["date", "type"])
    eclipse_split_data = info_to_split.split("—")
    eclipse_date, *rest, eclipse_type = eclipse_split_data
    eclipse_data = date_and_type(eclipse_date,eclipse_type.strip())
    return eclipse_data

def main():

    eclipse_url = "https://www.timeanddate.com/eclipse/"
    eclipse_time_date = requests.get(eclipse_url)
    soup = BeautifulSoup(eclipse_time_date.text, 'html.parser')
    eclipse_info = soup.find_all("div", class_= "six columns art__eclipse-txt")
    eclipse_filename = 'eclipse.txt'

    if (len(eclipse_info) > 0):
        try:
            print("URL: {0}\n".format(eclipse_url))
            eclipse_data =  generate_eclipse_data(eclipse_info)

            for eclipse in eclipse_data:
                if eclipse.takesplacein("europe"):
                    print(eclipse.date_and_type())

            for eclipse in eclipse_data:
                if eclipse.month_and_year_is("July", "2018"):
                    print(eclipse.date_and_type())
        except ValueError as error:
            print(error)
        except TypeError as error:
            print(error)
        except Exception as error:
            print(error)
    else:
        print("No data available")

if __name__ == "__main__":
    main()

def parse_location(info_to_clean): constructs a list and stores it in a namedtuple.

Any ways I can improve it further?

Answer 1

Is the use of isinstance() justified?

Part of what you are trying to validate with the isinstance() is the Date strings. I would suggest that this whole thing would be better done by converting to and using datetime.date objects. This would mean the validation could come when the conversion is done. That, at least for the date, would remove the need for the isinstance().

Are the method names takesplacein and month_and_year_is clean?

I would suggest takesplacein would read better as takes_place_in and month_and_year_is would read better is_month_and_year. But to your larger point, I also prefer these sorts of names because as you note they allow the code to read better.

Properties don't really need get_

These properties are named with get_ prefix. In the case of properties, the fact that it returns something is reasonably obvious, so get_ doesn't really provide more information. So:

@property
def get_eclipse_date(self):
    return self._eclipse_date

@property
def get_eclipse_type(self):
    return self._eclipse_type
    

Would be a bit cleaner as simply:

@property
def eclipse_date(self):
    return self._eclipse_date

@property
def eclipse_type(self):
    return self._eclipse_type        
    

additionally, I would consider make this a property also:

def date_and_type(self):
    return "{0}\n{1}\n".format(self._eclipse_date, self._eclipse_type)

Answer 2

This review is only for the code in Main. A few style suggestions, in no particular order:

Extra Intermediate Variables

Separate variable names for intermediate products are useful in a few cases, eg:

1. The name of the variable is important to document what the intermediate product represents.
2. Need to break a very complex expression down into smaller pieces.
3. The intermediate product is used in more than one place.

So, I would suggest that this:

cleanup_location_and_new_features = info_to_clean.split("? New Features")
cleanup_location, *rest, new_features = cleanup_location_and_new_features
location_list = cleanup_location.split(",")
location_data = clean_location(location_list)

is actually easier to read as:

cleanup_location, *rest, new_features = info_to_clean.split("? New Features")
location_data = clean_location(cleanup_location.split(","))

I don't think the two intermediate variable names provide any more clarity or functionality, but they do double the code line count.

Multiple Exception Types:

This:

except ValueError as error:
    print(error)
except TypeError as error:
    print(error)
except Exception as error:
    print(error)
    

Can be reduced to:

except (ValueError, TypeError, Exception) as error:
    print(error)
    

But then since ValueError and TypeError are both based on Exception it could be further reduced to:

except Exception as error:
    print(error)
    

But catching all exceptions is considered bad practice, so you may want to consider what functionality you really want here..

Remove Redundant Parenthesis:

if (len(eclipse_info) > 0):
    

Can simply be:

if len(eclipse_info) > 0:

anything > 0 will evaluate to True so you can just do:

if len(eclipse_info):
    

And finally many types evaluate to True if their len is > 0, and for those types you can simply do:

if eclipse_info:

PEP8

You should consider formatting your code in accordance with pep8. This is important when sharing code, as the consistent style makes it much easier for other programmers to read your code. There are various tools available to assist in making the code pep8 compliant. I use the PyCharm IDE which will show pep8 violations right in the editor.