2

Straight to the point, here's the original code:

integer = input(...)
size = len(integer)

# Created three dicts for matching numbers as keys to corresponding words
dic_ones = {...}
dic_tens = {...}
dic_tys = {...}


# Created one main function digits_length within which I called functions(ones,tens,hundreds,thousands,millions,billions)
# based on the input length
def ones(num):
    print(dic_ones[num], end="")


def tens(num):
    if num//10 == 1:
        print(dic_tens[num], end="")
    elif num % 10 == 0:
        print(dic_tys[num], end="")
    else:
        print(dic_tys[num-(num % 10)], end="")
        ones(num % 10)


def hundreds(num):
    if num % 100 == 0:
        ones(num//100)
        print(" Hundred")
    else:
        ones(num//100)
        print(" Hundred", end="")
        digits_length(len(str(num % 100)), num % 100)


def billions(num):
    ones(num//1000000000)
    print(" Billion", end="")
    if num % 1000000000 != 0:
        digits_length(len(str(num % 1000000000)), num % 1000000000)


def millions(num):
    digits_length(len(str(num//1000000)), num//1000000)
    print(" Million", end="")
    if num % 1000000 != 0:
        digits_length(len(str(num % 1000000)), num % 1000000)


def thousands(num):
    digits_length(len(str(num//1000)), num//1000)
    print(" Thousand", end="")
    if num % 1000 != 0:
        digits_length(len(str(num % 1000)), num % 1000)


def digits_length(length, number):
    if length == 1:
        ones(number)
    elif length == 2:
        tens(number)
    elif length == 3:
        hundreds(number)
    elif 7 > length > 3:
        thousands(number)
    elif 10 > length > 7:
        millions(number)
    else:  # else applies to length 10 or billions
        billions(number)


# ones and tens are the smallest magnitudes so they do not include recursive call to digits_length
digits_length(size, int(integer))

(I replaced some irrelevant parts with ...).

Here's my new, refactored code:

class FormatNumber():
    @staticmethod
    def create_maps(start=0, stop=10, skip=None, names=()):
        if skip:
            return {k: v for k, v in zip(range(start, stop, skip), names)}
        return {k: v for k, v in zip(range(start, stop), names)}

    # Created three dicts for matching numbers as keys to corresponding words
    dict_ones = create_maps(1, 10, names=["One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight", "Nine"])

    dict_tens = create_maps(10, 20,
                            names=[" Ten ", " Eleven ", " Twelve ", " Thirteen ", " Fourteen ", " Fifteen ", " Sixteen ",
                                   " Seventeen ", " Eighteen ", " Nineteen "])
    dict_tys = create_maps(20, 100, 10,
                           names=[" Twenty ", " Thirty ", " Forty ", " Fifty ", " Sixty ", " Seventy ", " Eighty ",
                                  " Ninety "])

    def __init__(self, number):
        self.num = number

    def ones(self, num):
        print(self.dict_ones[num], end="")

    def tens(self, num):
        if num // 10 == 1:
            print(self.dict_tens[num], end="")
        elif num % 10 == 0:
            print(self.dict_tys[num], end="")
        else:
            print(self.dict_tys[num - (num % 10)], end="")
            self.ones(num % 10)

    def hundreds(self, num):
        # print(type(num))
        if num % 100 == 0:
            self.ones(num // 100)
            print(" Hundred")
        else:
            self.ones(num // 100)
            print(" Hundred", end="")
            self._format_number(num % 100)

    def billions(self, num):
        self.ones(num // 1000000000)
        print(" Billion", end="")
        if num % 1000000000 != 0:
            self._format_number(num % 1000000000)

    def millions(self, num):
        self._format_number(num // 1000000)
        print(" Million", end="")
        if num % 1000000 != 0:
            self._format_number(num % 1000000)

    def thousands(self, num):
        self._format_number(num // 1000)
        print(" Thousand", end="")
        if num % 1000 != 0:
            self._format_number(num % 1000)

    def _format_number(self, number):
        length = len(str(number))
        if length == 1:
            self.ones(number)
        elif length == 2:
            self.tens(number)
        elif length == 3:
            self.hundreds(number)
        elif 7 > length > 3:
            self.thousands(number)
        elif 10 > length > 7:
            self.millions(number)
        else:  # else applies to length 10 or billions
            self.billions(number)

    def format_number(self):
        self._format_number(self.num)


if __name__ == '__main__':
    integer = int(input("Enter non-negative integer  number which should not begin with zero"))
    english_repr = FormatNumber(integer)
    english_repr.format_number()

Is refactoring in this case a good idea? Or should the separate ones(), tens(), hundreds(), …etc. stay as functions? Also I would greatly appreciate anyone pointing out any other structural weaknesses in my code and potential fixes (I considered posting this on code review but I decided that for the structure part I would ask here).

Note: The refactored code is actually my answer to a question on codereview.stackexchange, but it created some controversy as to whether or not my refactoring is suitable for this case. Thus, I asked here in hopes of knowing if refactoring is a good idea in this case.

Improve this question
2
  • 8
    I don't see a clear benefit to use a class in this case. Having to initialize a class just to print a number seem like unnecessary overhead. The file itself serves as a unit of encapsulation so the class does not give you any further encapsulation. The second example does have the advantage it can be used as a library (because the input is inside a if __name__ == '__main__') but this is unrelated to the use of a class. Commented Oct 2, 2024 at 6:03
  • 1
    Is this question intended to be about Python only?  (Some answers are clearly Python-specific; and while some of the principles might apply to other languages, both language features and common practice varies.) Commented Oct 3, 2024 at 20:22

3 Answers 3

Reset to default
14

All you want is a formatting function which takes a number as input and returns a string. This function requires a few constant (!) dictionaries, but if one avoids storing the input inside a member variable, I don't think there is a striking reason why more than one instance of the FormatNumber class is required. Moreover, the "print" statements should not be used inside the class. When there are helper functions needed inside, make them return strings as well, concatenate their result as required and let the caller process the result (maybe just print it or process it differently).

The changed API might be used like this:

 formatter = FormatNumber()
 english_repr1 = formatter.format(integer1)
 print(english_repr1)
 english_repr2 = formatter.format(integer2)
 english_repr3 = formatter.format(integer3)

Hence, all you really need here is a module to encapsulate the dictionaries, which serves as a common namespace for isolating the dictionaries in their own scope.

Now there are languages like Java or C# where modules are usually implemented by classes, often as static classes, since there is no special "module" element in the language (and even if there was, the code would probably not become simpler). In Python, however, that seems to be a little bit overengineered. Python provides a simple module mechanism, where a module is defined by the content of a source file, and putting code in classes always produces more boilerplate than just staying with a module.

Hence, I would probably prefer to put the original code into a file FormatNumber.py, which avoids the self boilerplate and is fully sufficient for this use case. The using code will then look like

import FormatNumber

english_repr1 = FormatNumber.format(integer1)
english_repr2 = FormatNumber.format(integer2)
...

For the general question when to prefer classes over modules, just look into these older posts:

Improve this answer
0
5

The answer to (almost) any question that contains the word "always" is always (See, that means almost always) No. There is no absolute truth here.

In your specific case, the functions are not self-contained (impure) as they access global state and do not operate on only their inputs. The discussion of why that is not a good idea has been held many times (wont go into detail Here), but hints to an answer:

Bundling the state with methods into a class, as you proposed, reduces the scope where the state is available and thus the program's complexity. In fact, this is what one programming paradigm is all about: object-orientation encapsulates state, hides it internally in objects, bundles it with methods that expose behavior. Thats not a bad idea.

There are other means to achieve the ultimate goal, which is to reduce complexity: you could follow a functional approach and pass the state/data as inputs to your function, so it does not require external "dependencies"/state.

One more point: grouping related things together is also a good idea. It is found in the principle that modules should have high internal cohesion, but be loosely coupled between eachother (the related things form one module). Following principles such as SOLID (mostly applied in OOP) will steer you towards your proposed refactoring.

Edit to answer the comment: which approach is to be preferred? That depends on your context (Team, Domain, language, mutability, ...). Imo, both are equal if applied correctly. But: Always Be Consistent.

Improve this answer
3
  • So is it generally a good idea in this case? Or are other methods (functional) better? Commented Oct 2, 2024 at 4:55
  • 6
    @LukeL: Your comment suggests that you've not really heard the core answer. Commented Oct 2, 2024 at 4:58
  • 1
    "the functions are not self-contained (impure) as they access global state" - I don't see any mutable global state there, they only seem to access constants. Apart from printing instead of returning, the functions seem pretty much pure to me. Commented Oct 3, 2024 at 15:21
4

The context of the code is not explicitly mentioned, but given that the original question was a simple programming exercise, in my opinion the solution provided introduces unnecessary complexity by using a class structure. For a problem of this nature, I would expect a more straightforward answer that solves the problem without resorting to "advanced" concepts. While classes can be beneficial for scalability and future improvements, they might make the code harder to understand at first glance, especially if the task doesn't explicitly call for such extensibility.

However, if the code is intended to be extended or improved in the future, introducing a class could be a reasonable choice. If there are plans for the code to evolve into a more complex solution, starting with a class-based approach could make future enhancements easier to implement.

Additionally (as mentioned by others already), the current implementation of the class misses some opportunities to fully leverage the strengths of object-oriented design, particularly encapsulation. For example, instead of relying on direct prints inside methods, returning values from methods and providing a cleaner interface for the user to interact with the formatted result would make the class more flexible and reusable. By better encapsulating the logic, the class could also hide its internal workings, making it easier to extend or modify the functionality without affecting the external interface.

Improve this answer
1
  • 1
    "more flexible and reusable"... and also (importantly) more testable :) Commented Oct 2, 2024 at 13:52

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.