6
\$\begingroup\$

Python 3.7.0

from sys import argv
class Version:
    def __init__(self, ver_str):
        if type(ver_str) != 'string':
            ver_list = ver_str.split('.')
            self.Major = int(ver_list[0])
            self.Minor = int(ver_list[1])
            self.Patch = int(ver_list[2])
            self.Build = int(ver_list[3])

    def __repr__(self):
        return "{}.{}.{}.{}".format(self.Major, self.Minor, self.Patch, self.Build)
    def __lt__(self, other):
        if other.Major != self.Major:
            return self.Major < other.Major
        elif other.Minor != self.Minor:
            return self.Minor < other.Minor
        elif other.Patch != self.Patch:
            return self.Patch < other.Patch

        else:
            return self.Build < other.Build 

    def __gt__(self, other):
        if other.Major != self.Major:
            return self.Major > other.Major
        elif other.Minor != self.Minor:
            return self.Minor > other.Minor
        elif other.Patch != self.Patch:
            return self.Patch > other.Patch

        else:
            return self.Build > other.Build 

    def printme(self):
        print("{}.{}.{}.{}".format(self.Major, self.Minor, self.Patch, self.Build))


def main(argv):
    validate_args(argv)

    ver_1 = Version(argv[1])

    ver_2 = Version(argv[2])
    op = argv[3]

    if op == '<':
        print("{} < {}: {}".format(ver_1, ver_2, ver_1 < ver_2))
    elif op == '>':
        print("{} > {}: {}".format(ver_1, ver_2, ver_1 > ver_2))
    else:
        print("Incorrect operator")
        exit(-1)

def validate_args(argv):
    no_of_args = len(argv)
    if no_of_args != 4:
        print("USAGE: {} 1.1.1.1 2.2.2.2 '<' or '>'".format(argv[0]))
        exit(-1)
    if (len(argv[1].split('.')) != 4) or (len(argv[2].split('.')) != 4):
        print("USAGE: {} 1.1.1.1 2.2.2.2. '<' or '>' IMPROPER VERSION FORMAT".format(argv[0]))
        exit(-1)
    if argv[3] != '>' and argv[3] != '<':
        print("USAGE: {} 1.1.1.1 2.2.2.2. '<' or '>' IMPROPER OPERATOR".format(argv[0]))
        exit(-1)

if __name__ == '__main__':
    main(argv)
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

First of all, have you looked at LooseVersion and/or StrictVersion from distutils.version? They might already provide you with exactly what you need.


General “PEP-8” Comments:

You should have a blank line after your import, after class Version, and after the def __repr__(self) method. The blank-lines inside of __lt__ and __gt__ functions before the else: clause are inconsistent.

Your class and methods could benefit from """doc strings""".

The class members Major, Minor, Patch and Build should all be lower case. Only class names should begin with capital letters. Since the members should be private to the class, they should also be prefixed with an underscore.


The constructor will silently fail if type(ver_str) != 'string' is ever False, and then the instance becomes useless, as the members which are required in every other method of the class become reference errors. There is no need to check if ver_str is a string; just try to split it, and convert the parts to integers. If you haven’t passed in a string, the split or conversion to integers will cause an exception. (You could add a check that the split produces exactly 4 parts, to reject a version such as "1.2.3.4.5.6".)

Fortunately (as noted by @200_success in the comments), type(ver_str) will never equal 'string', because type(...) doesn't return strings; it returns types. The code only works because the test inverts the condition, and reads != when the intention is for the type to == a string. The correct test would be closer to if isinstance(ver_str, str):, but again explicit type checking is an anti-pattern.

Without the explicit type checking, and using a more functional approach, the constructor could be written as a one line method. As a side-effect of the deconstruction assignment, the version string must contain exactly 4 parts or an exception will be raised.

def __init__(self, ver_str):
    self.Major, self.Minor, self.Patch, self.Build = map(int, ver_str.split('.'))

printme() is unused and could be deleted. If you don’t wish to delete it, at least name it using snake_case to make it more readable: print_me(). You could write the function as simply print(repr(self)) to avoid duplicating code.


__repr__(self) should actually return a string like Version("1.2.3.4"), not the string "1.2.3.4" to conform to the repr() contract. What you have written would make a great __str__(self) method though, and you could then implement __repr__(self) using the str() method.


Inside def main(...):, you are using argv. This is a different argv, shadowing the one you import from sys using from sys import argv.

You should avoid shadowing the imported name, by using:

import sys

# ....

if __name__ == '__main__':
    main(sys.argv)

There is no need to exit(-1) from main(), since the program will terminate anyway immediately. exit() will terminate the Python interpreter, which can be unexpected, and can cause resources to not be properly closed. A unit test framework will abruptly terminate, preventing recording of the test results, and preventing other unit tests from running. In short, don’t ever use exit(). Ever.


Since you are explicitly writing for Python 3.7.0, you can use f-strings. This means you can replace many of your .format(...) calls with a more compact, readable representation where the substitution location and value are combined. Ie:

    print("{} < {}: {}".format(ver_1, ver_2, ver_1 < ver_2))

could become simply:

    print(f"{ver_1} < {ver_2}: {ver_1 < ver_2}")
\$\endgroup\$
2
  • 2
    \$\begingroup\$ The type(ver_str) check is even more bizarre, when you consider that type('1.2.3.4') == 'string' if False. I have no idea what that check in the initializer is intended to accomplish. \$\endgroup\$ Apr 24, 2019 at 17:26
  • \$\begingroup\$ @200_success Wow. I knew the explicit type-checking was bad, but I totally missed that it was outright wrong. \$\endgroup\$
    – AJNeufeld
    Apr 24, 2019 at 17:33
4
\$\begingroup\$

Python tuples already sort the way you want to, so you can replace your class with just a thin wrapper around the built-in tuple class. You only need a constructor that can parse a string of the form "3.1.0.2" into a tuple of ints and a nice representation as a version string:

class VersionTuple(tuple):
    def __new__(cls, s):
        return super().__new__(cls, map(int, s.split(".")))

    def __repr__(self):
        return ".".join(map(str, self))

Note that this does not restrict the number of arguments to include a major, minor, patch and build version. If you want that, you can instead inherit from a collections.namedtuple:

from collections import namedtuple

class VersionTuple(namedtuple("VersionTuple", "major minor patch build")):
    def __new__(cls, s):
        return super().__new__(cls, *map(int, s.split(".")))

    def __repr__(self):
        return ".".join(map(str, self))

Both work the same way with respect to comparisons (you even get the equality operator for free):

VersionTuple("3.1.0.0") < VersionTuple("3.1.2.0")
# True
VersionTuple("1.0.0.0") > VersionTuple("3.1.2.0")
# False
VersionTuple("1.0.0.0") == VersionTuple("1.0.0.0")
# True

And printing:

print(VersionTuple("3.6.3.0"))
# 3.6.3.0

While the latter allows you to also access the individual parts:

VersionTuple("3.6.3.0").patch
# 3

It also raises a TypeError if the version string does not contain four parts:

VersionTuple("3.6.3")
# ...
# TypeError: __new__() missing 1 required positional argument: 'build'
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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