Some guidelines to improve code quality #3
Comments
|
This is a lot of very useful tips you have provided. The code badly needed a clean-up, but I did not know where to start. I was thinking about making it modular a.k.a separating the code into many sub-files and importing them when necessary. Your approach seems promising 👍 . Defining a function to replace the repetition of code should be the first priority. It greatly help readability. Sparse is better than dense should be taken into account. Thanks a bunch for this detailed Issue. I will get back to you, if I land up in any trouble while re-writing the code. I also would like to know about your views on breaking the code to many sub-files instead of one long |
|
Definetely, it will be. You can make a folder (package) for graphical things like windows (for ex. SimpleMainInterface and AdvancedMainInterface) and for styling purposes (for ex. make your own buttons by inheriting from tk buttons with future customizations). Also make a core package with a class for calculation. The idea is to separate gui and core functionality for independent testing and development. I see the file hierachy as You need to make your windows implementing singleton pattern for some reasons. You can read about it here: singleton. It would help you to store the state of your window. Ask me if you have any questions and keep developing! |
|
In the process of binding control elements, def control_binding():
for control_element in [str(x) for x in range(10)]: # or your controls list
some_button= ttk.Button(keypad, text=control_element, command=lambda: cout(control_element))
# or append this buttons to button list and like button_list.append(some_button)To # earlier code (code to be refactored)
nine.grid(row=2, column=0)
eight.grid(row=2, column=1)
seven.grid(row=2, column=2)
six.grid(row=3, column=0)
five.grid(row=3, column=1)
four.grid(row=3, column=2)
three.grid(row=4, column=0)
two.grid(row=4, column=1)
one.grid(row=4, column=2)# refactored code
...
def control_binding(self):
for control_element in [str(number) for number in range(10)]:
some_button = ttk.Button(keypad, text=control_element, command=lambda: self.cout(control_element))
self.NUM_BUTTONS.append(some_button)
def gridding(self):
ROW = 1
for button_place in range(len(NUM_BUTTONS):
MOD = button_place%3
if MOD == 0:
ROW += 1
NUM_BUTTONS[button_place].grid(row=ROW, column = mod)
...Is this the correct approach to do it? |
|
Exactly! In the case of number buttons your can shorten gridding code as def gridding(self):
for button_index in range(len(num_buttons)):
num_buttons[button_index].grid(row=button_index//3, column=button_index%3) # or row=button_index//3 + 2 if you need to start from 2Most programming languages have own code style guides/naming conventions. In python the most considered style guide is PEP-8. According to this all uppercase variable names should refer to constant objects, for ex.: |
|
Nested ...
if char in OPERATORS:
if STORAGE[-1] == None:
if char in OPERATORS[:2]:
''' Case when first input is '/' or '*' '''
STORAGE += ['1']+list(char)
else:
''' Case when first input is '+' or '-' '''
STORAGE += ['0']+list(char)
elif STORAGE[-1] in OPERATORS:
''' Switching to latest operator, example: '3+-' evalutes to '3-' '''
STORAGE[-1] = char
else:
STORAGE += list(char)
else:
...
...The next following code is an attempt to make the nested block flat: ...
# if block
if char in OPERATORS and STORAGE[-1] is None and char in OPERATORS[:2]:
''' Case when first input is '/' or '*' '''
STORAGE += ['1']+list(char)
if char in OPERATORS and STORAGE[-1] is None and char not in OPERATORS[:2]:
''' Case when first input is '+' or '-' '''
STORAGE += ['0']+list(char)
if char in OPERATORS and STORAGE[-1] in OPERATORS:
''' Switching to latest operator, example: '3+-' evalutes to '3-' '''
STORAGE[-1] = char
if char in OPERATORS and STORAGE[-1] not in OPERATORS:
STORAGE += list(char)
# else block
if char not in OPERATORS and ...
... |
|
Logic operators are good to use in conditions, but the idea of flattening is to make represented conditionals more readable. Some ideas are below. It is not a perfect solution, but compare it to your version. Is it easier to understand? class Storage:
__operators: list = ["*", "/", "-", "+"]
__special: list = ["C", "AC", "P/M"]
__storage: list
def __init__(self):
self.__storage = []
def get_storage(self) -> list:
""" get storage as a list """
return self.__storage
def get_storage_as_string(self) -> str:
""" get string representation """
return "".join(
map(
lambda element: " " + element + " " if element in self.__operators else element,
self.__storage
)
)
def __repr__(self) -> str:
return self.get_storage_as_string()
def put_in_storage(self, character: str) -> None:
""" puts a character to storage """
if character in self.__operators:
return self.__put_operator(character)
if character in self.__special:
return self.__apply_special(character)
if not character.isnumeric() and character != ".":
raise Exception("Given symbol is invalid") # todo: check if there is only one dot in number
return self.__put_digit(character)
def __put_operator(self, character: str) -> None:
if len(self.__storage) > 0: # is not empty
if self.__storage[-1] in self.__operators:
self.__storage[-1] = character
else:
self.__storage.append(character)
else:
""" put "1" + ("/" or "*") or "0" + ("+" or "-") """
self.__storage.extend([["1", "0"][self.__operators.index(character)//2], character])
def __apply_special(self, character: str) -> None:
if character == "AC":
self.__storage.clear()
if character == "C":
pass
if character == "P/M":
pass
def __put_digit(self, character: str) -> None:
if len(self.__storage) == 0 or self.__storage[-1] in self.__operators:
""" is empty or previous chunk is an operator """
self.__storage.append(character)
else:
self.__storage[-1] += character
def test1(storage_instance: Storage) -> tuple:
storage_instance.put_in_storage("1")
storage_instance.put_in_storage("+")
storage_instance.put_in_storage("2")
storage_instance.put_in_storage("4")
storage_instance.put_in_storage("/")
storage_instance.put_in_storage("5")
result_str = storage_instance.get_storage_as_string()
return result_str, 1 if result_str == "1 + 24 / 5" else 0
def test2(storage_instance: Storage) -> tuple:
storage_instance.put_in_storage("/")
storage_instance.put_in_storage("5")
result_str = storage_instance.get_storage_as_string()
return result_str, 1 if result_str == "1 / 5" else 0
def test3(storage_instance: Storage) -> tuple:
storage_instance.put_in_storage("+")
storage_instance.put_in_storage("1")
result_str = storage_instance.get_storage_as_string()
return result_str, 1 if result_str == "0 + 1" else 0
if __name__ == "__main__":
# make unit tests using unittest?
for test in [test1, test2, test3]:
storage_instance = Storage()
test_result = test(storage_instance)
print("{}: got {}, valid? {}"
.format(test.__name__, test_result[0], ["not", "yes"][test_result[1]])) |
|
I know that it might be a bit complicated code for beginner, so if you have any questions feel free to ask me here. |
|
@nechepurenkoN, to be honest, the concepts of Thanks a lot for lending a helping hand. Your help and suggestions means a lot to me. You will be bothered many more times before the completion of this refactoring project 😄 . |
|
The truth is if you want to become a software developer, one day you will have to face up classes, tests and other boring stuff. Why these classes, refactoring, decomposition etc. are important? It might be like a steep trampoline, but any difficulties your overcome make you more experienced and professional. I'm not bothered to help you because I answer in my spare time and I don't need something in return. Don't be ashemed to ask questions that seems "fool" or something like that, because it's normal when you learn something new. Programming concepts like object-oriented paradigm are not easy to understand on your own, but I can help you with examples related to your project. I don't insist to do this refactoring things, but if you want to, mention me at this thread with any question you have. |
I have been refactoring this code, right from the day this thread was opened. I made little progress due to lack of experience and clarity. Your previous snippet of refactored code is just what I needed. I will update my progress regularly is this separate gist.
It is way easier to understand the code now. It is very clear to me now that architecture of the code is very important for readability.
This is so nice of you. 😄
TBH, it looked complicated at first sight. I sat down yesterday and started exploring all the unknowns concepts in your example code. End result, I was able to see the beauty of your refactored chunk of code after learning function annotation, variable annotation, difference between class variable and instance variable. I believe continuing to keep this thread as descriptive as possible will help other new-devs in future in some or the other way. 🤓 |
|
Update from WIP refactor source gist. class Storage:
...
def go_for_calc(self):
obj = Calculate(self.__storage)
self.__answer = obj.__calculate
...
class Calculate:
ans: str
def __init__(self, expr_as_list):
self.expression = ''.join(i for i in expr_as_list)
def __calculate(self) -> str:
self.ans = str(eval(self.expression)
return self.ansUnfortunately, this throws class Storage:
...
def go_for_calc(self):
obj = Calculate(self.__storage)
self.__answer = obj.ans
...
class Calculate:
ans: str
def __init__(self, expr_as_list):
self.expression = ''.join(i for i in expr_as_list)
self.__calculate()
def __calculate(self) -> str:
self.ans = str(eval(self.expression)This does give the desired answer but I am pretty sure there is a better way to do this. |
|
In the line 56 there is a typo: self.__storage.extend(['0', '1'][self.__operator.index(operator)//2, operator])instead of self.__storage.extend([['1', '0'][self.__operator.index(operator)//2], operator])It needs ']' after '//2', "1" before "0" and "[" in the beginning. How does it work? # we define a temporary list ["1", "0"]; for simplicity, let it be:
a = ["1", "0"]
# how can we get 1 if operators are "*" or "/" and 0 if operators are "+", "-" ?
# we know, that this character IS in our operators list, so lets find its position:
index = self.__operators.index(character)
# we will get index in [0..3] range, and 0-1 should be mapped to 0th index in a, 2-3 to 1th index in a
# so we divide index by 2
index_in_a = index // 2
# and get the number
result = a[index_in_a]
# hence, we can shorten this expression, by defining list "a" in place and not assigning it to a variable
# [create a][get index_in_a]
["1", "0"][self.__operators.index(character)//2] == ("1" if character in ["/", "*"]) or ("0" if character in ["+","-"])This is not a good practice make things like that, but I wanted to show you some language sugar :) |
|
Two underscores mark a class field or method as private. It means that you can access it only in class definition, for ex.: class A:
__a = 10
class B:
def __init__(self):
a_instance = A()
print(a_instance.__a) # is incorrect, because the field __a is private
# it cannot be accessed outside class A.Private functions also are not accessible outside class definition. There is a way to access it, but it is some kinda hacking. class A:
def calculate(self):
pass
def __calculate1(self):
pass
class B:
def foo(self):
a = A()
a.calculate() # all good
a.__calculate1() # errorWhy should you use private functions then? Let's look up in If it is not clear ask me for another examples. |
Ok, class Calculate(Storage):
operators = Storage.__operators
def __init__(self, expr_as_list):
self.expression = ''.join(i for i in expr_as_list)
...In this second approach I made the following changes in Storage and Calculate: class Storage:
...
def go_for_calc(self):
obj = Calculate(self.__storage, self.__operators)
...
...
class Calculate:
...
def __init__(self, expr_as_list, operators):
self.expression = ''.join(i for i in expr_as_list)
self.operators = operators
...@nechepurenkoN, can you please point out which is safer way, or if there is a better way to do it? |
|
IMHO, the second way is better. Inheritance is used when you want to extend existing class with new fields/methods or redefine methods. For example, there is Hero class and its objects have class Hero:
def __init__(self, base_damage):
self.base_damage = base_damage
def attack(self):
pass
class Knight(Hero):
def __init__(self, base_damage):
super().__init__(base_damage)
def attack(self):
print(f"Melee attack with {self.base_damage} damage")
class Archer(Hero):
def __init__(self, base_damage):
super().__init__(base_damage)
def attack(self):
print(f"Range attack with {self.base_damage} damage")Knight and archer are heroes, so it is logical to use inheritance. But Usually, all the class fields are made private. As I said, if we make storage_instance = Storage()
storage_instance.put_in_storage("&") # will not have a bad consequences
storage_instance.storage.append("&") # some problems hereSo we have to protect storage list -> make it private (__storage). And the question itself: how to access this field? class A:
def __init__(self):
self.__a = 10
self.__b = 5
def get_a(self):
return self.__a
def get_b(self):
return self.__b
def set_a(self, new_a):
self.__a = new_a
def set_b(self, new_b):
self.__b = new_b
obj = A()
print(obj.get_a())
obj.set_a(42)
print(obj.get_a())But in our case, we cannot just make storage_list = storage_instance.get_storage()
storage_list.append("&") # would affect __storage (see how does python store lists)Solution: We return a copy of storage list which we can read and manipulate with. Make a method in ...
def calculate(self, storage: Storage) -> float:
storage_list = storage.get_storage()
# or use storage.get_storage_as_string
...Usage: storage_instance = Storage()
calculator_instance = Calculator()
result = calculator_instance.calculate(storage_instance) |
|
There is a way to shorten calculation code. I adapt it to your method. operators = ["*", "/", "+", "-"]
expression_list = ["1", "+", "2"]
functional_mapping = {
"+": lambda x,y: x+y,
"-": lambda x,y: x-y,
"*": lambda x,y: x*y,
"/": lambda x,y: x/y
}
left_operand = float(expression_list[0])
right_operand = float(expression_list[2])
operator = expression_list[1]
result = functional_mapping[operator](left_operand, right_operand)
print(result)When we call |
This is absolute beauty. I really like this logic to shorten the code 👍.
Update from gist, when multiple decimal(dot) entry, logic updated to consider only first entry (ln: 79). def __put_dot(self, dot):
if self.__storage[-1] not in self.__operators:
index_of_dot = self.__storage[-1].find(dot)
if index_of_dot != -1:
''' Multiple decimal inputs ex! (3.2.).
Replacing '''
self.__storage[-1].replace(dot, '')
else:
self.__storage[-1] += dot It is still disturbing to see the need of using nested if-else here. I don't see a workaround to solve this without a nested block. |
|
If there was a dot before, why don't just ignore next number dots? This is how Windows calculator works. Test out, what happens if you are trying to add another dot to a number (in Linux for ex.)? To ignore other dots simply add a condition def __put_dot(self):
if len(self.__storage) == 0:
self.__storage.append("0.")
return
if self.__storage[-1] not in self.__operators and not "." in self.__storage[-1]:
self.__storage[-1] += "." |
Linux points out where the malformed expression is:
|
|
I think that ignoring is not a bad decision at all |
I came to a conclusion that ...
__layout = ['*', '/', 'C', 'AC',
'9', '8', '7', '-',
'6', '5', '4', '+',
'3', '2', '1', '+/-',
'.', '0', '=']
...
# ln: 170
''' Control Binding '''
buttons = []
for bt_text in self.__layout[:-1]:
bt = ttk.Button(master=keypad)
bt['text'] = bt_text
bt['command'] = lambda: (self.bt_cmd.into_storage(bt_text),
self.bt_cmd.show_storage(self.DISPLAY))
buttons.append(bt)
...Is this conclusion relevant or am I getting it wrong? |
|
Why do we refactor a code? To make the code shorted or to make it more readable? |
|
I really have no idea why doesn't buttons binding work :(. I haven't used python gui libraries so I can't help you with previous question. If you can make your code more readable, then make it, but keep in mind that some programmers are more experienced than we are. They don't use, for example, range loops, generally speaking - common approaches. The way they shorten the code is using some language technics, sugar etc. without making it more difficult to read for people who knows the language as intermediate user. For example: a = [1,2,3,4]
for i in range(len(a)):
print(f"index: {i}, element: {a[i]}")
for index, element in enumerate(a):
print(f"index: {index}, element: {element}")a = []
for i in range(5):
if i % 2 == 0
a.append(i*i)
a = [i*i for i in range(5) if i % 2 == 0]# consider a, b are int variables
temp = a
a = b
b = temp
a, b = b, aWe make our code more readable by using built-in function enumerate. The first approach is more general, it might be used in any programming language with arrays and for-loops. The second one is using a generator - some useful pythonic feature. Readability is the most important thing, but if if you can shorten the code without losing readability then make it. |
|
Presently there seems no better workaround for button binding problem. Anyhow, source.py is refactored and broken down to separate files.
|
|
I have thinking about this for a while: using a
Moreover it morally doesn't seem right. Here are some words from tkdocs:
Allowing users to directly enter input (ex: copy/paste) can be achieved by using |
Hello!
I've checked out your project and I want to suggest you some refactoring tips
and you can configure styles and other gui things in init method, for ex.:
and next you can use storage or temporary operand variables in any method here or directly pass it in other objects, for ex.:
This step needs to separate the gui functionality and the core program logic so you can independently change them.
and make an instance of this class a field of your MainWindow class. Using this approach you can easily bind control elements like:
instead of:
I think you would got a point.
Make a class with calc methods
This is another step to separate your program components. Also you can make an instanse of this class as a field of MainWindow to have a direct access.
Elimenate nested "if" instructions
For ex.:
can be written as:
I think you would got a point.
I recommend you to read "Clean code" by Robert Martin. There are a lot of guidelines to improve your code quality with perfect argumentation.
Good luck!
The text was updated successfully, but these errors were encountered: