Love to get some feedback on my first python app!!!

[Chris Angelico]

On Sat, Sep 20, 2014 at 11:16 PM, Nicholas Cannon
<nicholascannon1 at gmail.com> wrote:
> I have created my first python program and I have learnt a lot about python from this group and wanted some feedback. I am still improving it and trying to tackle some performance and GUI stuff so keep that in mind. I don't think it is the best program but is a good product of 3 months of python.
>
> link: https://github.com/nicodasiko/Article-Grab

Sure! But first, a couple of points that aren't specifically Python...

Firstly, you're using Google Groups. Until someone gets inside Google
and fixes up the code, it will make a mess of every post made through
it. Please use something better... either subscribe to the mailing
list and do everything through email, or access the newsgroup
comp.lang.python via a better client, or something of the sort.

Secondly, a note about verb tenses in English. You have this commit
message on the (as of 20140921) latest commit:

"""Fixed some bugs and did some GUI changes"""

And these comments in the code:

    #search API
    rawData = urllib.urlopen('http://ajax.googleapis.com/ajax/services/search/web?v=1.0&q='+encodedQuery).read()
    #loads data from API into json
    jsonData = json.loads(rawData)
    #extracts the results from API
    searchResults = jsonData['responseData']['results']

The more normal way to write these would be in present tense, in a
more imperative style: "Fix some bugs", "Load data", "Extract
results". (Although these comments are actually quite redundant - all
they do is tell you what the single next line of code does.) You may
also notice that the camelCase variable names you're using are in
stark contrast to the rest of the language. It's normal to use
lower_case_with_underscores instead.

Now, let's have a look at the code itself.

    query = queryVar.get()
... and further down ...

global queryVar
queryVar = StringVar()

You don't need to say "global" at top level. Everything at top-level
is global. However, I generally like to, as much as possible, define
things higher in the file than their usages - so that as you read the
file, the first reference to anything is the one that tells you what
it is. That may not be practical here, but it's something to keep in
mind. (This is why import statements generally go at the top of the
file, for instance.) The language doesn't require it, but it does make
the code easier for a human to read.

    links = []
    for result in searchResults:
        #loops through the searchResults data to find the title and
URL of the pages
        #and then add the links to the links list
        title = result['title']
        link = result['url']
        links.append(link)
    print links

You're not doing anything with the title, so what you have here is a
loop that builds up a list from empty, by repeated appends. That's a
perfect job  for a list comprehension:

    links = [result['url'] for result in searchResults]

I'm not sure why you print the links to stdout there - is that some
left-over debugging code?

Although, the only thing you do with the list of URLs is to iterate
over it, once. You could simply merge the two loops into one:

    for result in searchResults:
        url = result['url']
        pageData = urllib.urlopen(url).read()
        ... etc ...

Your text insertion is a little inconsistent: you put the URL and
title at the cursor position, then the body at the end. Is that
intentional? If so, that deserves a comment - which would be much more
useful than the comments that say what we can read in the code
already.

textDisplay.insert(INSERT, 'This spider searches for information on
your query and will display it here!')

You haven't written a spider here :) You're just calling on Google's
spider and showing some information. Similarly, the verb "CRAWL" on
your button isn't really accurate - and isn't particularly helpful to
the user either. I'd just word it in terms of how the user will value
it, and say "Search" on the button. Keep it simple.

queryWidgit = Entry(frame, textvariable=queryVar, fg='Blue', bd=5, width=50)

It's normally "Widget", not "Widgit". Not significant, but you seem to
have consistently used two i's, including in your comments.

Well, you asked for feedback. That's normally going to consist of
people pointing out where they think you did badly. But ultimately,
this is all relatively minor, so if the program works, consider
yourself to have broadly succeeded.

ChrisA