Performing SMART tests on many disks

So I'm relatively new to programming in general, and I've finally strapped in and started learning Python. This is my first significant project using it and I'm trying to build good habits with layout of my code, readability, comments, when to make functions, when not to etc.

The problem I was solving was that I have about 20 hard drives that I wanted to run the same tests on using smartmontools on my ubuntu server system and make nice files detailing their status/health before I put them into a cluster system.

Some specific concerns of mine are:

-
Overall workflow. Did I implement use of my main() function correctly? I figured some functions that I could use outside of this script I left outside of main() so I could possibly call them in other scripts. But left ones specific to main() inside.

-
Commenting. More? Less? This is just for me for now, but would you be able to see whats going on?

-
I do lots of string manipulation and it looks sorta messy to me at a glance. Are there cleaner/easier ways to do this?

Any other critiques are welcome!

```
#!/usr/bin/python
##----------------
##Script Name: hd-diag
##Description of Script: Runs diagnostics on harddrives and writes results to
##file
##Date: 2013.07.09-11:48
##Version: 0.1.0
##Requirements: python2, smartmontools, posix environment
##TODO: add support for other incorrect input in YN questions
##----------------

import os
import sys
import subprocess
import getopt

def unix_call(cmd):
'''(str) -> str

Calls unix terminal process cmd and returns stdout upon process completion.
'''

output = []
global errordic
p = subprocess.Popen(
cmd, shell=True,
stdout=subprocess.PIPE, stderr=subprocess.PIPE)
for line in iter(p.stdout.readline, ''):
output.append(line.strip('\n'))
if p.stderr:
for line in iter(p.stderr.readline, ''):
errordic['unix_call'] = line.strip('\n')
return '\n'.join(output).strip("'[]'")

def test_call(cmd)

Don't open files as this. Its a bad habit. It is easy to forget to close it.

to_file = open(destination, 'w')

Use it as this.

def do_write(string, destination):
    with open(destination, 'w') as to_file: 
        for item in string:
            to_file.write(item)

Also when you are passing a string then there is no need to use str again. Redundant.

You are making redundant elif checks. Obviously if '/' not in filenamepath: is False then you don't need to check whether it is in filepath. Use else instead.

Use os.path.join instead of string manipulation to join paths. It is os independent. Also checking for / isn't a good idea. You can use os.sep which again is os independent. But it would be a good idea to make sure that filename does not contain / by itself.

In the comments you are saying that full path must be given but in the function you are checking for that. Why? Decide what you want the function to do. It does both? Then you can use this

def write_str_to_file(string, filenamepath):
    '''Writes the contents of str to file as array.

    Assumes current directory if just filename is given
    otherwise full path must be given
    '''
    if os.sep not in filenamepath:
        dest = os.path.join(os.getcwd(), filenamepath)
    else:
        dest = filenamepath

    with open(dest, 'w') as to_file:
        for item in string:
            to_file.write(str(item))

Don't define global variables in the middle of nowhere. It makes it harder to find them. Put all globals at the top. Don't use global variables if you don't have to. Declare them in the main function and pass them around. You can return more than one values so problems with passing and returning all of them. Having global variables when you don't need them shows bad design. It might be necessary sometimes but it doesn't seem to be the in this case.

You don't need the additional variable test_results_raw in case of test_call. You can instead use

def test_call(cmd):
    p = subprocess.Popen(
        cmd, shell=True, stdout=subprocess.PIPE)
    return p.stdout.read()

Is the function usage actually necessary in the main function. You can define a constant at the beginning of the module and print that instead. Function calls are expensive and doing them for 1 print statement doesn't seem to be a good idea.

Again in the main function you are using if outputloc != '' and then elif outputloc == ''. It can be a simple else. Same goes for the nested if and elif. Checking for more conditions costs you in performance.

When checking for enableYN you can use enable.lower()[0] == 'y' instead of using enableYN in ('yYes'). In that case it checks for e and s also. People would be entering y, Y, yes or some other variation but with a y in the beginning. Same goes for checking for no.

I skipped a lot of the code because it is quite big.

Hope this helped.

About me being able to see what's going on by the comments. Not really. You missed what the main function is doing

About the strings being messy. Yes, they are. Try to change the things I have mentioned and I'll take a look again. Add your updated code like done in this question instead of modifying it.

StackExchange Code Review Q#28794, answer score: 2