Cleaner Code

[arkravitz]

Hey,

Sorry I haven't been around for a while. I'll help you clean up your code when I have some time. But just a few tips:

1. Coding is like writing, you can't just code and be done. You have to test, code, and revise, then pass your tests.
2. Functions should have one specific purpose. You have a lot of functions that are at many different levels of abstraction, making the code difficult to read.
3. Python files should be much smaller, and so should your classes. That will really help with anyone trying to decipher what's going on.
4. Try to separate your unit tests from your code. That way, you can edit the code without affecting the unit tests and vice versa.
5. Try to also keep todos separate from code. They can cause clutter, and oftentimes don't keep up with the code. Yes, you can keep them in the code, but in my opinion, it's better to keep a list on a website or something similar. Another idea is to have a TODO file. I just like my code to be just that, code.

All of this will help you to make your code more modular, easier to edit, and much easier for someone to jump in and read. If you want some recommended reading on the topic, feel free to email me (I believe you still have my email). The most important thing I've learned in the past few years is that code is an expression that BOTH a computer and humans should be able to understand. If you code for just the computer, you won't be able to change what you did in a couple of months.

[arkravitz]

Stuff like this view should be cleaned up into smaller, more accurate functions.

@APP.route('/change', methods=['GET','POST'])
@AUTH.required
def change_screenname():
    """
    Without the user being logged in, give the user the option to change their
    screenname from what Google OpenID gave us.
    """
    alt = ensure_user()
    if alt:
        return alt
    form = ChangeForm()
    if form.validate_on_submit():
        new_screenname = form.change.data
        if 'userid' in session:
            # Having a userid means they're in the database.
            req = ChangeScreennameRequest(session['userid'],
                                          new_screenname)
            resp = API().change_screenname(req)
            if resp == API.ERR_DUPLICATE_SCREENNAME:
                flash("That screenname is already taken.")
            elif isinstance(resp, APIError):
                logging.debug("change_screenname error: %s", resp)
                flash("An error occurred.")
            else:
                session['screenname'] = new_screenname
                flash("Your screenname has been changed to '%s'." %
                      (new_screenname, ))
                return redirect(url_for('home_page'))
        else:
            # User is not logged in. Changing screenname in session is enough.
            # Now when they go to the home page, ensure_user() will create their
            # account.
            session['screenname'] = new_screenname
            return redirect(url_for('home_page'))
    current = session['screenname'] if 'screenname' in session  \
        else g.user['name']
    navbar_items = [('', url_for('home_page'), 'Home'),
                    ('', url_for('about_page'), 'About'),
                    ('', url_for('faq_page'), 'FAQ')]
    return render_template('web/change.html', title='Change Your Screenname',
        current=current, form=form, navbar_items=navbar_items,
        is_logged_in=is_logged_in(), is_account=True)

alt = ensure_user() is unclear... What's alt stand for? Comments should supplement code, not replace it.
It would be clearer to use a longer name, like alternate_username = get_alternate_username() or something similar.
I'll review more in an update later.

[RangeVsRange]

Thanks arkravitz.

I'll replace all those ensure_user() calls with a decoration. And the one I can't, I'll rename "alt" to "alternate_response".

I look forward to reading more of your suggestions. Thanks again.

[arkravitz]

One thing I'd focus on is getting tests that test most of your functionality. Both unit tests and integration tests. Once you have those, refactoring will be easy. Also, best practice is to put tests in a /test folder and then include the modules you need in each test. That makes it clear and clean.

[arkravitz]

Also, what editor do you use? You have windows spaces and carriage returns which is not usually recommended for open source software.

[arkravitz]

Also, really glad to see you're still making progress on this. It's looking good!

[arkravitz]

Check rvr/views/range_editor.py on https://github.com/arkravitz/range-vs-range. I started to decompose your functions a bit. Some of them are quite beastly. I haven't finished and am not anywhere close to done, but you can check out how I did it.

[RangeVsRange]

I can't make a decorator for ensure_user(). It evidently interferes with @AUTH.required. In fact, I recall now that this is why I had this TODO:

# TODO: REVISIT: automagically hook this into AUTH.required

Anyway, I've renamed alt to alternative_response, at least.

[RangeVsRange]

I've been using Eclipse on Windows. Yes, I've just checked and it was set to use Windows line breaks.

I've not changed this, and converted all the text-based files in the project (at least, I think I got them all).

Thanks for the tip.