3
\$\begingroup\$

I have a code that scrapes list of URL and appends dataframe as below:

from selenium import webdriver
import pandas as pd
from tabulate import tabulate
import os
os.chdir(r"C:\Users\harsh\Google Drive\sportsintel.shop\Files")
cwd = os.getcwd()
print(cwd)

browser = webdriver.Chrome()

browser.get("https://www.oddsportal.com/soccer/england/premier-league/results/")

df = pd.read_html(browser.page_source, header=0)[0]

dateList = []
gameList = []
scoreList = []
home_odds = []
draw_odds = []
away_odds = []

for row in df.itertuples():
    if not isinstance(row[1], str):
        continue
    elif ':' not in row[1]:
        date = row[1].split('-')[0]
        continue
    time = row[1]
    dateList.append(date)
    gameList.append(row[2])
    scoreList.append(row[3])
    home_odds.append(row[4])
    draw_odds.append(row[5])
    away_odds.append(row[6])

result_comp_1 = pd.DataFrame({'date': dateList,
                       'game': gameList,
                       'score': scoreList,
                       'Home': home_odds,
                       'Draw': draw_odds,
                       'Away': away_odds})

print(tabulate(result_comp_1))

browser.get("https://www.oddsportal.com/soccer/england/premier-league/results/#/page/2/")

df = pd.read_html(browser.page_source, header=0)[0]

dateList = []
gameList = []
scoreList = []
home_odds = []
draw_odds = []
away_odds = []

for row in df.itertuples():
    if not isinstance(row[1], str):
        continue
    elif ':' not in row[1]:
        date = row[1].split('-')[0]
        continue
    time = row[1]
    dateList.append(date)
    gameList.append(row[2])
    scoreList.append(row[3])
    home_odds.append(row[4])
    draw_odds.append(row[5])
    away_odds.append(row[6])

result_comp = pd.DataFrame({'date': dateList,
                       'game': gameList,
                       'score': scoreList,
                       'Home': home_odds,
                       'Draw': draw_odds,
                       'Away': away_odds})


new_df =result_comp_1.append(result_comp, ignore_index=True)

Can I make my code better to avoid redundancy?

\$\endgroup\$
0

2 Answers 2

1
\$\begingroup\$

As mentioned by lsimmons your code repeats the same logic. Therefore you could abstract this into a method and call it twice.

You also have some variables overriding names in the standard library ie, date, time. Naming like this can lead to some hard to track bugs.

In addition you can remove the camel cased varaible names as this is not what standard styling defined here.

The class, although not needed, groups everything and keeps it clean. Plus if you ever have to use this data in other methods you don't have to have 6 args.

As your code gets more sophisticated you can decouple the parsing logic so you could use the same grouping code for multiple sites.

Here's some code that attempts the changes that might be helpful:

import pandas as pd
from selenium import webdriver

browser = webdriver.Chrome()


class GameData:

    def __init__(self):
        self.dates = []
        self.games = []
        self.scores = []
        self.home_odds = []
        self.draw_odds = []
        self.away_odds = []


def parse_data(url):
    browser.get(url)
    df = pd.read_html(browser.page_source, header=0)[0]
    game_data = GameData()
    game_date = None
    for row in df.itertuples():
        if not isinstance(row[1], str):
            continue
        elif ':' not in row[1]:
            game_date = row[1].split('-')[0]
            continue
        game_data.dates.append(game_date)
        game_data.games.append(row[2])
        game_data.scores.append(row[3])
        game_data.home_odds.append(row[4])
        game_data.draw_odds.append(row[5])
        game_data.away_odds.append(row[6])

    return game_data


if __name__ == '__main__':

    urls = ["https://www.oddsportal.com/soccer/england/premier-league/results/",
            "https://www.oddsportal.com/soccer/england/premier-league/results/#/page/2/"]

    results = None

    for url in urls:
        game_data = parse_data(url)
        result = pd.DataFrame(game_data.__dict__)
        if results is None:
            results = result
        else:
            results = results.append(result, ignore_index=True)

\$\endgroup\$
5
  • 3
    \$\begingroup\$ This needs some commentary. Code - only answers are off topic. \$\endgroup\$
    – Reinderien
    Commented Mar 13, 2021 at 20:39
  • 1
    \$\begingroup\$ @PyNoob put in fix with the last edit! \$\endgroup\$
    – pcauthorn
    Commented Mar 14, 2021 at 0:52
  • \$\begingroup\$ I tried the old code, I had 696 urls. I copy repeated the link and the function and run it, (I know, funny but this is before the loop solution), it takes 52s while the loop solution takes around 4 mins. Is there a reason this takes more time than the latter? \$\endgroup\$
    – PyNoob
    Commented Mar 14, 2021 at 6:52
  • 1
    \$\begingroup\$ @PyNoob That's weird, no reason it should take longer. What I would do is put in some print statements to see what's taking so long. python #at top start = datetime.now() ... print(f'<some descriptive text>: {(datetime.now()-start).total_seconds()}') You are becoming a real programmer my friend! :) \$\endgroup\$
    – pcauthorn
    Commented Mar 14, 2021 at 14:57
  • 1
    \$\begingroup\$ On the path to one: Yes, Have I reached there? No way! For the life of me, I am not able to figure out "for" loops :) \$\endgroup\$
    – PyNoob
    Commented Mar 14, 2021 at 22:06
3
\$\begingroup\$

Seems like there's one major point of redundancy, in that you're performing the same logic on the two different pages you're scraping (requesting site, parsing results, storing in dataframe).

You could eliminate the redundancy with a function that takes in the site URL and performs the logic... something like def get_matches_data(url)...

\$\endgroup\$
2
  • \$\begingroup\$ Can you pass a list of urls in the def function? \$\endgroup\$
    – PyNoob
    Commented Mar 12, 2021 at 17:06
  • \$\begingroup\$ you can write your functions to take whatever kinds of parameters you want - it's about what makes sense for your code, I'd say here it makes sense for it to take a single url, then loop through the target urls passing each one to the function \$\endgroup\$
    – lsimmons
    Commented Mar 12, 2021 at 17:07

Your Answer

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

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