Skip to main content
Code Review

Return to Answer

added 1 character in body
Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478

close() and run() are probably the only methodmethods you intend to expose onin Mengenlehreuhr. The other methods are internal interfaces, so you should name them with a leading underscore by convention.

close() and run() are probably the only method you intend to expose on Mengenlehreuhr. The other methods are internal interfaces, so you should name them with a leading underscore by convention.

close() and run() are probably the only methods you intend to expose in Mengenlehreuhr. The other methods are internal interfaces, so you should name them with a leading underscore by convention.

edited body
Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478
  • the redundancy between panel_name and the keys of panes (a quick fix would be to use an OrderedDict)
  • the use of string keys at all
  • the arrays of None to be replaced by curses windows later (I prefer to see them defined all at once, ideally using a list comprehension)
  • Thethe if-elif chains
  • Hardhard-coded magic numbers and dimensions. range(5) is because there are five panel types. width = 11 is written once in create_clock_face, and again as the default parameter value in create_window(). j * 11 is because most windows have a width of 10 and a one-column margin. The five_minutes case has width = 4 because the window width is 3, plus a one-column margin. offset = offset + 5 is because the window height is 4, according to the create_window() function. "Width" sometimes refers to the window width, and sometimes refers to the width and the margin.
  • offset should be named to make it obvious that it is a y-coordinate.
  • int(floor(n / 2)) would be better written as n // 2.
  • redefining variables makes the code harder to follow — screen_w gets halved; width gets overridden.
  • the redundancy between panel_name and the keys of panes (a quick fix would be to use an OrderedDict)
  • the use of string keys at all
  • the arrays of None to be replaced by curses windows later (I prefer to see them defined all at once, ideally using a list comprehension)
  • The if-elif chains
  • Hard-coded magic numbers and dimensions. range(5) is because there are five panel types. width = 11 is written once in create_clock_face, and again as the default parameter value in create_window(). j * 11 is because most windows have a width of 10 and a one-column margin. The five_minutes case has width = 4 because the window width is 3, plus a one-column margin. offset = offset + 5 is because the window height is 4, according to the create_window() function. "Width" sometimes refers to the window width, and sometimes refers to the width and the margin.
  • offset should be named to make it obvious that it is a y-coordinate.
  • int(floor(n / 2)) would be better written as n // 2.
  • redefining variables makes the code harder to follow — screen_w gets halved; width gets overridden.
  • the redundancy between panel_name and the keys of panes (a quick fix would be to use an OrderedDict)
  • the use of string keys at all
  • the arrays of None to be replaced by curses windows later (I prefer to see them defined all at once, ideally using a list comprehension)
  • the if-elif chains
  • hard-coded magic numbers and dimensions. range(5) is because there are five panel types. width = 11 is written once in create_clock_face, and again as the default parameter value in create_window(). j * 11 is because most windows have a width of 10 and a one-column margin. The five_minutes case has width = 4 because the window width is 3, plus a one-column margin. offset = offset + 5 is because the window height is 4, according to the create_window() function. "Width" sometimes refers to the window width, and sometimes refers to the width and the margin.
  • offset should be named to make it obvious that it is a y-coordinate.
  • int(floor(n / 2)) would be better written as n // 2.
  • redefining variables makes the code harder to follow — screen_w gets halved; width gets overridden.
Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478

Throttling

Your program runs in a tight loop, consuming all the CPU it can get (and battery power, if on a mobile device). For a clock that a user might want to run in the background for a long time, conservation is important. Inserting even just a 0.1 second sleep between updates brings CPU usage from 95% to 1% on my machine.

"Private" methods

close() and run() are probably the only method you intend to expose on Mengenlehreuhr. The other methods are internal interfaces, so you should name them with a leading underscore by convention.

Run loop

You shouldn't need to call exit(0) if you structure your code properly.

A more appropriate place for cleanup code is in a finally clause.

Clock face initialization

