i'm a python newbie & wrote my first script, can someone critique it?
Larry Hudson
orgnut at yahoo.com
Sat Jun 11 00:00:02 EDT 2016
On 06/10/2016 03:52 PM, mad scientist jr 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:
>
It is FAR too complicated -- it's BASIC written Python. Python is MUCH easier.
First a couple of general comments...
Drop the Hungarian notation!! Python uses dynamic typing. Variables do NOT have a type, the
data they hold have types, but not the variable itself. ANY variable can hod ANY data type at
ANY time. And Python uses duck typing -- get used to it.
Generally you should put all the imports at the beginning of the program, NOT in each function.
A possible exception could be if an import is only used in one function.
> ############################################################################################################################################################
> # GLOBAL VALUES
> sForPythonVersion="3"
> #sFolderPathTemplate = "c:\\myscripts\\MP3 Disc <iCount/>"
> sFolderPathTemplate = "c:\\temp\\MP3 Disc <iCount/>"
> iFromCount = 1
> iToCount = 250
> iCountWidth = 3
>
Drop the <iCount/> from the template string. (More on this below.)
> ################################################################################################################################################################
> # 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)
>
Not necessary.
> # //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> # 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
>
??? Is this used anyplace? Delete it.
> # //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> def get_exact_python_version():
> import sys
> sVersion = ".".join(map(str, sys.version_info[:3]))
> sVersion = sVersion.strip()
> return sVersion
>
sys.version_info[:3] by itself gives a three-element tuple. Probably easier to use than the
string version.
A couple alternatives:
sys.version[:5] or perhaps a bit safer -- sys.version.split()[0] both directly give you the
string version. (Note this uses version not version_info.)
> # //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> # TO DO: RETURN TO THE LEFT OF FIRST "."
> def get_python_version():
> sVersion = get_exact_python_version()
> return sVersion[0:1]
>
Probably unnecessary as a function. A simple sVersion[0] in-line might be easier.
> # //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> # 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)
>
Get used to Python string formatting...
print("Wrong Python version ({}), this script should be run using "
"Python {}.x, Exiting...".format(sCurrentVersion, sRightVersion))
Notice how I split the long single-line string into two shorter strings on two lines relying on
the automatic string concatenation in Python. "string1 " "string2" becomes "string1 string2".
Any whitespace (spaces, tabs, newlines) are ignored and the two strings are stuck together.
Also the common use is sys.exit() instead of os._exit().
> # //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> def get_script_filename():
> import os
> return os.path.basename(__file__)
>
sys.argv[0]
> # //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
> 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
>
> # //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
>
> 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;
More readable? The pythonic version would be simply to not use this at all. Now, which is more
readable -- this crap or blank lines?
>
> sName = ""
> sCount = ""
This is not BASIC, you don't need to pre-declare your variables.
> iLoop = iFromCount
> while (iLoop <= iToCount) and (len(sFolderPathTemplate) > 0):
Why check for the template? It's a global variable, you know it's valid. If not, you deserve
your errors.
> sCount = "{0}".format(iLoop)
> if (iCountWidth > 0):
> sCount = sCount.zfill(iCountWidth)
> sName = sFolderPathTemplate.replace("<iCount/>", sCount)
VERY tricky here, but...
sCount = "{{:0{}}.format(iCountWidth)" if iCountWidth else "{}"
sName = "{} {}".format(sFolderPathTemplate, sCount).format(iLoop)
More direct...
if iCountWidth:
fmt = "{{:0{}}}".format(iCountWidth)
sCount = fmt.format(iLoop)
else:
sCount = str(iLoop)
sName = "{} {}".format(sFolderPathTemplate, sCount)
Note this assumes the <iCount/> has been deleted from the template string as noted above.
> create_folder(sName)
> iLoop = iLoop + 1
More convenient and more pythonic: iLoop += 1
>
> ############################################################################################################################################################
> # 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 + ")")
> #print("get_python_version returns \"" + get_python_version() + "\"")
> #print("get_exact_python_version returns \"" + get_exact_python_version() + "\"")
Perhaps unnecessary to check the version, but YMMV... However, the
exit_if_wrong_python_version() aborts with an error message, why bother to state that the
version is good? If it isn't the program doesn't run anyway.
>
> # ----------------------------------------------------------------------------------------------------------------------------------------------------------------
> # 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)
As you say, these are all global variables and are therefore already available to the
create_folders() function, so why bother passing them as parameters? Your reason for globals
here is probably valid, especially since they are used as constants rather than variables. But
in general (in all programming languages) it is usually better and safer to avoid globals if
possible.
>
> # ----------------------------------------------------------------------------------------------------------------------------------------------------------------
> # NOW FINISH
> print("" + get_timestamp() + " " + get_script_filename() + " FINISHED.")
> #import os
> #os._exit(0)
Yes, your exit() is redundant and you are correct to comment it out.
But again I suggest that you get used to using print formatting, it is really versatile.
--
-=- Larry -=-
More information about the Python-list
mailing list