Arithematic formatter - can somebody suggest any improvements in my code

Python
#1

Hello guys,
I have just finished the arithematic formatter challenge. I have tried to modularise the code and tried to write it in optimised way. Can someone please suggest me some improvements/optimisation in my code

Here is my code

def add(elements,display_results):
  #global output_string, first_row, second_row, third_row, fourth_row
  problem_length = max(len(elements[0]),len(elements[2]))
  arithmetic_arranger.first_row+=elements[0].rjust(problem_length+2) + "    "
  arithmetic_arranger.second_row+="+ "+(elements[2]).rjust(problem_length) + "    "
  arithmetic_arranger.third_row+='-'*(problem_length+2)+ "    "
  if display_results:
    arithmetic_arranger.fourth_row+=str(int(elements[0])+int(elements[2])).rjust(problem_length+2)+"    "

def substract(elements,display_results):
  #global output_string, first_row, second_row, third_row, fourth_row
  problem_length = max(len(elements[0]),len(elements[2]))
  arithmetic_arranger.first_row+=elements[0].rjust(problem_length+2) + "    "
  arithmetic_arranger.second_row+="- "+(elements[2]).rjust(problem_length) + "    "
  arithmetic_arranger.third_row+='-'*(problem_length+2) + "    "
  if display_results:
    arithmetic_arranger.fourth_row+=str(int(elements[0])-int(elements[2])).rjust(problem_length+2)+ "    "

def arithmetic_arranger(problems,display_results=None):
  output_string=""
  arithmetic_arranger.first_row=""
  arithmetic_arranger.second_row=""
  arithmetic_arranger.third_row=""
  arithmetic_arranger.fourth_row=""
  if len(problems)>5:
    return "Error: Too many problems."
  else:
    for problem in problems:
      elements=problem.split()
      if not elements[0].isnumeric() or not elements[2].isnumeric():
        return "Error: Numbers must only contain digits."
      if len(elements[0])>4 or len(elements[2])>4:
        return "Error: Numbers cannot be more than four digits."
      if '+' in problem:
        add(elements,display_results)
      elif '-' in problem :
        substract(elements,display_results)
      else:
        return "Error: Operator must be '+' or '-'."
  
  if display_results:
    output_string = arithmetic_arranger.first_row.rstrip() + "\n" +arithmetic_arranger.second_row.rstrip() +"\n"+arithmetic_arranger.third_row.rstrip()+"\n"+arithmetic_arranger.fourth_row.rstrip()
  else:
    output_string = arithmetic_arranger.first_row.rstrip() + "\n" +arithmetic_arranger.second_row.rstrip() +"\n"+arithmetic_arranger.third_row.rstrip()
      
  # print(output_string)
  return(output_string)
#2

It seems like really bad practice to write a function that changes a value in another function. Because at that point it’s not an independant module but literally needs the other function to be present in order to work.

Also given you only call add() and subtract() at one position… that’s like violations of two coding principles - both encapsulation AND reusability.
You don’t reuse the function at different points in the code AND you can’t import the function into another program and use it.

Not a big issue - but the else is not needed because return exits the function without executing the following code anyway.

1 Like
#3

Thanks for the feedback @Jagaya

#4

@Jagaya if I try to fix this both encapsulation AND reusability I cannot distribute my code in functions. I have to write everything in one function.

What are your thoughts about fixing this?

#5

Hm… good point… ok I guess if you got complex pieces of code it makes sense to write them in a seperate function.
Maybe take these rules more as guidelines ^^°
But even then should the return-value be utilized as much as possible for later debugging. Imagine having 6000 lines of code and any function could change values in another function - vs. every function has a single return-value.

It’s common practice to avoid global-variables for exactly that reason. And you basically turn values inside functions global.

The other thing is, you functions are identical except for the operation symbols (literally two characters in the entire things). That’s a bit much, given you even have that character in elements[1].

#6

Approach you have taken looks more fit for using class instead of function. I’d suggest however instead of doing that, to try to put changing data inside of data structure that’s separate from either function, and it’s only passed to - returned after made operations on, when needed.

2 Likes
#7

Hi,

output_string = arithmetic_arranger.first_row.rstrip() + "\n" +arithmetic_arranger.second_row.rstrip() +"\n"+arithmetic_arranger.third_row.rstrip()+"\n"+arithmetic_arranger.fourth_row.rstrip()

this syntax is hard to read to another coder, and maybe even for you a month later.

Use python f string syntax instead:

output_string = f"""{arithmetic_arranger.first_row.rstrip()}
{arithmetic_arranger.second_row.rstrip()}
{arithmetic_arranger.third_row.rstrip()}
{arithmetic_arranger.fourth_row.rstrip()}"""

Using this syntax highly improve the readibility of your code.

2 Likes
#8

Wow! This is Super cool… :heart_eyes: Thanks @lendoo . Learnt something new today :slight_smile:

#9

Yeah you got a point, Thanks.