8
\$\begingroup\$

I need to convert an amount to an Indian currency format:

import decimal
def currencyInIndiaFormat(n):
  d = decimal.Decimal(str(n))
  if d.as_tuple().exponent < -2:
    s = str(n)
  else:
    s = '{0:.2f}'.format(n)
  l = len(s)
  i = l-1;
  res = ''
  flag = 0
  k = 0
  while i>=0:
    if flag==0:
      res = res + s[i]
      if s[i]=='.':
        flag = 1
    elif flag==1:
      k = k + 1
      res = res + s[i]
      if k==3 and i-1>=0:
        res = res + ','
        flag = 2
        k = 0
    else:
      k = k + 1
      res = res + s[i]
      if k==2 and i-1>=0:
        res = res + ','
        flag = 2
        k = 0
    i = i - 1

  return res[::-1]

def main():
  n = 100.52
  print "INR " + currencyInIndiaFormat(n)  # INR 100.52
  n = 1000.108
  print "INR " + currencyInIndiaFormat(n)  # INR 1,000.108
  n = 1200000
  print "INR " + currencyInIndiaFormat(n)  # INR 12,00,000.00

main()

Is there a way to make my currencyInIndiaFormat function shorter, more concise and clean? Is there a better way to write my currencyInIndiaFormat function?

Indian Currency Format:

For example, numbers here are represented as:

1
10
100
1,000
10,000
1,00,000
10,00,000
1,00,00,000
10,00,00,000

Refer: Indian Numbering System

\$\endgroup\$
0

4 Answers 4

8
\$\begingroup\$

