3
\$\begingroup\$

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)
\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Dan Pantry Jan 5 '16 at 16:41
  • \$\begingroup\$ Okay, I changed the title, thanks for your feedback guys \$\endgroup\$ – SemAllush Jan 5 '16 at 16:42
2
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – SemAllush Jan 5 '16 at 17:07
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – Caridorc Jan 5 '16 at 17:10
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks, that was very helpful, I just don't understand the timeit bit you wrote? How should that replace what I currently have? \$\endgroup\$ – SemAllush Jan 5 '16 at 16:33
  • \$\begingroup\$ @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. \$\endgroup\$ – Peilonrayz Jan 5 '16 at 16:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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