3
\$\begingroup\$

I am making a function to return a two-item tuple of ('os basename', 'os distro'). For instance, the return for Vista should be ('Windows', 'Vista') and ('Linux', 'Fedora') for Fedora.

Does this parse OS names correctly? Is this great, good or ugly functionally wise, and code-wise? Is there a flaw in any given ideal circumstance?

(Tested only on Vista SP2, Python 2.7.)

import sys, platform
PLATFORM_SYSTEM, PLATFORM_NODE, PLATFORM_RELEASE, PLATFORM_VERSION, PLATFORM_MACHINE, PLATFORM_PROCESSOR = platform.uname()

VER_NT_WORKSTATION = 1
VER_PLATFORM_WIN32_NT = 2
WIN_VERSIONS = {
#major, minor, platform, product_type
    (5, 1, VER_PLATFORM_WIN32_NT, VER_NT_WORKSTATION): 'XP',
    (5, 2, VER_PLATFORM_WIN32_NT, VER_NT_WORKSTATION): '2003',
    (6, 0, VER_PLATFORM_WIN32_NT, VER_NT_WORKSTATION): 'Vista',
    (6, 1, VER_PLATFORM_WIN32_NT, 0): 'Server 2008',
    (6, 1, VER_PLATFORM_WIN32_NT, VER_NT_WORKSTATION): '7',
    (6, 2, VER_PLATFORM_WIN32_NT, 0): 'Server 2012',
    (6, 2, VER_PLATFORM_WIN32_NT, VER_NT_WORKSTATION): '8',
    (6, 3, VER_PLATFORM_WIN32_NT, VER_NT_WORKSTATION): '8.1',
    (6, 3, VER_PLATFORM_WIN32_NT, 0): 'Server 2012 R2'
}
def ostuple():
    """return a two string tuple of basename of os and distro name, ie: ('Windows', 'Vista')"""
    ret = [PLATFORM_SYSTEM] #platform.uname()[0]
    if os.name == 'nt':
        ver = sys.getwindowsversion()
        major, minor, platform, product_type = (ver.major, ver.minor, ver.platform, ver.product_type)
        for t in WIN_VERSIONS:
            m, n, p, y = t
            if major == m:
                if minor == n:
                    if platform == p:
                        if y == 0:
                            if product_type != VER_NT_WORKSTATION:
                                ret += [WIN_VERSIONS[t]]
                        else:
                            ret += [WIN_VERSIONS[t]]
    else:
        if hasattr(platform, 'linux_distribution'):
            y = list(platform.linux_distribution(full_distribution_name=0))
            for k in y[:]:
                if not k: #to remove empty strings
                    y.remove(k)
            if y:
                ret += [' '.join(y)]
    if len(ret) < 2:
        ret += ['']
    return tuple(ret)
\$\endgroup\$
0
2
\$\begingroup\$

Some initial comments:

  • As provided, the code doesn’t run, because it’s missing an import os line.
  • When I tried to run this on my system, it barfed:

    Traceback (most recent call last):
      File "platformname.py", line 50, in <module>
        print ostuple()
      File "platformname.py", line 40, in ostuple
        y = list(platform.linux_distribution(full_distribution_name=0))
    UnboundLocalError: local variable 'platform' referenced before assignment
    

    I think this is because there’s a platform variable in the os.name == 'nt' branch, and Python is trying to look up the local variable in this line. That hasn’t been set in this branch, so it all falls over.

    Changing the variable name in the first branch made the problem go away. This is why you should choose names that don’t conflict with builtins and/or modules that you’ve imported.

  • Once I had it running, this is the output of ostuple():

    ('Darwin', '')
    

    Pop quiz: what OS am I running?

    I’m running Mac OS X. Darwin is the name of the Unix operating system that sits underneath OS X, but I don’t know if that’s particularly well-known outside the Mac community.

    Depending on where this function will be used, that may or may not be an acceptable result. Just bear in mind that this particular result may be unintelligible to many people.

Now, some comments on the code itself:

  • There’s nothing to tell me how this works. There are no comments or explanation for what’s going on. Since I’m not psychic, this makes it very difficult for me to debug your code.

    As a corollary, using single-letter variable names is terrible for readability. Names should describe their purpose/contents – good names make it much easier to read code.

  • When you’re doing tuple unpacking, common practice is to use an underscore (_) to mark parts of the tuple that you’re not going to use. When you unpack platform.uname(), you’re only actually interested in PLATFORM_SYSTEM, so the other components can be discarded:

    PLATFORM_SYSTEM, _, _, _, _, _ = platform.uname()
    

    This reduces noise in your code.

  • Rather than putting everything in a single monolithic function, I’d break it down into smaller, per-OS family functions. For example, get_windows_version(), get_linux_version(), get_android_version(), and so on, then let ostuple() choose the appropriate function to call. Putting all your code in a single version is bad because it makes it harder to reuse code.

  • Rather than nesting multiple if statements, consider combining them into a single condition; compare

    if major == m:
        if minor == n:
            if p2 == p:
    

    to

    if (major == m) and (minor == n) and (p2 == p):
    
\$\endgroup\$

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.