7
\$\begingroup\$

I created a script to scrape this website and gather the eclipse data (date, type, and location):

class Eclipse():

    def __init__(self, eclipse_date, eclipse_type, eclipse_location):
        self._eclipse_date = eclipse_date
        self._eclipse_type = eclipse_type
        self._eclipse_location = eclipse_location

    @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 __str__(self):
        return "Date: {0} Type {1} Location: {2}" .format(self._eclipse_date,self._eclipse_type,self._eclipse_location)

Main class:

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:
       eclipse_date_and_type = info.find('a').text
       retrieved_date_and_type = generate_date_and_type(eclipse_date_and_type)
       eclipse_location = info.find('p').text
       retrieved_location_data = generate_location(eclipse_location)
       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 generate_location(info_to_clean):
   clean_location = namedtuple("Clean_Location",["location"])
   cleanup_location = info_to_clean.split(" New Features")[0]
   location_data = clean_location(cleanup_location)
   return location_data


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

def write_data_to_file(filename, data):
    with open(filename, 'w') as eclipse_writer:
        for info in data:
            eclipse_writer.write(str(info))
            eclipse_writer.write("\n")


def main():

    eclipse_time_date = requests.get("https://www.timeanddate.com/eclipse/")
    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):
        eclipse_data =  generate_eclipse_data(eclipse_info)
        write_data_to_file(eclipse_filename, eclipse_data)
    else:
        print("No data available")

if __name__ == "__main__":
    main()

Output:

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

Questions:

  1. I tried to be as descriptive as I can when naming my variables and functions. Are they clear enough?

  2. Can I improve the script any further?

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Regarding the Eclipse class:

  1. Using "eclipse" in names is redundant. Beware though, that date and type are would not be good names since they both usually have a different meaning. A common pattern (but also unsatisfying) is to add an underscore after the name as in type_ to use special words as a variable or parameter name.
  2. In languages like Java I would probably make the properties private, but doing so in Python is much more of a maintainability trade-off as you've probably noticed. Avoiding the @property methods and naming the properties the same as the constructor parameters simplifies the code quite a bit.
  3. This class is only a container, with no logic and only a little bit of string formatting. The argument could be made that the code would be a fair bit simpler if it was replaced with a dict.
  4. You don't actually use any of the get_eclipse_ methods.

Regarding the main file:

  1. Many of the names, like eclipse_info, info_to_clean and data are vague, and don't encapsulate what they contain. In some cases you'll find that this is because they are rather arbitrary middle steps between two more cleanly defined things, in which case their processing might be improved by splitting differently, or that they really can be named more clearly in context. For example, the eclipse_data passed as data to write_data_to_file is actually a list of strings, or more accurately a list of Eclipses rather than an arbitrary binary blob.
  2. In generate_date_and_type you can merge several lines into one:

    eclipse_date, eclipse_type = info_to_split.strip().split("—")
    
  3. Rather than writing the results to a file, consider simply printing them. This makes your tool easier (and faster) to interact with using other shell tools, and it's still trivial to write the results directly to a file using a redirect.

General:

  1. This code could benefit from type hinting. This will also often emphasize names which should be changed.
  2. If I were working on this I might use an IDE to squish the entire program into a single function and see if there are any obvious classes or methods which show themselves. When building a larger system there is a lot of value to splitting and abstracting, but now that you have a working program it looks possible to squeeze the whole thing into about 20 comprehensible lines.
\$\endgroup\$
1
\$\begingroup\$

First of all, the Eclipse class' getters are totally redundant. Just use values directly. You can also use AttrDict from attrdict as a base class or maybe a namedtuple to achieve the same behavior. The idea is: classes without non-trivial behavior can use standard data classes (depends on what your bigger project standardize on though - consistency may matter).

Maybe, auxiliary functions like generate_date_and_type can operate on dictionaries instead (unless they are to be used also outside of the module): too much hassle with that namedtuple.

There are a lot of info, list, data words around. Try to give better names, without those junk words: Everything is info or data. List meaning can be conveyed by plural noun. Also, generate seems out of place: It's not generated. Probably, parse is a better word. Clean code book has a chapter on good naming.

I'd used regular expressions instead of this:

   eclipse_split_data = info_to_split.split("—")
   eclipse_date = eclipse_split_data[0]
   eclipse_type = eclipse_split_data[1].strip()

As for merging several lines to one: I disagree with the first answer. Values on their own lines helps with debugging and also help code readability.

Minor notice: it's better to use Unicode when dealing with non-ASCII encodings, I mean the "—". Of course, scripts should be also marked by the encoding directive in the very beginning: # -*- coding: utf-8 -*-

\$\endgroup\$

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.