From 2f7f46b25655aa6f2c2a7756663c97ddb4491100 Mon Sep 17 00:00:00 2001
From: Sato Hiroyuki <sathiroyuki@gmail.com>
Date: Wed, 6 Mar 2013 20:31:28 +0900
Subject: [PATCH 1/8] Refactor: replace "render :json = graph.to_json" to view
 template(show.json.erb).

Because model shouldn't know about view logic.
---
 app/controllers/graph_controller.rb | 17 +++++++----------
 app/helpers/graph_helper.rb         |  5 +++++
 app/models/graph/commit.rb          | 22 +---------------------
 app/models/graph/json_builder.rb    |  7 -------
 app/views/graph/show.json.erb       | 26 ++++++++++++++++++++++++++
 5 files changed, 39 insertions(+), 38 deletions(-)
 create mode 100644 app/helpers/graph_helper.rb
 create mode 100644 app/views/graph/show.json.erb

diff --git a/app/controllers/graph_controller.rb b/app/controllers/graph_controller.rb
index 33cb2d2dcb9..d27fd039478 100644
--- a/app/controllers/graph_controller.rb
+++ b/app/controllers/graph_controller.rb
@@ -8,24 +8,21 @@ class GraphController < ProjectResourceController
   before_filter :require_non_empty_project
 
   def show
-    if params.has_key?(:q) && params[:q].blank?
-      redirect_to project_graph_path(@project, params[:id])
-      return
-    end
-
     if params.has_key?(:q)
+      if params[:q].blank?
+        redirect_to project_graph_path(@project, params[:id])
+        return
+      end
+
       @q = params[:q]
       @commit = @project.repository.commit(@q) || @commit
     end
 
     respond_to do |format|
       format.html
+
       format.json do
-        graph = Graph::JsonBuilder.new(project, @ref, @commit)
-        graph.commits.each do |c|
-          c.icon = gravatar_icon(c.author.email)
-        end
-        render :json => graph.to_json
+        @graph = Graph::JsonBuilder.new(project, @ref, @commit)
       end
     end
   end
diff --git a/app/helpers/graph_helper.rb b/app/helpers/graph_helper.rb
new file mode 100644
index 00000000000..ba8c68a1ce0
--- /dev/null
+++ b/app/helpers/graph_helper.rb
@@ -0,0 +1,5 @@
+module GraphHelper
+  def join_with_space(ary)
+    ary.collect{|r|r.name}.join(" ") unless ary.nil?
+  end
+end
diff --git a/app/models/graph/commit.rb b/app/models/graph/commit.rb
index 8ed61f4b5af..e47a543daa9 100644
--- a/app/models/graph/commit.rb
+++ b/app/models/graph/commit.rb
@@ -4,7 +4,7 @@ module Graph
   class Commit
     include ActionView::Helpers::TagHelper
 
-    attr_accessor :time, :spaces, :refs, :parent_spaces, :icon
+    attr_accessor :time, :spaces, :refs, :parent_spaces
 
     def initialize(commit)
       @_commit = commit
@@ -17,26 +17,6 @@ module Graph
       @_commit.send(m, *args, &block)
     end
 
-    def to_graph_hash
-      h = {}
-      h[:parents] = self.parents.collect do |p|
-        [p.id,0,0]
-      end
-      h[:author]  = {
-        name: author.name,
-        email: author.email,
-        icon: icon
-      }
-      h[:time]    = time
-      h[:space]   = spaces.first
-      h[:parent_spaces]   = parent_spaces
-      h[:refs]    = refs.collect{|r|r.name}.join(" ") unless refs.nil?
-      h[:id]      = sha
-      h[:date]    = date
-      h[:message] = message
-      h
-    end
-
     def add_refs(ref_cache, repo)
       if ref_cache.empty?
         repo.refs.each do |ref|
diff --git a/app/models/graph/json_builder.rb b/app/models/graph/json_builder.rb
index 013d15fb754..2e0edb8a9ca 100644
--- a/app/models/graph/json_builder.rb
+++ b/app/models/graph/json_builder.rb
@@ -19,13 +19,6 @@ module Graph
       @days = index_commits
     end
 
-    def to_json(*args)
-      {
-        days: @days.compact.map { |d| [d.day, d.strftime("%b")] },
-        commits: @commits.map(&:to_graph_hash)
-      }.to_json(*args)
-    end
-
   protected
 
     # Get commits from repository
