3
\$\begingroup\$

Given a text file containing a list of links, I wrote some code to download them, one by one, from YouTube, using either YoutubeMP3 or Convert2mp3 logging any songs that failed to download.

The code with Youtube-mp3 is significantly faster than Convert2mp3 but less accurate (some songs fail for unknown reasons, even manually), due to the nature of the websites, but any tips on improving performance would be greatly appreciated.

Main.py

import sys
from tkinter.filedialog import askdirectory, askopenfilename

from selenium import webdriver
from selenium.common.exceptions import NoSuchWindowException, WebDriverException

from downloader import downloader
from downloader import logger


FIREFOX_DRIVER_PATH = "" # A path need to coded here.
CONVERTER2MP3 = "http://convert2mp3.net/en/index.php"
MP3_CONVERTER = "http://www.youtube-mp3.org/"
WEBSITE = {"1": CONVERTER2MP3, "2": MP3_CONVERTER}


def get_firefox_profile(download_directory):
    profile = webdriver.FirefoxProfile()
    profile.set_preference("dom.popup_maximum", 0)
    profile.set_preference("browser.download.folderList", 2)
    profile.set_preference("browser.download.dir", download_directory)
    profile.set_preference("browser.download.panel.shown", False)
    profile.set_preference("privacy.popups.showBrowserMessage", False)
    profile.set_preference("browser.download.manager.showWhenStarting", False)
    profile.set_preference("browser.helperApps.neverAsk.saveToDisk", ".mp3 audio/mpeg")
    return profile


def get_user_choice():
    input("press enter when ready to choose the list of songs")
    songs_list = askopenfilename(initialdir='.')
    if not songs_list:
        raise ValueError("You did not choose the songs list file.")
    if not songs_list.endswith("txt"):
        raise ValueError("A text file must be chosen.")

    input("press enter when ready to choose the download directory")
    download_directory = askdirectory(initialdir='.')
    if not download_directory:
        raise ValueError("You did not choose the download directory.")
    return songs_list, download_directory


def main():
    if len(sys.argv) != 2:
        print("Incorrect number of arguments")
        print("""You need to choose one of the following modes:\n1 - Downloading the songs from converter2mp3.\n2 - 
            Downloading the songs from mp3 converter.""")
        sys.exit(1)
    mode = sys.argv[1]
    try:
        songs_list, download_directory = get_user_choice()
    except ValueError as e:
        print(e)
        logger.exception(e)
        sys.exit(1)
    print("Starting to download songs...")
    try:
        profile = get_firefox_profile(download_directory)
        driver = webdriver.Firefox(firefox_profile=profile, executable_path=FIREFOX_DRIVER_PATH)
        driver.get(WEBSITE[mode])
        downloader(driver, mode, songs_list)
    except (WebDriverException, NoSuchWindowException) as e:
        logger.exception(e)
        sys.exit(1)
    else:
        driver.close()
        sys.exit(0)


if __name__ == '__main__':
    main()

Downloader.py

import logging
import time

from selenium.common.exceptions import NoSuchElementException, TimeoutException
from selenium.webdriver.common.by import By
from selenium.webdriver.remote.remote_connection import LOGGER
from selenium.webdriver.support import expected_conditions as EC
from selenium.webdriver.support.ui import WebDriverWait

LOGGER.setLevel(logging.WARNING)
logger = logging.getLogger(__name__)
logging.basicConfig(filename="failed_songs.log", level=logging.DEBUG)

# MP3_CONVERTER
ENTER_SONG_NAME = """document.getElementById("youtube-url").value="{}";"""
SUBMIT_SONG = """document.getElementById("submit").click();"""
PRESS_DOWNLOAD = """for(child in document.getElementById("dl_link").childNodes)if (document.getElementById("dl_link").childNodes.hasOwnProperty(child))if(document.getElementById("dl_link").childNodes[child].style.display==""){document.getElementById("dl_link").childNodes[child].click();break;}"""

# CONVERTER2MP3
INSERT_SONG_NAME = """document.getElementById("urlinput").value="{}";"""
CONVERT_SONG = """document.getElementsByTagName("button")[1].click();"""
SKIP_TAGS = """document.getElementsByTagName("a")[12].click();"""
DOWNLOAD_SONG = """document.getElementsByTagName("a")[9].click();"""
CONVERT_NEXT = """document.getElementsByTagName("span")[0].click();"""


def download_mp3_converter(driver, song):
    driver.execute_script(ENTER_SONG_NAME.format(song))
    driver.execute_script(SUBMIT_SONG)
    WebDriverWait(driver, 5).until(EC.visibility_of_element_located((By.ID, "dl_link")))
    driver.execute_script(PRESS_DOWNLOAD)