I refactored this a lot. I didn't like...

  • the redundancy between panel_name and the keys of panes (a quick fix would be to use an OrderedDict)
  • the use of string keys at all
  • the arrays of None to be replaced by curses windows later (I prefer to see them defined all at once, ideally using a list comprehension)
  • The if-elif chains
  • Hard-coded magic numbers and dimensions. range(5) is because there are five panel types. width = 11 is written once in create_clock_face, and again as the default parameter value in create_window(). j * 11 is because most windows have a width of 10 and a one-column margin. The five_minutes case has width = 4 because the window width is 3, plus a one-column margin. offset = offset + 5 is because the window height is 4, according to the create_window() function. "Width" sometimes refers to the window width, and sometimes refers to the width and the margin.
  • offset should be named to make it obvious that it is a y-coordinate.
  • int(floor(n / 2)) would be better written as n // 2.
  • redefining variables makes the code harder to follow — screen_w gets halved; width gets overridden.

The main improvement, in a nutshell, is to make the code data-directed, so that you declare the five types of panels rather than micromanaging their creation. Then write a simple layout engine to do the dirty work in a generic way.

import curses
from datetime import datetime
from time import sleep
class PanelType():
 def __init__(self, n, width, height=4, margin=1, color=lambda i, t: 0):
 self.n = n
 self.height = height
 self.width = width
 self.margin = margin
 self.color = color
 def create_windows(self, y, screen_w):
 """Make a row of n center-justified curses windows."""
 screen_mid = screen_w // 2
 total_width = self.n * self.width + (self.n - 1) * self.margin
 left = screen_mid - total_width // 2
 return [
 self._create_window(y, left + i * (self.width + self.margin),
 self.height, self.width)
 for i in range(self.n)
 ]
 @staticmethod
 def _create_window(y, x, height, width):
 win = curses.newwin(height, width, y, x)
 win.box()
 win.refresh()
 return win
class Mengenlehreuhr():
 PANEL_TYPES = [
 PanelType( # seconds
 n=1, width=10, color=lambda i, t: 3 if t.second % 2 else 2
 ),
 PanelType( # five hours
 n=4, width=10, color=lambda i, t: 1 if i >= t.hour // 5 else 2
 ),
 PanelType( # hours
 n=4, width=10, color=lambda i, t: 1 if i >= t.hour % 5 else 2
 ),
 PanelType( # five minutes
 n=11, width=3, color=lambda i, t: 1 if i >= t.minute // 5 else
 2 if i in (2, 5, 8) else 3
 ),
 PanelType( # minutes
 n=4, width=10, color=lambda i, t: 1 if i >= t.minute % 5 else 3
 ),
 ]
 def __init__(self):
 self.screen = curses.initscr()
 curses.noecho()
 curses.cbreak()
 self.screen.keypad(1)
 curses.start_color()
 curses.use_default_colors()
 curses.init_pair(1, curses.COLOR_WHITE, curses.COLOR_BLACK)
 curses.init_pair(2, curses.COLOR_WHITE, curses.COLOR_RED)
 curses.init_pair(3, curses.COLOR_WHITE, curses.COLOR_YELLOW)
 curses.curs_set(0)
 def close(self):
 """Restore the screen."""
 curses.nocbreak()
 self.screen.keypad(0)
 curses.echo()
 curses.endwin()
 def _create_clock_face(self):
 screen_h, screen_w = self.screen.getmaxyx()
 y = 0
 self.panels = []
 for panel_type in self.PANEL_TYPES:
 self.panels.append(panel_type.create_windows(y, screen_w))
 y += panel_type.height + panel_type.margin
 def _update(self, time):
 for panel_type, panel in zip(self.PANEL_TYPES, self.panels):
 for i, window in enumerate(panel):
 color_pair = curses.color_pair(panel_type.color(i, time))
 window.bkgd(' ', color_pair)
 window.refresh()
 def run(self):
 """Run the clock until a keyboard interrupt."""
 self._create_clock_face()
 try:
 while True:
 self._update(datetime.now())
 sleep(0.1)
 except KeyboardInterrupt:
 pass
 finally:
 self.close()
Mengenlehreuhr().run()
lang-py

AltStyle によって変換されたページ (->オリジナル) /