Python - Do I need a class or are these functions okay?

Question

I'm fairly new to Python but had to start using it and for reasons I have to use 2.7, which is why I am using that syntax. I have this function which gets data from a file, if it exists, and then gets passed onto other functions to perform SQL queries etc. The script is only intended to work for the directory it is running in, hence the os.getcwd()

This static data is the only data that gets looked up then returned and referenced by other functions. It works as it is but would you say for professionalism and understanding python better that a class would be better here or a restructure in someway? I am of course only posting a snippet.

 currentdirectory = os.getcwd()
 versionf = "version.ini"
 verfile = os.path.join(currentdirectory, versionf)
 
 def main():
 get_local_details()
 push_app(application, version)
 do_something(app_name_version)
 
 def get_local_details():
 if os.path.isfile(verfile):
 def get_app_name():
 try: 
 global application
 application = subprocess.check_output(['grep', 'application', verfile]).strip('\n')
 application = application.rsplit('=')[1]
 return application
 except:
 print "Application missing from version.ini"
 def version_number():
 try: 
 global version
 version = subprocess.check_output(['grep', 'version', verfile]).strip('\n')
 version = version.rsplit('=')[1]
 return version
 except:
 print "version missing from version.ini"
 get_app_name()
 version_number()
 def app_name_version(application, version)
 try:
 global applicationver
 applicationver = application + "." + version
 return applicationver
 except:
 print "error here"
 app_name_version(application, version) 
 else:
 print "File does not exist."
main()

contents of ini file are

application=testapp
version=1.0

do_something is a function that contains a SQL query that uses the app_name_version as the WHERE statement.

Is it even possible to use a function within a class to define the attributes?

• Mast: The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly.
• Kate: First of all, Python 2.7 is end of life. Continuing development on that environment means accruing technical debt. Is it possible that the code you posted is truncated and incomplete ?
• Mast: do_something always looks fishy in a piece of code. What details are we missing? What does the INI look like?
• greybeard: new job and all their libs Are you aware that you put code under Creative Commons just by presenting it here? Are you in a position to do so?
• SimonT: @greybeard I haven't posted anything that includes private libs or details.

Answer 1

Wow, this is kind of a mess. I'll just review items in sequence as I read them. My short answer, tl;dr: Yes!

Try executing python 2.7 code with a python 3 interpreter, even if only during development on your laptop, or in unit tests. It will give you the confidence to import six or otherwise drag ancient code kicking and screaming into the modern era. Small changes like print x → print(x) are easy and helpful.

The first three lines are nice. We define currentdirectory, versionf, & verfile as top-level module constants. PEP-8 prefers the name current_directory, but I quibble, it's fine as-is.

You conditionally def or print. I recommend an early abort:

 if not os.path.isfile(verfile):
 raise FileNotFoundError(verfile)

You have unfortunate coupling in this code. Python, like all O-O languages, offers you structuring tools to improve cohesion. Please avoid the global keyword, it is a bad code smell. And please avoid nested def, as its effect on namespaces is similar to using global variables. For private helpers, choose _private names like def _get_app_name, def _version_number, and def _app_name_version.

You wrote:

application = subprocess.check_output(['grep', 'application', verfile]) ...

In several places you reference verfile. Just listen to your code. Clearly it is telling you it wants an __init__ constructor which assigns to self.verfile, so that it may be used here. Also, fork'ing grep is a tool one might use, but it's not the most appropriate one for the task at hand. Let me introduce you to import configparser.

 global application
 ...
 return application

Ok, you used to have 3 module-level variables, and now you've added a 4th one. Sadly, you have scattered them all around the module, and you will continue to define additional ones. This is poorly structured. There's no central place for a maintenance engineer to discover "what are the important pieces of state I need to worry about?"

Also, not sure why, after side effecting the variable, you bothered to return it, since caller ignores the return value. Rather than no-arg invocations of get_app_name and version_number, you might consider passing in the values they need as arguments. But if you turn this into a class, then self can supply those values.

 except:
 print ...

Better for your catch to be except Exception, lest you accidentally catch CTRL-C exceptions, as well. Consider using a logger.

Then we continue to see a pattern of "more of the same", I won't belabor it with repetition. You are consistently abusing namespaces, relying on implicit args, and side effecting globals at module-level.

It's not quite clear from the generic code you posted, but I'm going to go out on a limb and say that this module seems to be about "app verification".

---

Please do this:

class AppVerifier:

and then define half a dozen object attributes as I suggested above. Doing just that one thing will have a huge effect on making your code more maintainable for the next engineer that has to interact with it.

• Peilonrayz: Hmm, you've said "print x → print(x)" without explaining why it works. Additionally the better choice to just import that functionality from __future__. This makes me think this is a poor answer from the get go.
• Roland Illig: Why didn't you mention 2to3?