1. Style guides

  • leave two empty lines before each function (when you're not inside of a class)
  • Python is not C; don't use ;
  • put a space before and after operands. i>=0 will become i >= 0, s[i]=='.' will become s[i] == '.' and so on.
  • use the snake_case notation for function names: currencyInIndiaFormat(..) will become currency_in_indian_format(..)
  • use format() instead of + notation: e.g: print "INR " + currencyInIndiaFormat(n) might be print "INR {}".format(currencyInIndiaFormat(n))
  • also, I'd recommend you use print('w/e you want here') so that your code can be python 3 compatible.
  • use 4 spaces for indentation instead of 2.
  • use augmented assignments where you can: res = res + s[i] can be written as res += s[i]
  • add docstrings to your functions to describe what their purpose are

If you want to read more about Python style guide, you should start from here.

Preview:

import decimal


def currency_in_indian_format(n):
    """ Convert a number (int / float) into indian formatting style """
    d = decimal.Decimal(str(n))

    if d.as_tuple().exponent < -2:
        s = str(n)
    else:
        s = '{0:.2f}'.format(n)

    l = len(s)
    i = l - 1

    res, flag, k = '', 0, 0
    while i >= 0:
        if flag == 0:
            res += s[i]
            if s[i] == '.':
                flag = 1
        elif flag == 1:
            k += 1
            res += s[i]
            if k == 3 and i - 1 >= 0:
                res += ','
                flag = 2
                k = 0
        else:
            k += 1
            res += s[i]
            if k == 2 and i - 1 >= 0:
                res += ','
                flag = 2
                k = 0
        i -= 1

    return res[::-1]


def main():
    n = 100.52
    print("INR {}".format(currency_in_indian_format(n)))  # INR 100.52
    n = 1000.108
    print("INR {}".format(currency_in_indian_format(n)))  # INR 1,000.108
    n = 1200000
    print("INR {}".format(currency_in_indian_format(n)))  # INR 12,00,000.00


if __name__ == '__main__':
    main()

NOTE: I've also added if __name__ == '__main__'. By doing the main check, you can have that code only execute when you want to run the module as a program and not have it execute when someone just wants to import your module and call your functions themselves.

2. Algorithm

As I said in the comments, I would strongly recommend using the existing builtin module locale, which is doing exactly what you want:

Example code (it's working if you have the locale set on your machine - so that would be an ugly constraint):

>>> import locale
>>> locale.setlocale(locale.LC_ALL, 'en_IN.utf8')  # if this doesn't work, you might try as well: locale.setlocale(locale.LC_ALL, '')
'en_IN'
>>> locale.format("%d", 1255000, grouping=True)
'12,55,000'

I'll let others to comment on your code regarding the efficiency of the algorithm you implemented and methods to make it shorter / better.

\$\endgroup\$
2
  • \$\begingroup\$ Python is not C; don't use ; I don't see any semicolons there. Am I missing something? \$\endgroup\$ Apr 18, 2018 at 13:33
  • \$\begingroup\$ @RaimundKrämer i = l-1; :) \$\endgroup\$ Apr 18, 2018 at 13:37
3
\$\begingroup\$

Working with strings rather than numbers here is an anti-pattern. It's more natural to use division and modulo to "group" digits together rather than slicing their string representation. The divmod builtin function helping to neater you code with that.

You also had the right move using Decimal instead of float as it will take care of removing superfluous decimals when using divmod instead of forcing you to use floats all the way down.

A first rewrite could look like:

def indian_formatted_currency(amount):
    amount = decimal.Decimal(str(amount))
    amount, thousands = divmod(amount, 1000)
    groups = [thousands]
    while amount:
        amount, group = divmod(amount, 100)
        groups.insert(0, group)

     return ','.join(map(str, groups))

But it is somewhat inneficient as insert(0, ...) on a list is \$O(n)\$. It might be enough for your needs as you might handle numbers with less than 10 groups. But if you want better alternatives, you can either use a collection.deque where left insertion is \$O(1)\$ or use append and then reverse the list:

def indian_formatted_currency(amount):
    amount = decimal.Decimal(str(amount))
    amount, thousands = divmod(amount, 1000)
    groups = [thousands]
    while amount:
        amount, group = divmod(amount, 100)
        groups.append(group)

     return ','.join(map(str, reverse(groups)))
\$\endgroup\$
2
  • \$\begingroup\$ This won't work for some numbers. For eg. 3306249. The result is 33,6,249 \$\endgroup\$ Apr 3, 2018 at 6:21
  • \$\begingroup\$ @AdityaSatyavada you're right, you can change str in the map call at the last line into {:02}.format to account for that but need a special case for the last 3 digits. Will edit when I get some time for a clean one. \$\endgroup\$ Apr 6, 2018 at 7:21
1
\$\begingroup\$

you can often avoid a while loop, index variables and flag variables and make it more readable:

def split_on_n(s, n):
  return s[:n], s[n:]

# from http://stackoverflow.com/questions/312443/how-do-you-split-a-list-into-evenly-sized-chunks
def chunks(l, n):
  """Yield successive n-sized chunks from l."""
  for i in range(0, len(l), n):
    yield l[i:i + n]


def currencyInIndiaFormat(n):
  d = decimal.Decimal(str(n))
  if d.as_tuple().exponent < -2:
    s = str(n)
  else:
    s = '{0:.2f}'.format(n)

  def inner():
    parts = []
    before_point, after_point = s.split('.')
    parts.extend([after_point, '.'])

    remainder, last3digits = split_on_n(before_point, -3)
    parts.append(last3digits)
    if len(remainder) == 0:
      return parts
    # correct the chunking for the odd part
    odd_part, remainder = split_on_n(remainder, len(remainder) % 2)
    remaining_parts = list(chunks(remainder, 2))
    parts.append(",")
    if len(odd_part) > 0:
      remaining_parts = [odd_part] + remaining_parts
    parts.append(",".join(remaining_parts))
    return parts

  return ''.join(reversed(inner()))
\$\endgroup\$
1
  • \$\begingroup\$ It's better to explicitly pass s to inner() rather than using it as a global variable. \$\endgroup\$ Apr 18, 2018 at 15:56
0
\$\begingroup\$

Your variable names should be more explicit. Most of them are a single character, and even the multicharacter names are rather opaque. flag, for instace, would be more clear if named something else, such as number_position. You're using integers for flag values, but it's not really being used as an integer. Your code would be more readable if you assigned it strings describing what each state consists of, such as number_position = 'right_of_decimal_point'. You're using conditionals where a case statement would be more appropriate. Your last case sets flag to 2, but I don't see how you get to that unless flag is already 2. However, since you're progressing through each of the values of flag, the whole structure of the while loop is inappropriate. You have three things you want to do, and the second always happens after the first, and the third after the first and second. The natural things to do is:

#do first thing
#do second thing
#do third thing

Instead, you're doing:

while condition:
   if havent_done_first_thing:
      #do first thing
   elif havent_done_second_thing:
      #do second thing
   else:
      #do third thing

In your first thing, you're checking each character one by one to see whether it's a decimal point. Using string functions, you can just do:

decimal_part, integer_part = s.split('.')

In your second thing, you're trying to take the last three digits. So just do that:

if len(integer_part) < 4:
    return("{}.{}".format(integer_part,decimal_part)
else:
   thousands_part = integer_part[-3:]
   hundreds_part = integer_part[:-3]

Now you just have to split the hundreds part into 2-digit sections. Instead of doing this one digit at a time, you can do it two digits at a time.

digit_index = len(hundreds_part)
while digit_index > 2:
    res = res + hundreds_part[digit_index-1:digit_index]
    digit_index = digit_index - 2
res = res + hundreds_part[0:digit_index]

You could do this treating the number as a decimal object rather than a str, but I don't see that treating it as a str is all that unnatural.

\$\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.