Countdown AppIndicator

I've decided to translate an old Unity AppIndicator I had written in C to Ruby, so I can practice coding with it (got a little bored with the completely beginner lessons I've been following, which I'll finish!). The C indicator itself was a bit raw, I kind of wrote it in an afternoon, so I think it reflected on the Ruby indicator as well.

It's still a work in progress. For now the timer can be configured only through a configuration file (which is described in the README and in the example file). Once the file is set, the user can start the indicator, click on the icon, "Start timer..." and "Get from config file". The reason this window pops up is because I was thinking about making a more friendly UI where the user could input the same parameters.

There are 4 parameters you can configure: Enable Notify and Notify Delay (issues notify-send calls to remind the user of how much time is left), Initial timer and Persistent timer (the former just sets at how many seconds will the timer start at and the latter will make the timer persistent once you close and open the app again). The persistent timer works by writing a file which has the epoch time when the timer is supposed to end, so it can resume.

The icon color of the indicator changes in some ranges (over 10 minutes it is blue, over 5 minutes it's orange, below 5 minutes it's red and when it reaches 0 it's black). Once it reaches 0, nothing happens (I plan to implement some notification, maybe an alarm).

GitHub repo

```
#!/usr/bin/ruby

require 'ruby-libappindicator'

# DEBUG FUNCTION
DEBUG=true
def debug(message)
if DEBUG==true
puts "DEBUG: "+message
end
end

# SOME CONSTANTS
BLUE_ICON_RANGE = 600 #10 minutes
ORANGE_ICON_RANGE = 300 #5 minutes
# So here is how it goes: Timer > 10 minutes icon is blue. 10 minutes > Timer > 5 minutes icon is orange. Timer 0)
param_value_list = line.split("=")
debug("Parameter: #{param_value_list[0]} - Val

There are many places you can improve the code, and it is tempting to rewrite large chunks of it because it will warrant a rewrite to fit in with Ruby standards and idioms.

basic style

I would suggest you read through the ruby style guide, and fix up those simple things - indentation, method calls (no need for empty '()'), spacing, single quotes vs double quotes, parens around conditionals etc.

Think in objects

It's clear you are coming from a more procedural language, but you have to unlearn those habits and learn new object-oriented approaches to your code design.

You have a single object, but this program definitely deserves at least a few more objects. I would suggest you look through design pattern examples and perhaps buy the accompanying book.

If your code was logically broken down into different objects that had meaningful relationships and clearly defined responsibilities, it would be much easier to read it from afar and maintain it in the future.

Break up your object

..into other objects and/or small methods.

Generally, you should be doing as little processing in your initialize method as possible. Initializing a class should not kick off a bunch of stuff to create this God Object. A simple, temporary solution to this would be to move out everything you can into a separate method, aptly named.

Something like:

class MyClass
  def initialize(name, item, category)
    @name, @item, @category = name, item, category
    # very minimal processing - set variables, etc, that's it
  end

  def self.build(name, item, category)
    new(name, item, category).build
  end

  def build
    # throw bulk initialization processing in here
  end
end

# usage
MyClass.build(name, item, category)

While you are moving your initializing code, think about breaking it up into smaller, descriptive methods.

For example, you could extract your @indicator_icons into a method..

def indicator_icons
  @indicator_icons ||= begin
    [
      File.realpath("./Icons/IconBlack.png"),
      File.realpath("./Icons/IconBlue.png"),
      ...
    ]
  end
end

Can't comment much on the menu object as it's a little difficult to read. One thing I will say it looks like you're weaving in and out of class and object contexts (use of self in initialize). Be wary of this.

fix your attributes

The way you are specifying attributes is wrong, and actually shouldn't have an effect at all. If you want to define attributes and a way to access or modify them, you can use either attr_reader :list, :of, :attributes or attr_writer :list, :of, :attributes or, for both attr_accessor :list, :of, :attributes. See here for more info.

extract more!

Another prime opportunity for extraction is your config file parsing. If you can use a different format that doesn't require so much parsing (yaml, etc), great! If not, you should look into extracting the parser into its own object.

You would then have something like (note no empty ()):

def config
  @config ||= Config.parse('CountdownI.config')
end

Couple of other notes on the above code. I've removed read_ as that is redundant. In Ruby, we forget about read_ this, get_ that, is_ this, set_ that. Be succinct and descriptive. The method name clearly indicates it will return your config.

I have also used memoization (@config ||=). This is to prevent the method from parsing the config multiple times. It will parse once and set variable as return of parse, then simply return variable value on subsequent calls. Get used to this and use it for making your code more efficient.

extract yet more!

Another opportunity for extraction is in your conditionals.

You have: if(@enable_notify)

First, as noted in the basic style part of this review, you shouldn't use parentheses. It should be if @enable_notify. Better yet, you can extract this into a predicate method:

def notify?
  @enable_notify
end

if notify?
  ...
end

in closing

The main issues I would say:

• basic syntax/ruby style is off - read style guides

• single object is doing too much - break out responsibilities into other objects

• breakdown objects into small, descriptive methods

Hope this helps!

StackExchange Code Review Q#147568, answer score: 5