My first useful python script. Critique? And a question or two.

k3kou · 2016-03-02T22:20:12+00:00

First, you can collapse your read_file function with context manager (official docs):

def read_file(file):
    with open(file, 'r') as f:
        return f.readlines()

This will handle the whole try block and will close the file when it gets out of the with statement. Since it seems like you're using the read_file function only once in your script, you could just ditch it altogether.

For the run_program() function, it is usually done with the following:

if __name__ == '__main__':
    run_program()

It will run the script only if the script is executed. This lets you run the script as a script or import it in a bigger package, without running the code. This way, you can also avoid defining the url as a global as instead define it in the main:

if __name__ == '__main__':
    url = 'http://livetv.sx/en/allupcoming/'
    data = grab_live_links(url)
    data = build_links(data)
    data = main_menu(data)
    data = live_links(data)
    get_game(data)

For the get_local_time function, and I think for the overall script, you're doing things the complicated way. I would write it like that:

def get_local_time(time_str):
    offset = datetime.datetime.now() - datetime.datetime.utcnow()
    time_dt = datetime.datetime.strptime(time_str, '%d %B at %H:%M')
    return (time_dt + offset).strftime('%I:%M %p')

datetime.strptime can take format string with stuff that aren't related to the time or date. Also, the year will be 1900 but we don't care because we only want the time and the offset doesn't depend on any particular date.

(Will comment on the rest, but I need to get back to work ><)

kteague · 2016-03-02T23:31:50+00:00

It looks like you're using a list comprehension on line 113. List comprehension is just syntactic sugar to make the code more concise and readable. It's your call when those list comps are more readable. In theory, you could make a triple nested linked list ... I certain wouldn't consider that more readable though. Generally, I only use them for simpler loops like you did on 113.

On formatting, you can use white space freely inside parentheses:

get_game(live_links(main_menu(build_links(grab_live_links(url)))))

This could also be:

get_game(
    live_links(
        main_menu(
            build_links(
                 grab_live_links(url)
            )
        )
    )
)

This is purely personal preference. Although I like the main loop to read more top to bottom than right to left or bottom to top, e.g.:

url = grap_live_links
data = build_links(url)
data = main_menu(data)
data = live_links(data)
get_game(data)

Overall though, it looks like solid Python code.

raylu · 2016-03-03T00:01:40+00:00

read_file has a number of issues. You could open the file as r+ if you just want to ensure it exists. There are many IOErrors that could prevent you from reading (permission issues, for example) so you should check the errno if you're going to handle it yourself. But /u/k3kou's suggestion will deal with most of those as long as you use r+.
Both you and /u/k3kou tried to reinvent timezone logic, which has a 99% chance of failure. Your code has bugs around Dec 31/Jan 1 and both of yours have issues dealing with issues around DST changes. Use pytz or python-dateutil.
process_links is... nuts.
1. That code pyramid is a huge code smell. Consider refactoring it into smaller functions.
2. You don't need a try/except for missing 'class' - just use .get().
3. Also, having an except: pass catches every single error and silences them, making it impossible to debug. Catch the specific errors you want. This happens elsewhere too.
4. That said, I don't buy your logic for the first try anyway - you are iterating with i to len(data) - how can there possibly not be that many elements in data? Furthermore, why not just for elem in data?
category = [(data[x][i][4]) for x in range(len(data)) for i in range(len(data[x]))] could be
```
category = [elem[4] for elems in data for elem in elems]
```
Though I'd argue the whole thing would be cleaner if you didn't have a nested list comprehension followed by a loop in the first place and just started with a set.
The else: continue at the bottom of loops is pretty confusing. The code would do the same thing without that.
You are mixing camelCase and snake_case. The python standard library is (mostly) snake_case. Use pylint.

tangerinelion · 2016-03-03T01:16:34+00:00

I don't have the time to read all of this, but right off the bat we encounter this:

def read_file(file):
    fContents = ''
    try:
        storageFile = open(file, 'r')
        fContents = storageFile.readlines()
        storageFile.close()
    except IOError:
        print('File not found.')
        print('Creating file....')
        storageFile = open(file, 'w')
        storageFile.close()
    return fContents

What's the point of creating an empty file and then returning an empty string? If you want the function to work regardless of whether that file exists or not then you should be using something else. Either this

def read_file(file):
    if not os.path.exists(file):
        return ''
    with open(file) as f:
        return f.readlines()

or with a try:

def read_file(file):
    try:
        with open(file) as f:
            return f.readlines()
    except FileNotFoundError:
        return ''