def download_converter2mp3(driver, song):
    def execute(web_driver, command):
        web_driver.execute_script(command)
        time.sleep(3)

    driver.execute_script(INSERT_SONG_NAME.format(song))
    driver.execute_script(CONVERT_SONG)
    WebDriverWait(driver, 20).until(EC.element_to_be_clickable((By.ID, "advancedtags_btn")))
    execute(driver, SKIP_TAGS)
    execute(driver, DOWNLOAD_SONG)
    execute(driver, CONVERT_NEXT)


DOWNLOAD_FUNCTIONS = {"1": download_converter2mp3, "2": download_mp3_converter}


def downloader(driver, mode, songs_list):
    download_song = DOWNLOAD_FUNCTIONS[mode]
    with open(songs_list, "r", encoding="utf-8-sig") as songs:
        for song in songs:
            try:
                download_song(driver, song.strip("\n"))
            except (TimeoutException, NoSuchElementException):
                logger.warning(" Failed to download: " + song.strip("\n"))

This is my first project not done to the instruction of or under the supervision of anyone, so code review is important to me.

Points I am unsure about:

  • Organization, naming and conventions, the placement of my many constants, the hardcoded Javascript.
  • Downloader.py my nested execute wrapper function, it can easily be replaced and although I like it better, it may be superfluous.

I could have logged every activity going on, but since logging is not the main purpose of the code, I removed the info loggings and kept only the important ones.

Besides that, any other opinion is much appreciated, including topics I didn't take into consideration like security.

\$\endgroup\$
2
\$\begingroup\$

Here are some first impressions and thoughts.

Code Organization

  • Move all selenium specific code to the "downloader". I think your "main" script should only be responsible for getting the input parameters and then invoke the downloader which "hides" the way it does the downloading internally. I would move the profile preparation and the webdriver initialization to the "downloader".
  • to continue on the topic of your "downloader". You should consider creating a "Downloader" class. One of the benefits of having a class is that you may keep your driver as an instance variable. And, you may also define the WebDriverWait instance once and reuse instead of re-instantiating it every time you need to wait.

Code Style

  • consider using multi-line strings for your JS scripts
  • add meaningful docstrings to your modules and functions
  • you can improve the get_firefox_profile() function by pre-defining a dictionary of preferences and setting them at once. Something like:

    FIREFOX_PREFERENCES = {
        "dom.popup_maximum": 0,
        "browser.download.folderList": 2,
        "browser.download.panel.shown": False,
        "privacy.popups.showBrowserMessage": False,
        "browser.download.manager.showWhenStarting": False,
        "browser.helperApps.neverAsk.saveToDisk": ".mp3 audio/mpeg",
    }
    
    
    def get_firefox_profile(download_directory):
        profile = webdriver.FirefoxProfile()
        profile.default_preferences.update(FIREFOX_PREFERENCES)
        profile.set_preference("browser.download.dir", download_directory)
    
        return profile
    

Reliability

  • Fragile locators. Even though you are defining JS scripts as locators, the locators themselves are fragile - for instance, relying on the a element to be 12th or 9th in the DOM tree may be broken easily.
  • To continue on this topic, I would expect the "downloader" to use Selenium-specific location techniques instead of doing it in pure JavaScript. You could have used CSS selectors or XPath expressions based on meaningful element classes or other attributes. Though, I suspect that you did that intentionally to minimize the number of Selenium commands processed via JSON over HTTP. If this is the case, I think you sacrifice reliability. And, also make things more difficult to understand. For instance, it is not immediately obvious what PRESS_DOWNLOAD script does.
  • avoid using time.sleep() - it generally leads to waiting more than needed in 95% of cases and waiting less in 5%. Ideally, you should use Explicit Waits if it is at all possible in your situation.

Other Random Thoughts

  • Improve reading command-line arguments. You don't have a lot of CLI arguments, but you can still improve the way you do it - consider switching to argparse which will help to make this part of the code more readable and concise
  • If the project and the browser interaction logic will continue to grow, consider using a Page Object pattern
  • selenium brings you a lot of overhead, please see if you can either find an mp3 converter that has a public API, or see if you can use existing two without using a real browser
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for the many suggestions, I will certainly make use of them! About selenium and JS, what methods are there to use those websites without a real browser? (even though finding an API would make things easier) \$\endgroup\$ – Lumon Apr 11 '17 at 8:30
  • \$\begingroup\$ @Lumon glad to help; the idea is usually to open browser developer tools, observe the requests sent to the server when you make browser actions and then try to mimic these requests in your code - typically, using requests package. But, before doing that please search the PyPI index in case there is a module existing for your use case - there are over 100000 packages there nowadays :) \$\endgroup\$ – alecxe Apr 11 '17 at 16:07

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.