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 selenium import webdriver
from selenium.common.exceptions import NoSuchWindowException, WebDriverException

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

profile = webdriver.FirefoxProfile()
profile.set_preference("dom.popup_maximum", 0)
profile.set_preference("privacy.popups.showBrowserMessage", False)
return profile

def get_user_choice():
input("press enter when ready to choose the list of songs")
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.")

def main():
if len(sys.argv) != 2:
print("Incorrect number of arguments")
sys.exit(1)
mode = sys.argv[1]
try:
except ValueError as e:
print(e)
logger.exception(e)
sys.exit(1)
try:
driver = webdriver.Firefox(firefox_profile=profile, executable_path=FIREFOX_DRIVER_PATH)
driver.get(WEBSITE[mode])
except (WebDriverException, NoSuchWindowException) as e:
logger.exception(e)
sys.exit(1)
else:
driver.close()
sys.exit(0)

if __name__ == '__main__':
main()


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
SUBMIT_SONG = """document.getElementById("submit").click();"""

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

driver.execute_script(ENTER_SONG_NAME.format(song))
driver.execute_script(SUBMIT_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)
execute(driver, SKIP_TAGS)
execute(driver, CONVERT_NEXT)

with open(songs_list, "r", encoding="utf-8-sig") as songs:
for song in songs:
try:
except (TimeoutException, NoSuchElementException):


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

• 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.

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
• 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,
"privacy.popups.showBrowserMessage": False,
}

profile = webdriver.FirefoxProfile()
profile.default_preferences.update(FIREFOX_PREFERENCES)

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
• 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) – Lumon Apr 11 '17 at 8:30
• @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 :) – alecxe Apr 11 '17 at 16:07