Форум сайта python.su
0
Хотелось бы услышать ваше мнение по этому коду:
#! /usr/bin/python
'''Calculator in object oriented style,
__future__ to use true division by '/'
'''
from __future__ import division
from Tkinter import \
Tk, Frame, Entry, Button, \
END, TOP, LEFT, RIGHT
class GUI:
def __init__(self, title='My calc',
width_root=160, height_root=170,
width_num=1, width_math=2,
btn_names='C7894561230.=+-/*'):
self.root = Tk()
self.root.title(title)
self.root.maxsize(width = width_root, height = height_root)
self.root.minsize(width = width_root, height = height_root)
fra = Frame(self.root)
fra_ent = Frame(self.root)
fra_math = Frame(self.root)
fra_num = Frame(fra)
fra_num2 = Frame(fra)
fra_num3 = Frame(fra)
fra_num4 = Frame(fra)
self.entry = Entry(fra_ent)
self.entry.insert(END, '0')
self.entry.pack(side = TOP)
fra_ent.pack(side = TOP)
fra.pack(side = LEFT)
fra_math.pack(side = RIGHT)
fra_num.pack(side = TOP)
fra_num2.pack(side = TOP)
fra_num3.pack(side = TOP)
fra_num4.pack(side = TOP)
strct = {btn_names[0]: (fra_ent, width_math, RIGHT)}
for i in btn_names[1:4]:
strct[i] = (fra_num, width_num, LEFT)
for i in btn_names[4:7]:
strct[i] = (fra_num2, width_num, LEFT)
for i in btn_names[7:10]:
strct[i] = (fra_num3, width_num, LEFT)
for i in btn_names[10:13]:
strct[i] = (fra_num4, width_num, LEFT)
for i in btn_names[13:]:
strct[i] = (fra_math, width_math, TOP)
for name in btn_names:
Button(strct[name][0], text = name,
width = strct[name][1],
command = lambda x = name: self.write(x)
).pack(side = strct[name][2])
def write(self, char):
self.show(char)
def show(self, value):
self.entry.delete(first = 0, last = END)
self.entry.insert(END, value)
def run(self):
self.root.mainloop()
class Calc(GUI):
def __init__(self):
GUI.__init__(self)
self.numb = ''
self.memr = ''
self.result = ''
self.switch = {'=': lambda: self.calculate(),
'C': lambda: self.reset(),
'.': lambda: self.dot(),
'+': lambda: self.operation('+'),
'-': lambda: self.operation('-'),
'*': lambda: self.operation('*'),
'/': lambda: self.operation('/')}
def write(self, char):
if char in self.switch:
self.switch[char]()
else:
self.numb += char
self.show(self.numb)
def reset(self):
self.numb = ''
self.memr = ''
self.result = ''
self.show('0')
def dot(self):
if not self.numb:
# Has not achieved the number before '.'
if self.result:
self.reset()
self.numb = '0.'
self.show(self.numb)
elif '.' in self.numb:
# Repeated, incorrect entry '.'
pass
else:
self.numb += '.'
self.show(self.numb)
def operation(self, char):
'''Arithmetic operations'''
if self.result:
# Use the result of the previous calculation
self.numb = self.result
self.result = ''
self.memr += self.numb + char
self.show(self.numb + char)
self.numb = ''
def calculate(self):
try:
result = eval('%s%s' % (self.memr, self.numb))
except (SyntaxError, ZeroDivisionError):
self.reset()
self.show('ERROR')
else:
self.result = str(result)
self.numb = ''
self.memr = ''
self.show(self.result)
if __name__ == '__main__':
Calc().run()
Отредактировано (Окт. 27, 2011 14:14:43)
Офлайн
13
В целом неплохо. Но есть вопросы и рекомендации.
- from Tkinter import * это так принято при программировании с использованием Tkinter? Вообще-то import * считается признаком плохого тона - засорение текущего namespace-а непонятно чем с серьезными последствиями.
- font, title, width, height я бы сделал параметрами
- В чем смысл from future import __division__ ?
- почему для кнопок ‘5’ и ‘6’ не используется width=1?
- if __name__ == ‘__main__’: Calc().root.mainloop() я бы не писал в одну строку
- вместо char == ‘+’ or char == ‘-’ or char == ‘/’ or char == ‘*’ я бы использовал char in “+-/*”
Запустите pylint. Ему ваш код сильно не понравился. Его оценка: Your code has been rated at -11.61/10 (previous run: -11.61/10)
PS: Это все при беглом просмотре. Могу найти еще много чего, но сначала это исправьте.
Офлайн
0
EdУ Лутца, как и в http://docs.python.org/library/tkinter.html примеры с import *, но это не сложно исправить, что я и сделал.
- from Tkinter import * это так принято при программировании с использованием Tkinter? Вообще-то import * считается признаком плохого тона - засорение текущего namespace-а непонятно чем с серьезными последствиями.
EdИстинное деление при отсутствии дополнительной логики.
- В чем смысл from future import __division__ ?
Офлайн
20
Смешивать логику и интерфейс считается неправильным. Должен быть объект Калькулятор, который предоставляет API для выполнения операций над числами. И должен быть объект ГУИ, который имеет ссылку на Калькулятор. Когда пользователь нажимает кнопку, то ГУИ просит Калькулятор выполнить какое-то действие, вызывая его метод, и возвращает пользователю результат. Причем если предполагается, что операция займет продолжительное время, то ее выполнение запускается в отдельном потоке, чтобы не блокировать интерфейс и дать пользователю возможность отменить свое действие. На мой взгляд, это два обязательных пункта, которые выполняются в любой программе с графическим интерфейсом.
Офлайн
13
Угу, сейчас получше.
Следующая порция вопросов:
- в чем смысл разделения на 2 класса?
- Может упростить всю эту генерацию кнопок? Там простенькая структурка с фреймами и названиями кнопок просто напрашивается. Они же все одинаковые.
- докстринг модуля поставьте до импортов - pylint перестанет на него ругаться
- может проще отлавливать SyntaxError, который генерит eval, чем проверять синтаксис самому. Мне удалось его завалить тупо набрав / 2 = не говоря уже о 1 / 0
- shebang-а нет. Вы под виндой?
Достаточно :) ?
Офлайн
0
Ed
- в чем смысл разделения на 2 класса?
SotericЕсли я правильно понимаю, вы говорите о одном и том же. Исправил свою ошибку переносом метода show из класса Calc в GUI. Теперь Calc обращается к GUI только через наследуемый метод show и переопределенный write.
Смешивать логику и интерфейс считается неправильным.
EdНет, Debian. Скрипт вне IDLE использовать не предполагается, я даже и не подумал об этом.
shebang-а нет. Вы под виндой?
Отредактировано (Окт. 27, 2011 00:50:39)
Офлайн
0
Если изменить
def __init__(self):
GUI.__init__(self)
self.numb = ''
self.memr = ''
self.result = ''
def __init__(self):
GUI.__init__(self)
self.reset()
def reset(self):
self.numb = ''
self.memr = ''
self.result = ''
Отредактировано (Окт. 29, 2011 16:57:02)
Офлайн
13
Если есть желание разделять UI и логику, то по-моему лучше это делать не путем наследования класса с логикой от класса с UI. Даже с точки зрения здравого смысла это, кхм, вызывает вопросы. Попробуйте вместо наследования просто передавать объект UI в качестве параметра в конструктор вашего калькулятора.
Если есть желание, то можете наваять простенький commandline UI, чтобы проверить достаточен ли интерфейс вашего UI класса. Думаю, вы сильно удивитесь :)
В сторону MVC смотрели?
Насчет кнопок - вы меня неправильно поняли. Чем такая реализация лучше уж то, что было. Я говорил о некой структуре, взглянув на которую можно было понять иерархию UI. Из нее же и генерить все или хотя бы часть виджетов. А при такой реализации читабельность сильно пострадала.
Офлайн
13
pylint ругается: Attribute ‘result’ defined outside __init__. Что плохого в defined outside __init__?то, что когда объект создается у вас этого атрибута еще нет, то есть потенциально можно получить AttributeError при доступе к нему.
Еще ругань на W: 83:Calc.__init__.<lambda>: Lambda may not be necessary. Это значит что есть еще способ определить кортеж выбора?Он абсолютно прав, лямбда там лишняя:
self.switch = {'=': self.calculate,
'C': self.reset,
'.': self.dot
...Отредактировано (Окт. 27, 2011 14:39:52)
Офлайн
13
Еще пара советов:
- лямбду можно заменить на partial из functools, если там простой вызов с определенным параметром.
- то, что при наборе скажем 2 + 1 + 1 после нажатия первой единицы все исчезает мне кажется неправильным. Может сделать просто тупой вывод того, что набирается, а при нажатии на = делать eval? Тогда можно будет и скобки легко реализовать и многое другое. И не нужна будет вся эта лишняя логика с numb, result и memr. Все это выглядит как ненужные костыли. Вы же все равно юзаете eval, лучше и проще будет быть честным до конца :)
Офлайн