Merge lp:~julien-spautz/cerbere/watchdog-refactor into lp:~elementary-pantheon/cerbere/cerbere

[Julien Spautz]

Refactored the code in Watchdog.vala to eliminate the nested if clauses and make it more readable.

Refactored the code in Watchdog.vala to eliminate the nested if clauses and make it more readable.

None of the behavior should have changed, and the API is also untouched.

I commited each small refactoring step so you can see that they don't have any side-effects.

[Victor Martinez (victored)]

Hey, nice work!

+1 for improving readability. This could use further improvements.

- "requires" is being used for things that can, and are expected to fail at runtime, instead of static contract definition. It is not a programming error to call "add_process" and pass the name of a process that is already monitored. This can happen if a user adds the same string twice to monitored-processed in GSettings. Instead, the method should just do nothing, and/or print a warning, but assertions are not applicable here.

- For consistency, since there is "is_new_command", the statements "processes.unset (command)" should be replaced by a new method "remove_command" (preferrably "unmonitor_command") that does it, so that nobody accesses the hashmap directly.

- Diff line 91 should be: "if (is_new_command (command)) { ...".

- Diff line 32 should be: "return !processes.has_key (command);". Same style change is desired fo diff line 112.

- Diff line 102: use "%u" instead of "%s" to avoid calling "to_string" for "max_crashes".

- Instead of "is_new_command" and "remove_command" (not there yet), I would suggest using "is_command_monitored" and "unmonitor_command", respectively.

review: Needs Fixing

[lp:~julien-spautz/cerbere/watchdog-refactor]

62. By Julien Spautz on 2013-11-03

replace 'processes.unset (command)' with 'unmonitor_command (command)'

63. By Julien Spautz on 2013-11-03

fixed strings and use ! instead of == false

64. By Julien Spautz on 2013-11-03

use is_new_command instead of \!processes.has_key (command)

65. By Julien Spautz on 2013-11-03

turn pre condition into simple fallthrough if clause

66. By Julien Spautz on 2013-11-03

add monitor_and_run_command method

67. By Julien Spautz on 2013-11-03

replace is_new_command with \!command_is_monitored

[Julien Spautz (julien-spautz)]

Fixed everything you said (hopefully) and additionally created a monitor_and_run_command () method.

[David Gomes (davidgomes)]

Should be WatchDog not Watchdog.

review: Needs Fixing

[Victor Martinez (victored)]

Julien, I like the changes, thank you!

Even though it's implied, for a future branch it would be nice if you could explicitly define the methods as "private" to keep the code consistent.

David, since Julien didn't write that class definition, can you propose a separate branch for that?

review: Approve

[David Gomes (davidgomes)]

Agreed, will do that when I get home.

review: Approve

[lp:~julien-spautz/cerbere/watchdog-refactor]

68. By Julien Spautz on 2013-11-09

add explicit 'private' for consistency

[Julián Unrrein (junrrein)]

Is this ready for landing?

[Victor Martinez (victored)]

It is ready for landing. I've already proof-read the changes and the control flow remains the same, but written in a nicer way.

review: Approve

[Victor Martinez (victored)]

We can replace diff lines 100-105 with "if (!command_is_monitored) assert_not_reached ();", since the warning is not visible and doesn't add any value (I should have never added it).

That is for a separate branch though. Anyone up to propose another merge with that change?

=== modified file 'src/Watchdog.vala'
--- src/Watchdog.vala	2012-11-22 23:12:01 +0000
+++ src/Watchdog.vala	2013-11-09 18:17:48 +0000
@@ -34,64 +34,75 @@
         processes = new Gee.HashMap<string, ProcessWrapper> ();
     }
 
-    public void add_process (string command) {
-        if (command.strip () == "")
-            return;
-
-        // Check if a process for this command has already been created
-        if (processes.has_key (command))
-            return;
-
+    public void add_process (string command)
+        requires (is_valid_command (command)) {
+        
+        if (command_is_monitored (command)) {
+            warning ("Command '%s' is already being monitored.", command);
+            return;
+        }
+        
+        monitor_and_run_command (command);
+    }
+
+    private bool is_valid_command (string command) {
+        return command.strip () != "";
+    }
+
+    private bool command_is_monitored (string command) {
+        return processes.has_key (command);
+    }
+    
+    private void monitor_and_run_command (string command) {
         var process = new ProcessWrapper (command);
         processes[command] = process;
-
         process.exited.connect (on_process_exit);
-
         process.run_async ();
     }
 
     /**
-     * Process exit handler.
-     *
-     * Respawning occurs here. If the process has crashed more times than max_crashes, it's not
-     * respawned again. Otherwise, it is assumed that the process exited normally and the crash
-     * count is reset to 0, which means that only consecutive crashes are counted.
+     * Respawning occurs here. If the process has crashed more times than
+     * max_crashes, it's not respawned again. Otherwise, it is assumed that the
+     * process exited normally and the crash count is reset to 0, which means
+     * that only consecutive crashes are counted.
      */
     private void on_process_exit (ProcessWrapper process, bool normal_exit) {
-        if (normal_exit) {
-            // Reset crash count. We only want to count consecutive crashes, so that
-            // if a normal exit is detected, we reset the counter to 0.
+        if (normal_exit)
             process.reset_crash_count ();
-        }
 
-        bool remove_process = false;
         string command = process.command;
 
-        // if still in the process list, relaunch if possible
-        if (command in App.settings.process_list) {
-            // Check if the process is still present in the map since it could have been removed
-            if (processes.has_key (command)) {
-                // Check if the process already exceeded the maximum number of allowed crashes.
-                uint max_crashes = App.settings.max_crashes;
-
-                if (process.crash_count <= max_crashes) {
-                    process.run_async (); // Reload right away
-                } else {
-                    warning ("'%s' exceeded the maximum number of crashes allowed (%s). It won't be launched again", command, max_crashes.to_string ());
-                    remove_process = true;
-                }
-            } else {
-                // If a process is not in the map, it means it wasn't re-launched after it exited, so in theory
-                // this code is never reached.
-                critical ("Please file a bug at http://launchpad.net/cerbere and attach your .xsession-errors and .xsession-errors.old files.");
-            }
-        } else {
+        if (is_not_in_settings (command)) {
             warning ("'%s' is no longer in settings (not monitored)", command);
             process.reset_crash_count ();
-            remove_process = true;
-        }
-
-        if (remove_process)
-            processes.unset (command);
-    }
-}
\ No newline at end of file
+            unmonitor_command (command);
+            return;
+        }
+
+        if (!command_is_monitored (command)) {
+            critical ("Please file a bug at http://launchpad.net/cerbere and " +
+                      "attach your .xsession-errors and .xsession-errors.old " +
+                      "files.");
+            return;
+        }
+
+        uint max_crashes = App.settings.max_crashes;
+
+        if (process.crash_count > max_crashes) {
+            warning ("'%s' exceeded the maximum number of crashes allowed " +
+                     "(%u). It won't be launched again", command, max_crashes);
+            unmonitor_command (command);
+            return;
+        }
+
+        process.run_async ();
+    }
+    
+    private bool is_not_in_settings (string command) {
+        return !(command in App.settings.process_list);
+    }
+    
+    private void unmonitor_command (string command) {
+        processes.unset (command);
+    }
+}