diff --git a/app/views/graph/show.json.erb b/app/views/graph/show.json.erb
new file mode 100644
index 00000000000..0531bc3c920
--- /dev/null
+++ b/app/views/graph/show.json.erb
@@ -0,0 +1,26 @@
+<% self.formats = ["html"] %>
+
+<%= raw(
+  {
+    days: @graph.days.compact.map { |d| [d.day, d.strftime("%b")] },
+    commits: @graph.commits.map do |c|
+      {
+        parents: c.parents.collect do |p|
+          [p.id,0,0]
+        end,
+        author: {
+          name: c.author.name,
+          email: c.author.email,
+          icon: gravatar_icon(c.author.email, 20)
+        },
+        time: c.time,
+        space: c.spaces.first,
+        parent_spaces: c.parent_spaces,
+        refs: join_with_space(c.refs),
+        id: c.sha,
+        date: c.date,
+        message: c.message,
+      }
+    end
+  }.to_json
+) %>

From 784aa266bdd38ec560c11bea92fc9b815e2ca456 Mon Sep 17 00:00:00 2001
From: Sato Hiroyuki <sathiroyuki@gmail.com>
Date: Wed, 6 Mar 2013 21:01:40 +0900
Subject: [PATCH 2/8] Refactor: grouping parent and their space by including
 array.

