i'm a python newbie & wrote my first script, can someone critique it?
Matt Wheeler
m at funkyhat.org
Sat Jun 11 00:28:50 EDT 2016
First of all welcome :)
The other suggestions you've received so far are good so I won't repeat
them... (note that in particular I've reused the names you've chosen in
your program where I've given code examples, but that's purely for clarity
and I agree with the others who've said you should use a more pythonic
naming convention)
On Fri, 10 Jun 2016, 23:52 mad scientist jr, <mad.scientist.jr at gmail.com>
wrote:
> Is this group appropriate for that kind of thing?
> (If not sorry for posting this here.)
>
> So I wanted to start learning Python, and there is soooo much information
> online, which is a little overwhelming. I really learn best from doing,
> especially if it's something actually useful. I needed to create a bunch of
> empty folders, so I figured it was a good exercise to start learning Python.
> Now that it's done, I am wondering what kind of things I could do better.
> Here is the code:
>
> # WELCOME TO MY FIRST PYTHON SCRIPT!
> # FIRST OF ALL, IT ***WORKS***!!! YAY!
> # I AM A VBA AND JAVASCRIPT PROGRAMMER,
> # SO IT IS PROBABLY NOT VERY "PYTHONIC",
> # SO PLEASE FEEL FREE TO TEAR THE SCRIPT A NEW ONE
> # AFTER YOU ARE SHOCKED BY THIS BAD CODE,
> # ALL I ASK IS THAT IF YOU THINK SOMETHING IS BAD,
> # PLEASE POST AN EXAMPLE OF THE "CORRECT" WAY OF DOING IT
> # COMING FROM VBA, I KNOW A LITTLE OOP,
> # BUT NOT C++ C# JAVA STUFF LIKE "INTERFACES" AND "ABSTRACT BASE CLASSES"
> # SO IF YOU GET INTO SUCH TERMINOLOGY MY EYES MIGHT START TO GLAZE OVER
> # UNLESS YOU CARE TO EXPLAIN THAT TOO
>
Don't worry, Python doesn't have "interfaces" like Java (though it does
have multiple inheritance instead, which is more powerful), and it's not
necessary to learn about advanced things like abstract classes until you're
comfortable :)
>
>
> ############################################################################################################################################################
> # GLOBAL VALUES
> sForPythonVersion="3"
> #sFolderPathTemplate = "c:\\myscripts\\MP3 Disc <iCount/>"
> sFolderPathTemplate = "c:\\temp\\MP3 Disc <iCount/>"
>
It's possible in Python to use forward slashes in paths, even on Windows,
which would allow you to avoid the double slashes above (and can make your
code more easily ported to other OSes, which pretty universally use a '/'
in paths.
I would also suggest you use python's string formatting capabilities
instead here (see below where I use this FOLDER_TEMPLATE string):
FOLDER_TEMPLATE = 'C:/temp/MP3 Disk {count:03}'
iFromCount = 1
> iToCount = 250
> iCountWidth = 3
>
>
> ################################################################################################################################################################
> # SUPPORT FUNCTIONS
>
> #
> //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> def is_string(myVar): # is_string IS MORE READABLE THAN isinstance (PLAIN
> ENGLISH!)
> #PYTHON 3 IS NOT LIKING THIS: return ( isinstance(myVar, str) or
> isinstance(myVar, unicode) )
> #PYTHON 3 IS NOT LIKING THIS: return isinstance(myVar, basestr)
> return isinstance(myVar, str)
>
I know this one's been asked already but I think it deserves expanding on.
In general in Python it's recommended that you don't try too hard (if at
all) to check types of things. Think about what purpose this serves.
Pythonic programs follow what's known as "duck typing". i.e. if it quacks
like a duck then we can treat it like it's a duck.
This lets you write code which expects, for example something like a
string, or a dict or list, but isn't strictly bound to those types. That
allows other programmers to create their own classes, perhaps a subclass of
dict or list, or some unconnected class which just happens to be list-like
enough, and it can just work with your code.
Of course there are places where you do need to check the type of
something, but if you think you need to it's always worth really asking
yourself why you think that.
(I think this is particularly true when it comes to `str`. Most classes can
be represented one way or another as a string, so why not let people pass
you anything if you're going to read it as a string anyway? Refusing to
accept a non-string is more work for you and more work for them for little
benefit)
#
> //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> # THIS IS SOME SAMPLE FUNCTION FROM A BOOK I AM READING "PYTHON IN EASY
> STEPS"
> def strip_one_space(s):
> if s.endswith(" "): s = s[:-1]
> if s.startswith(" "): s = s[1:]
> return s
>
> #
> //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> def get_exact_python_version():
> import sys
>
Imports belong at the top of the file unless there's a good reason you
don't always want the module imported (e.g. if it's really large or doesn't
exist on all the systems your program runs on and isn't a strict
requirement). `sys` or `os` definitely don't fall into that category (and I
see you've imported them more than once!)
sVersion = ".".join(map(str, sys.version_info[:3]))
> sVersion = sVersion.strip()
> return sVersion
>
> #
> //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> # TO DO: RETURN TO THE LEFT OF FIRST "."
> def get_python_version():
> sVersion = get_exact_python_version()
> return sVersion[0:1]
>
Why not just look up sys.version_info[0] instead of having this function?
#
> //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> # CHECK PYTHON VERSION, IF IT'S WRONG THEN GRACEFULLY EXIT BEFORE IT BLOWS
> UP
> # (DAMN, PYTHON 2.x STILL COMPLAINS WITH SYNTAX ERRORS!!)
> # MAYBE THIS COULD STILL BE USEFUL FOR CHECKING THE SUB-VERSION, IN THAT
> CASE
> # TO DO: MORE GRANULAR CHECK, EG IF VERSION >= 3.5.0
> def exit_if_wrong_python_version(sRightVersion):
> import os
> sCurrentVersion = get_python_version()
> if (sCurrentVersion != sRightVersion):
> print("" +
> "Wrong Python version (" +
> sCurrentVersion +
> "), this script should be run using Python " +
> sRightVersion +
> ".x. Exiting..."
> )
> os._exit(0)
Better would be just let it crash. That way you can see a stacktrace and
work out how to fix it. There's a place for doing version checking like
this, but small single-purpose scripts should be small and easy to follow.
(I have a feeling you should be using sys.exit() rather than os._exit()
too, maybe someone else can confirm)
#
> //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> def get_script_filename():
> import os
return os.path.basename(__file__)
>
This function's name is nearly as long as the code you're abstracting. Is
it worth having a function for this?
#
> //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> def get_timestamp():
> import datetime
> return datetime.datetime.strftime(datetime.datetime.now(), '%Y-%m-%d
> %H:%M:%S')
>
> #
> //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
>
> def create_folder(sPath):
> import os
> import errno
> #if not os.path.exists(directory):
> # os.makedirs(directory)
> try:
> os.makedirs(sPath, exist_ok=True)
> except OSError as exception:
> #if exception.errno != errno.EEXIST:
> print("ERROR #" + str(exception.errno) + "IN makedirs")
> raise
>
If you're targetting Python 3 only you should be able to choose the more
specific subclasses of OSError such as FileExistsError rather than worrying
about the errno. If you want to catch *some* of OSError's subclasses but
not others that's possible like:
try:
...
except (FileExistsError, IsADirectoryError) as e:
...
raise
#
> //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
>
> def create_folders(sFolderPathTemplate:str="", iFromCount:int=1,
> iToCount:int=0, iCountWidth:int=0):
> # MAKE SURE TEMPLATE'S A STRING. OH, IS THIS NOT "PYTHONIC"? WELL IT'S
> MORE READABLE
> if is_string(sFolderPathTemplate) == False:
> iFromCount = 1; iToCount = 0;
>
> sName = ""
> sCount = ""
>
There's no need to initialize variables before they are used
> iLoop = iFromCount
> while (iLoop <= iToCount) and (len(sFolderPathTemplate) > 0):
> sCount = "{0}".format(iLoop)
>
str(iLoop) would have done fine
> if (iCountWidth > 0):
> sCount = sCount.zfill(iCountWidth)
> sName = sFolderPathTemplate.replace("<iCount/>", sCount)
> create_folder(sName)
> iLoop = iLoop + 1
>
But in Python it's not necessary to manage your own loops like this.
It would be better rewritten as a for loop:
for i in range(1, iToCount + 1):
create_folder(FOLDER_TEMPLATE.format(count=i))
Note I haven't had to deal with padding the counter as the format spec in
FOLDER_TEMPLATE deals with that, I don't need to manually increment a loop
counter, for and range are working together to give me that.
############################################################################################################################################################
> # MAIN LOGIC
>
> def main():
> #
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------
> # MAKE SURE PYTHON VERSION IS CORRECT
> exit_if_wrong_python_version(sForPythonVersion)
> print("PYTHON VERSION (" + get_exact_python_version() + ") MATCHES
> REQUIRED VERSION (" + sForPythonVersion + ")")
>
Look up string formatting (things like `'Python version: {}. Required
version: {}'.format(get_exact_python_version(), sForPythonVersion)`) as an
alternative to using + multiple times. It makes things more manageable and
is much more powerful.
#print("get_python_version returns \"" + get_python_version()
> + "\"")
> #print("get_exact_python_version returns \"" +
> get_exact_python_version() + "\"")
>
> #
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------
> # PRINT START TIMESTAMP
> print("" + get_timestamp() + " " + get_script_filename() + " STARTED.")
>
> #
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------
> # DO WHAT WE CAME TO DO
> # IT'S PROBABLY BAD FORM TO REFERENCE GLOBAL VARIABLES INSIDE THE
> SCOPE OF A FUNCTION?
> # BUT I WANT TO MAKE IT EASY TO CONFIGURE THE SCRIPT JUST BY CHANGING
> A COUPLE OF LINES
> # AT THE TOP, WITHOUT HAVING TO SEARCH THROUGH THE CODE
> create_folders(
> sFolderPathTemplate,
> iFromCount,
> iToCount,
> iCountWidth)
>
> #
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------
> # NOW FINISH
> print("" + get_timestamp() + " " + get_script_filename() + "
> FINISHED.")
>
If you were using a *NIX OS I would suggest dropping your start and end
timestamps and just calling your script via the 'time' command, but
apparently Windows doesn't have that :( however I would perhaps move them
out of your `main()` function and wrapping the `main()` call below with
them instead. I don't feel like they are really "part of the program". That
may just be my taste though.
#import os
> #os._exit(0)
>
>
> ############################################################################################################################################################
>
> main()
>
This is good, you've not just put your logic in the body of the script, it
could be slightly better though. The standard pattern goes:
if __name__ == '__main__':
main()
Which means if you were to import your script as a library `main()` won't
be run, allowing you to reuse functions easily, and making writing tests
much easier.
>
More information about the Python-list
mailing list