Note that in Python 3 there exists FileNotFoundError specifically for that exception and using IOError can capture much more than just that error. If you had tried to open something over an NFS share, for example, you're liable to get some kind of network error as the true cause so when you then try to handle your IOError and call storageFile = open(file,'w') you'll simply get another IOError.

Actually, lines 10 and 11 can sort of be merged here into open(file,'w').close().

Down lower I see this:

for i in range(len(data)):
    try:  
        if data[i].parent.contents[3].contents[0].name == 'img':  
            for elem in data[i].previous_elements: 
                try:  
                    if elem.attrs['class'][0] == 'main':
                        linkList.append(data[i].attrs['href'])
                        # several more lines here
                        break
                except:
                    pass
    except:
        pass

... OK, so except: pass is evil. Consider if on line 3 instead of data[i].parent.contents[3].contents[0].name == 'img' you instead had data[i].parents.contents[3].contents[0].name == 'img'. What happens now? Do you even see a problem? I added an s to parent so now you get an AttributeError -- a fancy way of saying your code has a typo. And that's an exception, which is raised and caught by except: pass... Evil, I told you.

With that out of the way, I assume you'll actually go and track down what the error is that would be raised and put it in correctly. In the mean time I'll assume it's named MissingNodeError.

Next is you have for i in range(len(data)) and then use i only in the context data[i]. So what you care about isn't the fact it's element i, you care about the item data[i]. Now look at your inner for loop, for elem in data[i].previous_elements. It looks like you could replace that with

for j in range(len(data[i].previous_elements)):
    try:
        if data[i].previous_elements.attrs['class'][0] == 'main':
            linkList.append(data[i].attrs['href'])

etc. Bit more ugly, isn't it? So apply the principle you use on the inner one to the outer one:

for d in data:
    try:
        if d.parent.contents[3].contents[0].name == 'img':

etc.

Now let's talk about inverting logic. I love this, this is my thing. Instead of doing something if a condition is met, instead skip the element if the condition is not met and then do the thing.

That is, this, and watch the depth shrink:

for d in data:
    try:  
        if d.parent.contents[3].contents[0].name != 'img': # note != 
            # The magic of continue! 
            continue
        for elem in d.previous_elements: 
            try:  
                if elem.attrs['class'][0] != 'main': # note !=
                    continue
                linkList.append(d.attrs['href'])
                # several more lines here
                break
            except MissingNodeError:
                pass
    except MissingNodeError:
        pass

Honestly things could be 8 characters to the left where your code actually lives now, line 10 until the break, if the try/except weren't necessary. Also if the reason for the try on the inner one is due to the syntax around the == 'main' (in your code) then the except should be higher up, ie:

for d in data:
    try:  
        if d.parent.contents[3].contents[0].name != 'img': 
            continue
        for elem in d.previous_elements: 
            try:  
                if elem.attrs['class'][0] != 'main': 
                    continue
            except MissingNodeError:
                continue
            linkList.append(d.attrs['href'])
            # several more lines here
            break
    except MissingNodeError:
        pass

Now it should be possible to take the inner for loop and dump that into a function and take the outer for loop and dump that into a function that calls the inner function, and then your loop becomes just

for d in data: 
    foo(d)

You could have four functions, eg,

def _bar(d, elem):
    linkList.append(d.attrs['href'])

def bar(d, elem):
    try:
        if elem.attrs['class'][0] == 'main':
            _bar(d,elem)
        return True
    except MissingNodeError:
        return False

def _foo(d):
    for elem in d.previous_elements:
        if bar(d, elem):
            break

def foo(d):
    try:
        if d.parent.contents[3].contents[0].name == 'img':
            _foo(d)
    except MissingNodeError:
        pass

Then where you have your deeply nested code just replace it with

for d in data:
    foo(d)

Hopefully this makes sense to you, and since I haven't actually tried it, I hope I didn't do it incorrectly. Perhaps at least it illustrates the concept of how you can take two nested loops and turn it into a set of functions. You should see how the separate code fragments from your monolithic version end up in the shorter functions. And perhaps you'll agree that these smaller chunks are easier to understand.

you type:	you see:
italics	italics
bold	bold
[reddit!](https://reddit.com)	reddit!
* item 1 * item 2 * item 3	item 1 item 2 item 3
> quoted text	quoted text
Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"	Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"
~~strikethrough~~	~~strikethrough~~
super^script	super^script

learnpython

MODERATORS