---
 app/assets/javascripts/branch-graph.js | 2 +-
 app/helpers/graph_helper.rb            | 5 +++++
 app/views/graph/show.json.erb          | 5 +----
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/app/assets/javascripts/branch-graph.js b/app/assets/javascripts/branch-graph.js
index 137e87de37f..520336adc0f 100644
--- a/app/assets/javascripts/branch-graph.js
+++ b/app/assets/javascripts/branch-graph.js
@@ -117,7 +117,7 @@
       // Draw lines
       for (var j = 0, jj = this.commits[i].parents.length; j < jj; j++) {
         c = this.preparedCommits[this.commits[i].parents[j][0]];
-        ps = this.commits[i].parent_spaces[j];
+        ps = this.commits[i].parents[j][1];
         if (c) {
           var cx = offsetX + 20 * c.time
             , cy = offsetY + 10 * c.space
diff --git a/app/helpers/graph_helper.rb b/app/helpers/graph_helper.rb
index ba8c68a1ce0..369330151f4 100644
--- a/app/helpers/graph_helper.rb
+++ b/app/helpers/graph_helper.rb
@@ -2,4 +2,9 @@ module GraphHelper
   def join_with_space(ary)
     ary.collect{|r|r.name}.join(" ") unless ary.nil?
   end
+
+  def parents_zip_spaces(parents, parent_spaces)
+    ids = parents.map { |p| p.id }
+    ids.zip(parent_spaces)
+  end
 end
diff --git a/app/views/graph/show.json.erb b/app/views/graph/show.json.erb
index 0531bc3c920..4a8605eec55 100644
--- a/app/views/graph/show.json.erb
+++ b/app/views/graph/show.json.erb
@@ -5,9 +5,7 @@
     days: @graph.days.compact.map { |d| [d.day, d.strftime("%b")] },
     commits: @graph.commits.map do |c|
       {
-        parents: c.parents.collect do |p|
-          [p.id,0,0]
-        end,
+        parents: parents_zip_spaces(c.parents, c.parent_spaces),
         author: {
           name: c.author.name,
           email: c.author.email,
@@ -15,7 +13,6 @@
         },
         time: c.time,
         space: c.spaces.first,
-        parent_spaces: c.parent_spaces,
         refs: join_with_space(c.refs),
         id: c.sha,
         date: c.date,

From e03a018d28488260eb6c69741680691426f823a6 Mon Sep 17 00:00:00 2001
From: Sato Hiroyuki <sathiroyuki@gmail.com>
Date: Thu, 7 Mar 2013 15:42:30 +0900
Subject: [PATCH 3/8] Refactor: rename module and class names.

* Module: Graph -> Network
* Class: JsonBuilder -> Graph
---
 app/controllers/graph_controller.rb                    |  2 +-
 app/models/{graph => network}/commit.rb                |  2 +-
 app/models/{graph/json_builder.rb => network/graph.rb} | 10 +++++-----
 features/steps/project/project_network_graph.rb        |  8 ++++----
 features/steps/shared/paths.rb                         |  4 ++--
 5 files changed, 13 insertions(+), 13 deletions(-)
 rename app/models/{graph => network}/commit.rb (98%)
 rename app/models/{graph/json_builder.rb => network/graph.rb} (97%)

diff --git a/app/controllers/graph_controller.rb b/app/controllers/graph_controller.rb
index d27fd039478..b4bf9565112 100644
--- a/app/controllers/graph_controller.rb
+++ b/app/controllers/graph_controller.rb
@@ -22,7 +22,7 @@ class GraphController < ProjectResourceController
       format.html
 
       format.json do
-        @graph = Graph::JsonBuilder.new(project, @ref, @commit)
+        @graph = Network::Graph.new(project, @ref, @commit)
       end
     end
   end
diff --git a/app/models/graph/commit.rb b/app/models/network/commit.rb
similarity index 98%
rename from app/models/graph/commit.rb
rename to app/models/network/commit.rb
index e47a543daa9..9c0a99ccd41 100644
--- a/app/models/graph/commit.rb
+++ b/app/models/network/commit.rb
@@ -1,6 +1,6 @@
 require "grit"
 
-module Graph
+module Network
   class Commit
     include ActionView::Helpers::TagHelper
 
diff --git a/app/models/graph/json_builder.rb b/app/models/network/graph.rb
similarity index 97%
rename from app/models/graph/json_builder.rb
rename to app/models/network/graph.rb
index 2e0edb8a9ca..c9ecbc910e8 100644
--- a/app/models/graph/json_builder.rb
+++ b/app/models/network/graph.rb
@@ -1,7 +1,7 @@
 require "grit"
 
-module Graph
-  class JsonBuilder
+module Network
+  class Graph
     attr_accessor :days, :commits, :ref_cache, :repo
 
     def self.max_count
@@ -19,7 +19,7 @@ module Graph
       @days = index_commits
     end
 
-  protected
+    protected
 
     # Get commits from repository
     #
@@ -30,8 +30,8 @@ module Graph
       # Decorate with app/models/commit.rb
       @commits.map! { |commit| Commit.new(commit) }
 
-      # Decorate with lib/gitlab/graph/commit.rb
-      @commits.map! { |commit| Graph::Commit.new(commit) }
+      # Decorate with app/model/network/commit.rb
+      @commits.map! { |commit| Network::Commit.new(commit) }
 
       # add refs to each commit
       @commits.each { |commit| commit.add_refs(ref_cache, repo) }
diff --git a/features/steps/project/project_network_graph.rb b/features/steps/project/project_network_graph.rb
index 7e9a7c295d0..cf5fa751ccf 100644
--- a/features/steps/project/project_network_graph.rb
+++ b/features/steps/project/project_network_graph.rb
@@ -8,8 +8,8 @@ class ProjectNetworkGraph < Spinach::FeatureSteps
   end
 
   When 'I visit project "Shop" network page' do
-    # Stub Graph::JsonBuilder max_size to speed up test (10 commits vs. 650)
-    Graph::JsonBuilder.stub(max_count: 10)
+    # Stub Graph max_size to speed up test (10 commits vs. 650)
+    Network::Graph.stub(max_count: 10)
 
     project = Project.find_by_name("Shop")
     visit project_graph_path(project, "master")
@@ -25,7 +25,7 @@ class ProjectNetworkGraph < Spinach::FeatureSteps
     end
   end
 
-  And 'I switch ref to "stable"' do
+  When 'I switch ref to "stable"' do
     page.select 'stable', :from => 'ref'
     sleep 2
   end
@@ -40,7 +40,7 @@ class ProjectNetworkGraph < Spinach::FeatureSteps
     end
   end
 
-  And 'I looking for a commit by SHA of "v2.1.0"' do
+  When 'I looking for a commit by SHA of "v2.1.0"' do
     within ".content .search" do
       fill_in 'q', :with => '98d6492'
       find('button').click
diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb
index 431d5299d8f..54cdbd4ba2f 100644
--- a/features/steps/shared/paths.rb
+++ b/features/steps/shared/paths.rb
@@ -142,8 +142,8 @@ module SharedPaths
   end
 
   Given "I visit my project's network page" do
-    # Stub Graph::JsonBuilder max_size to speed up test (10 commits vs. 650)
-    Graph::JsonBuilder.stub(max_count: 10)
+    # Stub Graph max_size to speed up test (10 commits vs. 650)
+    Network::Graph.stub(max_count: 10)
 
     visit project_graph_path(@project, root_ref)
   end

From 8c5003cf75bf48faf88d7148a241c9e89f623c97 Mon Sep 17 00:00:00 2001
From: Sato Hiroyuki <sathiroyuki@gmail.com>
Date: Thu, 7 Mar 2013 17:56:01 +0900
Subject: [PATCH 4/8] Refactor: clean up models.

* Network::Commit
** Removing unnecessary accessors.
** Removing add_refs methods.
* Network::Graph
** Removing unnecessary accessors.
** The 3 times loop of commits don't need.
---
 app/models/network/commit.rb | 21 ++++++---------------
 app/models/network/graph.rb  | 26 ++++++++++++++------------
 2 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/app/models/network/commit.rb b/app/models/network/commit.rb
index 9c0a99ccd41..27c8cc97284 100644
--- a/app/models/network/commit.rb
+++ b/app/models/network/commit.rb
@@ -4,28 +4,19 @@ module Network
   class Commit
     include ActionView::Helpers::TagHelper
 
-    attr_accessor :time, :spaces, :refs, :parent_spaces
+    attr_reader :refs
+    attr_accessor :time, :spaces, :parent_spaces
 
-    def initialize(commit)
-      @_commit = commit
+    def initialize(raw_commit, refs)
+      @commit = ::Commit.new(raw_commit)
       @time = -1
       @spaces = []
       @parent_spaces = []
+      @refs = refs || []
     end
 
     def method_missing(m, *args, &block)
-      @_commit.send(m, *args, &block)
-    end
-
-    def add_refs(ref_cache, repo)
-      if ref_cache.empty?
-        repo.refs.each do |ref|
-          ref_cache[ref.commit.id] ||= []
-          ref_cache[ref.commit.id] << ref
-        end
-      end
-      @refs = ref_cache[@_commit.id] if ref_cache.include?(@_commit.id)
-      @refs ||= []
+      @commit.send(m, *args, &block)
     end
 
     def space
diff --git a/app/models/network/graph.rb b/app/models/network/graph.rb
index c9ecbc910e8..f130bffca14 100644
--- a/app/models/network/graph.rb
+++ b/app/models/network/graph.rb
@@ -2,7 +2,7 @@ require "grit"
 
 module Network
   class Graph
-    attr_accessor :days, :commits, :ref_cache, :repo
+    attr_reader :days, :commits
 
     def self.max_count
       @max_count ||= 650
@@ -13,7 +13,6 @@ module Network
       @ref = ref
       @commit = commit
       @repo = project.repo
-      @ref_cache = {}
 
       @commits = collect_commits
       @days = index_commits
@@ -24,17 +23,11 @@ module Network
     # Get commits from repository
     #
     def collect_commits
-
-      @commits = Grit::Commit.find_all(repo, nil, {date_order: true, max_count: self.class.max_count, skip: to_commit}).dup
-
-      # Decorate with app/models/commit.rb
-      @commits.map! { |commit| Commit.new(commit) }
+      @commits = Grit::Commit.find_all(@repo, nil, {date_order: true, max_count: self.class.max_count, skip: to_commit}).dup
 
       # Decorate with app/model/network/commit.rb
-      @commits.map! { |commit| Network::Commit.new(commit) }
-
-      # add refs to each commit
-      @commits.each { |commit| commit.add_refs(ref_cache, repo) }
+      refs_cache = build_refs_cache
+      @commits.map! { |commit| Network::Commit.new(commit, refs_cache[commit.id]) }
 
       @commits
     end
@@ -78,7 +71,7 @@ module Network
 
     # Skip count that the target commit is displayed in center.
     def to_commit
-      commits = Grit::Commit.find_all(repo, nil, {date_order: true})
+      commits = Grit::Commit.find_all(@repo, nil, {date_order: true})
       commit_index = commits.index do |c|
         c.id == @commit.id
       end
@@ -280,5 +273,14 @@ module Network
         leaves.push(commit)
       end
     end
+
+    def build_refs_cache
+      refs_cache = {}
+      @repo.refs.each do |ref|
+        refs_cache[ref.commit.id] = [] unless refs_cache.include?(ref.commit.id)
+        refs_cache[ref.commit.id] << ref
+      end
+      refs_cache
+    end
   end
 end

From ccc712b1983f49216fc85deb61a7e5efd7790936 Mon Sep 17 00:00:00 2001
From: Sato Hiroyuki <sathiroyuki@gmail.com>
Date: Thu, 7 Mar 2013 19:16:51 +0900
Subject: [PATCH 5/8] Refacor: removing the times array, because that is same
 with @commits array.

---
 app/models/network/graph.rb | 52 ++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/app/models/network/graph.rb b/app/models/network/graph.rb
index f130bffca14..0c0741c3795 100644
--- a/app/models/network/graph.rb
+++ b/app/models/network/graph.rb
@@ -23,13 +23,21 @@ module Network
     # Get commits from repository
     #
     def collect_commits
-      @commits = Grit::Commit.find_all(@repo, nil, {date_order: true, max_count: self.class.max_count, skip: to_commit}).dup
-
-      # Decorate with app/model/network/commit.rb
       refs_cache = build_refs_cache
-      @commits.map! { |commit| Network::Commit.new(commit, refs_cache[commit.id]) }
 
-      @commits
+      Grit::Commit.find_all(
+        @repo,
+        nil,
+        {
+          date_order: true,
+          max_count: self.class.max_count,
+          skip: count_to_display_commit_in_center
+        }
+      )
+      .map do |commit|
+          # Decorate with app/model/network/commit.rb
+          Network::Commit.new(commit, refs_cache[commit.id])
+      end
     end
 
     # Method is adding time and space on the
@@ -40,14 +48,13 @@ module Network
     #
     # @return [Array<TimeDate>] list of commit dates corelated with time on commits
     def index_commits
-      days, times = [], []
+      days = []
       map = {}
 
-      commits.reverse.each_with_index do |c,i|
+      @commits.reverse.each_with_index do |c,i|
         c.time = i
         days[i] = c.committed_date
         map[c.id] = c
-        times[i] = c
       end
 
       @_reserved = {}
@@ -62,17 +69,16 @@ module Network
       end
 
       # find parent spaces for not overlap lines
-      times.each do |c|
-        c.parent_spaces.concat(find_free_parent_spaces(c, map, times))
+      @commits.each do |c|
+        c.parent_spaces.concat(find_free_parent_spaces(c, map))
       end
 
       days
     end
 
     # Skip count that the target commit is displayed in center.
-    def to_commit
-      commits = Grit::Commit.find_all(@repo, nil, {date_order: true})
-      commit_index = commits.index do |c|
+    def count_to_display_commit_in_center
+      commit_index = Grit::Commit.find_all(@repo, nil, {date_order: true}).index do |c|
         c.id == @commit.id
       end
 
@@ -85,7 +91,7 @@ module Network
     end
 
     def commits_sort_by_ref
-      commits.sort do |a,b|
+      @commits.sort do |a,b|
         if include_ref?(a)
           -1
         elsif include_ref?(b)
@@ -108,7 +114,7 @@ module Network
       heads.include?(@ref)
     end
 
-    def find_free_parent_spaces(commit, map, times)
+    def find_free_parent_spaces(commit, map)
       spaces = []
 
       commit.parents.each do |p|
@@ -122,9 +128,9 @@ module Network
                   end
 
           space = if commit.space >= parent.space then
-                    find_free_parent_space(range, parent.space, -1, commit.space, times)
+                    find_free_parent_space(range, parent.space, -1, commit.space)
                   else
-                    find_free_parent_space(range, commit.space, -1, parent.space, times)
+                    find_free_parent_space(range, commit.space, -1, parent.space)
                   end
 
           mark_reserved(range, space)
@@ -135,19 +141,19 @@ module Network
       spaces
     end
 
-    def find_free_parent_space(range, space_base, space_step, space_default, times)
-      if is_overlap?(range, times, space_default) then
+    def find_free_parent_space(range, space_base, space_step, space_default)
+      if is_overlap?(range, space_default) then
         find_free_space(range, space_step, space_base, space_default)
       else
         space_default
       end
     end
 
-    def is_overlap?(range, times, overlap_space)
+    def is_overlap?(range, overlap_space)
       range.each do |i|
         if i != range.first &&
           i != range.last &&
-          times[i].spaces.include?(overlap_space) then
+          @commits[reversed_index(i)].spaces.include?(overlap_space) then
 
           return true;
         end
@@ -282,5 +288,9 @@ module Network
       end
       refs_cache
     end
+
+    def reversed_index(index)
+      -index - 1
+    end
   end
 end

From 79cd1ca3043822d5f763b8a1900fd2bea52b0ff0 Mon Sep 17 00:00:00 2001
From: Sato Hiroyuki <sathiroyuki@gmail.com>
Date: Thu, 7 Mar 2013 19:52:46 +0900
Subject: [PATCH 6/8] Refactor: change the map hash from a local variable to
 private variable.

---
 app/models/network/graph.rb | 65 ++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/app/models/network/graph.rb b/app/models/network/graph.rb
index 0c0741c3795..3504332ffa0 100644
--- a/app/models/network/graph.rb
+++ b/app/models/network/graph.rb
@@ -44,33 +44,31 @@ module Network
     # list of commits. As well as returns date list
     # corelated with time set on commits.
     #
-    # @param [Array<Graph::Commit>] commits to index
-    #
     # @return [Array<TimeDate>] list of commit dates corelated with time on commits
     def index_commits
       days = []
-      map = {}
+      @map = {}
 
       @commits.reverse.each_with_index do |c,i|
         c.time = i
         days[i] = c.committed_date
-        map[c.id] = c
+        @map[c.id] = c
       end
 
-      @_reserved = {}
+      @reserved = {}
       days.each_index do |i|
-        @_reserved[i] = []
+        @reserved[i] = []
       end
 
       commits_sort_by_ref.each do |commit|
-        if map.include? commit.id then
-          place_chain(map[commit.id], map)
+        if @map.include? commit.id then
+          place_chain(commit)
         end
       end
 
       # find parent spaces for not overlap lines
       @commits.each do |c|
-        c.parent_spaces.concat(find_free_parent_spaces(c, map))
+        c.parent_spaces.concat(find_free_parent_spaces(c))
       end
 
       days
@@ -114,12 +112,12 @@ module Network
       heads.include?(@ref)
     end
 
-    def find_free_parent_spaces(commit, map)
+    def find_free_parent_spaces(commit)
       spaces = []
 
       commit.parents.each do |p|
-        if map.include?(p.id) then
-          parent = map[p.id]
+        if @map.include?(p.id) then
+          parent = @map[p.id]
 
           range = if commit.time < parent.time then
                     commit.time..parent.time
@@ -164,23 +162,22 @@ module Network
 
     # Add space mark on commit and its parents
     #
-    # @param [Graph::Commit] the commit object.
-    # @param [Hash<String,Graph::Commit>] map of commits
-    def place_chain(commit, map, parent_time = nil)
-      leaves = take_left_leaves(commit, map)
+    # @param [::Commit] the commit object.
+    def place_chain(commit, parent_time = nil)
+      leaves = take_left_leaves(commit)
       if leaves.empty?
         return
       end
 
       time_range = leaves.last.time..leaves.first.time
-      space_base = get_space_base(leaves, map)
+      space_base = get_space_base(leaves)
       space = find_free_space(time_range, 2, space_base)
       leaves.each do |l|
         l.spaces << space
         # Also add space to parent
         l.parents.each do |p|
-          if map.include?(p.id)
-            parent = map[p.id]
+          if @map.include?(p.id)
+            parent = @map[p.id]
             if parent.space > 0
               parent.spaces << space
             end
@@ -192,8 +189,8 @@ module Network
       min_time = leaves.last.time
       parents = leaves.last.parents.collect
       parents.each do |p|
-        if map.include? p.id
-          parent = map[p.id]
+        if @map.include? p.id
+          parent = @map[p.id]
           if parent.time < min_time
             min_time = parent.time
           end
@@ -209,19 +206,19 @@ module Network
 
       # Visit branching chains
       leaves.each do |l|
-        parents = l.parents.collect.select{|p| map.include? p.id and map[p.id].space.zero?}
+        parents = l.parents.collect.select{|p| @map.include? p.id and @map[p.id].space.zero?}
         for p in parents
-          place_chain(map[p.id], map, l.time)
+          place_chain(p, l.time)
         end
       end
     end
 
-    def get_space_base(leaves, map)
+    def get_space_base(leaves)
       space_base = 1
       if leaves.last.parents.size > 0
         first_parent = leaves.last.parents.first
-        if map.include?(first_parent.id)
-          first_p = map[first_parent.id]
+        if @map.include?(first_parent.id)
+          first_p = @map[first_parent.id]
           if first_p.space > 0
             space_base = first_p.space
           end
@@ -232,7 +229,7 @@ module Network
 
     def mark_reserved(time_range, space)
       for day in time_range
-        @_reserved[day].push(space)
+        @reserved[day].push(space)
       end
     end
 
@@ -241,7 +238,7 @@ module Network
 
       reserved = []
       for day in time_range
-        reserved += @_reserved[day]
+        reserved += @reserved[day]
       end
       reserved.uniq!
 
@@ -260,19 +257,19 @@ module Network
     # Takes most left subtree branch of commits
     # which don't have space mark yet.
     #
-    # @param [Graph::Commit] the commit object.
-    # @param [Hash<String,Graph::Commit>] map of commits
+    # @param [::Commit] the commit object.
     #
-    # @return [Array<Graph::Commit>] list of branch commits
-    def take_left_leaves(commit, map)
+    # @return [Array<Network::Commit>] list of branch commits
+    def take_left_leaves(raw_commit)
+      commit = @map[raw_commit.id]
       leaves = []
       leaves.push(commit) if commit.space.zero?
 
       while true
         return leaves if commit.parents.count.zero?
-        return leaves unless map.include? commit.parents.first.id
+        return leaves unless @map.include? commit.parents.first.id
 
-        commit = map[commit.parents.first.id]
+        commit = @map[commit.parents.first.id]
 
         return leaves unless commit.space.zero?
 

From b7e5f4556ba845cf228eb8cb70f437e2f6792c69 Mon Sep 17 00:00:00 2001
From: Sato Hiroyuki <sathiroyuki@gmail.com>
Date: Thu, 7 Mar 2013 20:36:40 +0900
Subject: [PATCH 7/8] Refactor: Removing the duplicated code.

---
 app/models/network/commit.rb  |  9 +++++
 app/models/network/graph.rb   | 73 ++++++++++++++---------------------
 app/views/graph/show.json.erb |  2 +-
 3 files changed, 38 insertions(+), 46 deletions(-)

diff --git a/app/models/network/commit.rb b/app/models/network/commit.rb
index 27c8cc97284..d0bc61c3bf7 100644
--- a/app/models/network/commit.rb
+++ b/app/models/network/commit.rb
@@ -26,5 +26,14 @@ module Network
         0
       end
     end
+
+    def parents(map)
+      @commit.parents.map do |p|
+        if map.include?(p.id)
+          map[p.id]
+        end
+      end
+      .compact
+    end
   end
 end
diff --git a/app/models/network/graph.rb b/app/models/network/graph.rb
index 3504332ffa0..074ec371fd2 100644
--- a/app/models/network/graph.rb
+++ b/app/models/network/graph.rb
@@ -2,7 +2,7 @@ require "grit"
 
 module Network
   class Graph
-    attr_reader :days, :commits
+    attr_reader :days, :commits, :map
 
     def self.max_count
       @max_count ||= 650
@@ -61,9 +61,7 @@ module Network
       end
 
       commits_sort_by_ref.each do |commit|
-        if @map.include? commit.id then
-          place_chain(commit)
-        end
+        place_chain(commit)
       end
 
       # find parent spaces for not overlap lines
@@ -115,25 +113,21 @@ module Network
     def find_free_parent_spaces(commit)
       spaces = []
 
-      commit.parents.each do |p|
-        if @map.include?(p.id) then
-          parent = @map[p.id]
+      commit.parents(@map).each do |parent|
+        range = if commit.time < parent.time then
+                  commit.time..parent.time
+                else
+                  parent.time..commit.time
+                end
 
-          range = if commit.time < parent.time then
-                    commit.time..parent.time
-                  else
-                    parent.time..commit.time
-                  end
+        space = if commit.space >= parent.space then
+                  find_free_parent_space(range, parent.space, -1, commit.space)
+                else
+                  find_free_parent_space(range, commit.space, -1, parent.space)
+                end
 
-          space = if commit.space >= parent.space then
-                    find_free_parent_space(range, parent.space, -1, commit.space)
-                  else
-                    find_free_parent_space(range, commit.space, -1, parent.space)
-                  end
-
-          mark_reserved(range, space)
-          spaces << space
-        end
+        mark_reserved(range, space)
+        spaces << space
       end
 
       spaces
@@ -175,25 +169,18 @@ module Network
       leaves.each do |l|
         l.spaces << space
         # Also add space to parent
-        l.parents.each do |p|
-          if @map.include?(p.id)
-            parent = @map[p.id]
-            if parent.space > 0
-              parent.spaces << space
-            end
+        l.parents(@map).each do |parent|
+          if parent.space > 0
+            parent.spaces << space
           end
         end
       end
 
       # and mark it as reserved
       min_time = leaves.last.time
-      parents = leaves.last.parents.collect
-      parents.each do |p|
-        if @map.include? p.id
-          parent = @map[p.id]
-          if parent.time < min_time
-            min_time = parent.time
-          end
+      leaves.last.parents(@map).each do |parent|
+        if parent.time < min_time
+          min_time = parent.time
         end
       end
 
@@ -206,7 +193,7 @@ module Network
 
       # Visit branching chains
       leaves.each do |l|
-        parents = l.parents.collect.select{|p| @map.include? p.id and @map[p.id].space.zero?}
+        parents = l.parents(@map).select{|p| p.space.zero?}
         for p in parents
           place_chain(p, l.time)
         end
@@ -215,13 +202,10 @@ module Network
 
     def get_space_base(leaves)
       space_base = 1
-      if leaves.last.parents.size > 0
-        first_parent = leaves.last.parents.first
-        if @map.include?(first_parent.id)
-          first_p = @map[first_parent.id]
-          if first_p.space > 0
-            space_base = first_p.space
-          end
+      parents = leaves.last.parents(@map)
+      if parents.size > 0
+        if parents.first.space > 0
+          space_base = parents.first.space
         end
       end
       space_base
@@ -266,10 +250,9 @@ module Network
       leaves.push(commit) if commit.space.zero?
 
       while true
-        return leaves if commit.parents.count.zero?
-        return leaves unless @map.include? commit.parents.first.id
+        return leaves if commit.parents(@map).count.zero?
 
-        commit = @map[commit.parents.first.id]
+        commit = commit.parents(@map).first
 
         return leaves unless commit.space.zero?
 
diff --git a/app/views/graph/show.json.erb b/app/views/graph/show.json.erb
index 4a8605eec55..d0a0709ac47 100644
--- a/app/views/graph/show.json.erb
+++ b/app/views/graph/show.json.erb
@@ -5,7 +5,7 @@
     days: @graph.days.compact.map { |d| [d.day, d.strftime("%b")] },
     commits: @graph.commits.map do |c|
       {
-        parents: parents_zip_spaces(c.parents, c.parent_spaces),
+        parents: parents_zip_spaces(c.parents(@graph.map), c.parent_spaces),
         author: {
           name: c.author.name,
           email: c.author.email,

From c0d312bebe8ba68b842788e4715c5c8a1612bb29 Mon Sep 17 00:00:00 2001
From: Sato Hiroyuki <sathiroyuki@gmail.com>
Date: Thu, 7 Mar 2013 20:47:04 +0900
Subject: [PATCH 8/8] Refactor: Removing a if statement, because the nil value
 is already removed in the view template.

---
 app/assets/javascripts/branch-graph.js | 96 +++++++++++++-------------
 1 file changed, 47 insertions(+), 49 deletions(-)

diff --git a/app/assets/javascripts/branch-graph.js b/app/assets/javascripts/branch-graph.js
index 520336adc0f..525b17954b0 100644
--- a/app/assets/javascripts/branch-graph.js
+++ b/app/assets/javascripts/branch-graph.js
@@ -118,58 +118,56 @@
       for (var j = 0, jj = this.commits[i].parents.length; j < jj; j++) {
         c = this.preparedCommits[this.commits[i].parents[j][0]];
         ps = this.commits[i].parents[j][1];
-        if (c) {
-          var cx = offsetX + 20 * c.time
-            , cy = offsetY + 10 * c.space
-            , psy = offsetY + 10 * ps;
-          if (c.space == this.commits[i].space && c.space == ps) {
-            r.path([
-              "M", x, y,
-              "L", cx, cy
-            ]).attr({
-              stroke: this.colors[c.space], 
-              "stroke-width": 2
-            });
+        var cx = offsetX + 20 * c.time
+          , cy = offsetY + 10 * c.space
+          , psy = offsetY + 10 * ps;
+        if (c.space == this.commits[i].space && c.space == ps) {
+          r.path([
+            "M", x, y,
+            "L", cx, cy
+          ]).attr({
+            stroke: this.colors[c.space], 
+            "stroke-width": 2
+          });
 
-          } else if (c.space < this.commits[i].space) {
-            if (y == psy) {
-                r.path([
-                    "M", x - 5, y,
-                    "l-5,-2,0,4,5,-2",
-                    "L", x - 10, y,
-                    "L", x - 15, psy,
-                    "L", cx + 5, psy,
-                    "L", cx, cy])
-                .attr({
-                  stroke: this.colors[this.commits[i].space], 
-                  "stroke-width": 2
-                });
-            } else {
-                r.path([
-                    "M", x - 3, y - 6,
-                    "l-4,-3,4,-2,0,5",
-                    "L", x - 5, y - 10,
-                    "L", x - 10, psy,
-                    "L", cx + 5, psy,
-                    "L", cx, cy])
-                .attr({
-                  stroke: this.colors[this.commits[i].space], 
-                  "stroke-width": 2
-                });
-            }
+        } else if (c.space < this.commits[i].space) {
+          if (y == psy) {
+              r.path([
+                  "M", x - 5, y,
+                  "l-5,-2,0,4,5,-2",
+                  "L", x - 10, y,
+                  "L", x - 15, psy,
+                  "L", cx + 5, psy,
+                  "L", cx, cy])
+              .attr({
+                stroke: this.colors[this.commits[i].space], 
+                "stroke-width": 2
+              });
           } else {
-            r.path([
-                "M", x - 3, y + 6,
-                "l-4,3,4,2,0,-5",
-                "L", x - 5, y + 10,
-                "L", x - 10, psy,
-                "L", cx + 5, psy,
-                "L", cx, cy])
-            .attr({
-              stroke: this.colors[c.space], 
-              "stroke-width": 2
-            });
+              r.path([
+                  "M", x - 3, y - 6,
+                  "l-4,-3,4,-2,0,5",
+                  "L", x - 5, y - 10,
+                  "L", x - 10, psy,
+                  "L", cx + 5, psy,
+                  "L", cx, cy])
+              .attr({
+                stroke: this.colors[this.commits[i].space], 
+                "stroke-width": 2
+              });
           }
+        } else {
+          r.path([
+              "M", x - 3, y + 6,
+              "l-4,3,4,2,0,-5",
+              "L", x - 5, y + 10,
+              "L", x - 10, psy,
+              "L", cx + 5, psy,
+              "L", cx, cy])
+          .attr({
+            stroke: this.colors[c.space], 
+            "stroke-width": 2
+          });
         }
       }