1
\$\begingroup\$

I wrote this script for gaining experience, this script takes in a website link (NSFW) and download the video on that site. It uses regular expressions to parse the website source code to extract urls for downloading.

I'd appreciate it if anyone could take a look and told me the all thing I did wrong, as I'm sure I have employed a lot of bad programming practices.

import argparse
import requests
import re


def download_url_content(url, filename):
    with open(filename, "wb") as f:
        print("Downloading %s" % filename)
        response = requests.get(url, stream=True)
        total_length = response.headers.get('content-length')

        if total_length is None:  # no content length header
            f.write(response.content)
        else:
            dl = 0
            total_length = int(total_length)
            for data in response.iter_content(chunk_size=4096):
                dl += len(data)
                f.write(data)
                done = int(50 * dl / total_length)
                sys.stdout.write("\r[%s%s]" % ('=' * done, ' ' * (50 - done)))
                sys.stdout.flush()


def connection_error(status_code):
    print(f"Could not connect, server returned : {status_code}, exiting")
    exit()


def get_mydaddy_player_url(text):
    #matching <url: '/blocks/altplayer.php?i=//mydaddy.cc/video/refgregfef4548/',>
    result = re.search('i=//(.*?)/\'', text)
    link = result.group(1)
    print(f"Found player url, https://{link}")
    return f"https://{link}"


def extract_cdn_url_from_mydaddy_url(mydaddy_player_url):
    page = requests.get(mydaddy_player_url)
    if page.status_code != 200:
        connection_error(status_code)
    #need to match something like < <a href='//s2.bigcdn.cc/pubs/62cf1617754f13.dsad/1080.mp4' > #replace 1080 with 720/360/1080/2160
    result = re.findall('href=\'//(.*?).mp4', page.text)
    if result:
        return (result)
    else:
        print("Couldn't find cdn urls, exiting")
        exit()


def download_video(video_link, quality):
    page = requests.get(video_link)
    if page.status_code != 200:
        connection_error(page.status_code)
    #matching <title> Title </title>
    title = re.search('<title>(.*?)</title>', page.text).group(1)
    print(f"Found video : {title}")
    mydaddy_player_url = get_mydaddy_player_url(page.text)
    cdn_urls = extract_cdn_url_from_mydaddy_url(mydaddy_player_url)
    flag = True
    for item in cdn_urls:
        if item.find(str(quality)) != -1:
            cdn_url = f"https://{item}.mp4"
            print(f"Found cdn url : {cdn_url}")
            download_url_content(cdn_url, f"{title}.mp4")
            flag = False
            break
    if flag:
        print("Couldn't find proper cdn url, exiting")
        exit()


def main():
    parser = argparse.ArgumentParser(
        description='A Python script to download Youtube videos')
    group_ = parser.add_mutually_exclusive_group()
    group_.add_argument('--video_link',
                        metavar='video-link',
                        type=str,
                        help='Enter the video url')
    parser.add_argument('--quality',
                        metavar='quality',
                        type=int,
                        help='Enter the desired video quality')
    args = parser.parse_args()
    if args.video_link:
        video_link = args.video_link
        download_video(video_link, args.quality)

main()
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Some quick thoughts in no particular order:


Using regular expressions to parse HTML code is not a good idea, ever. Use Beautiful Soup instead.

Likewise, to extract the title you'll then simply do like this:

from bs4 import BeautifulSoup
soup = BeautifulSoup(html, 'html.parser')
title = soup.find('title')

Note that this is safer than your current method:

title = re.search('<title>(.*?)</title>', page.text).group(1)

You're assuming that the regex capture will always return data - this assumption may not always be true. Your function get_mydaddy_player_url also makes the same assumption. It's better to check that something was actually returned, rather than let your program crash or behave unpredictably.

Also a regex parsing HTML should at a minimum perform a multiline search. It's common for HTML tags to span several lines.

From the BS docs:

If find_all() can’t find anything, it returns an empty list. If find() can’t find anything, it returns None

So, find may return None if no title was found but your code will not crash.


Make it a habit to use the main guard in your code


If you need to increment a variable in a for loop, you'll probably want to use the enumerate function.


Variable video_link is superfluous, you could just pass args.video_link like you do args.quality.

if args.video_link:
    video_link = args.video_link
    download_video(video_link, args.quality)

The variable flag is equally superfluous. If your function extract_cdn_url_from_mydaddy_url returns a list, then it's sufficient to check that the list has a size > 0. And you can perform the check early, before entering the for loop.

To get back to that extract_cdn_url_from_mydaddy_url function:

  • first of all rename it to make it more generic
  • a function should be consistent in terms of what type of values it returns. You could return an empty list if it fails, but return something consistent and predictable. The decision to exit or not should be left to the calling routine. This function should do only one thing: return a list of URLs, not decide on program control flow. And in fact your code already makes that decision to quit later on, which further emphasizes the point.

I think you can drop the connection_error function. Just add handling for the requests exceptions ie:

try:
    # run some code
except requests.exceptions.HTTPError as ex:

Or you may want to use the raise_for_status() method.


Why the add_mutually_exclusive_group() option?


Add some more flexibility: your code should be able to handle more than just .mp4 files.

Your code resorts to a mix of print and sys.stdout.write. I suggest that you use the logging module instead. One benefit is that you can log the output to a file as well, which is useful when your console buffer is full or you want to review the results of some previous invocation. You can even do fancy stuff like color printing as long as your console supports it.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for the input. The "player_url" is inside a javascript. I can extract the script using beautifulsoup. Is it safe to use regex for parsing javascript code? \$\endgroup\$
    – User
    Jul 15, 2022 at 10:06
  • \$\begingroup\$ That depends on the exact output. But it's still possible to use regexes on top of BS if you have a very peculiar parsing need. \$\endgroup\$
    – Kate
    Jul 15, 2022 at 16:55

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.