current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite

I'm a coder who's relatively new to Python and I wanted to spice up my console applications a bit by making animations. The code I've written works perfectly, but I just wanted to know if it was good in a pythonic way and if it follows the OO standards. If not please tell me what could be improved and why.

import subprocess as sp
import time

class Animation(object): 

    def __init__(self):

        self.frames = []


    def add_frame(self,frame):
        #Adds frame to end of list
        self.frames.append(frame)

    def remove_last_frame(self):
        #Removes last frame in list
        del self.frames[-1]

    def clear_frames(self):
        #Deletes all frames
        del self.frames[:]

    def get_frames(self):
        #Returns list of frames
        return self.frames

    def run_animation(self,loops,fps):
        #Runs the animation at a desired framerate and loops it a given amount of times
        delay = 1.0/fps
        for x in range(0,loops):
            for frame in self.frames:
                #The following line clears the shell
                tmp = sp.call('clear',shell=True)
                print frame
                time.sleep(delay)
share|improve this question
2  
Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. – Dan Pantry Jan 5 at 16:41
    
Okay, I changed the title, thanks for your feedback guys – SemAllush Jan 5 at 16:42
up vote 2 down vote

Reconsider the need for a class

The functions:

def add_frame(self,frame):
    #Adds frame to end of list
    self.frames.append(frame)

def remove_last_frame(self):
    #Removes last frame in list
    del self.frames[-1]

def clear_frames(self):
    #Deletes all frames
    del self.frames[:]

def get_frames(self):
    #Returns list of frames
    return self.frames

Are useless and may be removed.

So you are left with a class that has only __init__ and one other method.

You do not need a class for this:

def run_animation(frames, loops, fps):
    delay = 1.0 / fps
    for _ in range(loops):
        for frame in frames:
            sp.call('clear',shell=True)
            print(frame)
            time.sleep(delay)

Please do not make a class for the sake of making a class, often it just complicates the code.

Minor clean-ups:

  • x -> _ as you do not use it.
  • tmp = -> `` as, again, you do not use it.
  • print frame = -> print(frame) for Python 3 compatibility.
share|improve this answer
    
I have made changes to the init after posting this, now it imports the animation from a file when it's initialized. Do you still think a class is useless? Because it really cleans up my code – SemAllush Jan 5 at 17:07
1  
@SemAllush an empty __init__ was useless, importing from a file it probably makes sense to have, but maybe not, I cannot really say without seeing the code. I suggest waiting a day and then posting the new version reading from a file up for review. – Caridorc Jan 5 at 17:10
1  
Thank you for your help! – SemAllush Jan 5 at 17:11
up vote 1 down vote

Most of your functions can, and should be removed. Most of the functions are just helper functions to self.frames, and some of them are already functions!

  • add_frame should be removed, this is as you should use frames.append instead.

  • remove_last_frame can be replaced with frames.pop()

  • clear_frames is bad.

    You used the most performance inefficient approach to do this. Instead just change it's value.

    >>> import timeit
>>> timeit.timeit('a=[1,2];del a[:]')
0.40249805013166945
>>> timeit.timeit('a=[1,2];a[:] = []')
0.31216538638113
>>> timeit.timeit('a=[1,2];a = []')
0.19833872924905194

  • get_frames is un-Pythonic, you don't use getters and setters in Python. And if you do, you have sugar to remove the get_my_var_name, set_my_var_name.

    Also you don't use it yourself...

There are a too few spaces in your code, e.g. 1.0 / fps instead of 1.0/fps. But that's not that major a problem. If you want to know more you can read PEP8.

One thing that you should change is range(0,loops) to range(loops). It behaves the same and saves you some typing. Also in Python2 you would want to use xrange over range for minimal speed benefits, which can add up. Or leave it as range and convert to Python3.

share|improve this answer
    
Thanks, that was very helpful, I just don't understand the timeit bit you wrote? How should that replace what I currently have? – SemAllush Jan 5 at 16:33
    
@SemAllush timeit is a way to find how long functions take to run. What it shows is def clear_frames(self):self.frames = [] would be faster than def clear_frames(self):del self.frames[:]. Also someone else may come along with a better review than me, and so you may want to unaccepted my answer and see if you get a better review. – Joe Wallis Jan 5 at 16:39

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.