current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
1

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)
share|improve this question

migrated from stackoverflow.com Sep 19 '14 at 15:19

This question came from our site for professional and enthusiast programmers.
    
Whats up with the for k in y[:] and if not k: construct? Why do you do that? – holroy Dec 5 at 2:43
    
Removal from a list when iterating it is an error, but removal iterating a copy is not. As an added note, I should have called this 'parsing better windows OS variants' as there is no dict that hashes strings for nix builds. In this coding atmosphere i would believe many dicts like WIN_VERSIONS for other OSes would be needed to be more complete. – user1576916 yesterday
    
Your ostuple() is quite close to being a wintuple()... :-) – holroy yesterday

1 Answer 1

up vote 2 down vote

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):
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.