PullReview | Review of branch rewrite

test/test_helper.rb:22

lib/logable.rb:25

branchme.rb:36

tagme.rb:37

tagme.rb:81

branchme.rb:82

tagme.rb:88

branchme.rb:89

tagme.rb:90

branchme.rb:91

branchme.rb:93

logme.rb:107

10    err = false
11    unless ruby_compatible?
12      puts "- Ruby #{COMPATIBLE_RUBIES.join(' or ')} required."
13      puts "  Currently using: #{@ruby_version}"
14      err = true
15    end

11    unless ruby_compatible?
12      puts "- Ruby #{COMPATIBLE_RUBIES.join(' or ')} required."
13      puts "  Currently using: #{@ruby_version}"
14      err = true
15    end
16    missing_binaries.each do |m|

15    end
16    missing_binaries.each do |m|
17      puts "- Missing binary: #{m}."
18      err = true
19    end
20    fail 'Not all requirements met.' if err

test/test_helper.rb:22

20    next if File.basename(testfile) == File.basename(__FILE__)
21    next if File.basename(testfile).include?('blackbox')
22    puts "Adding Test File: #{testfile}"
23    require_relative testfile
24  end
25end

lib/logable.rb:25

23  require 'logger/colors'
24rescue LoadError
25  puts 'Logging colors are not available. Install logger-colors gem if desired'
26end
27
28require_relative 'requirements'

branchme.rb:36

34
35if options[:branch].nil?
36    puts "error, you need to set branchname"
37    exit 1
38end
39

tagme.rb:37

35
36if options[:version].nil?
37    puts "error, you need to set version"
38    exit 1
39end
40

tagme.rb:81

79tag_projects = read_release_data()
80tag_projects.each do | tag_project |
81    puts "--- #{tag_project.project.identifier} ---"
82    source = Source.new
83    source.target = "tmp-tagme"
84    source.cleanup()

branchme.rb:82

80tag_projects = read_release_data()
81tag_projects.each do | tag_project |
82    puts "--- #{tag_project.project.identifier} ---"
83    source = Source.new
84    source.target = "tmp-branchme"
85    source.cleanup()

tagme.rb:88

86
87    Dir.chdir(source.target) do
88        puts "::git tag -s -m 'Tagging #{options[:version]}' v#{options[:version]} #{tag_project.git_rev}"
89        %x[git tag -s -m 'Tagging #{options[:version]}' v#{options[:version]} #{tag_project.git_rev}]
90        puts "::git push --tags origin"
91        %x[git push --tags origin]

branchme.rb:89

87
88    Dir.chdir(source.target) do
89        puts "::git branch #{options[:branch]} #{tag_project.git_rev}"
90        %x[git branch #{options[:branch]} #{tag_project.git_rev}]
91        puts "::git checkout #{options[:branch]}"
92        %x[git checkout #{options[:branch]}]

tagme.rb:90

88        puts "::git tag -s -m 'Tagging #{options[:version]}' v#{options[:version]} #{tag_project.git_rev}"
89        %x[git tag -s -m 'Tagging #{options[:version]}' v#{options[:version]} #{tag_project.git_rev}]
90        puts "::git push --tags origin"
91        %x[git push --tags origin]
92    end
93

branchme.rb:91

89        puts "::git branch #{options[:branch]} #{tag_project.git_rev}"
90        %x[git branch #{options[:branch]} #{tag_project.git_rev}]
91        puts "::git checkout #{options[:branch]}"
92        %x[git checkout #{options[:branch]}]
93        puts "::git push origin #{options[:branch]}"
94        %x[git push origin #{options[:branch]}]

branchme.rb:93

91        puts "::git checkout #{options[:branch]}"
92        %x[git checkout #{options[:branch]}]
93        puts "::git push origin #{options[:branch]}"
94        %x[git push origin #{options[:branch]}]
95    end
96

logme.rb:107

105projects.each do | project |
106  # FIXME: lol project.project xD
107  puts "--- #{project.project.identifier} ---"
108  Dir.mktmpdir do |tmpdir|
109    # FIXME: this really could do with some threading.
110    source = Source.new

Why

You don't want to clutter your logs with raw puts, pp, or p. Output using p will not always appear in your logs, nor will it inherit from any log config you may have (to add information such as the timestamp).

How to fix

In a Rails application

Use Rails.logger instead.

In Ruby code

Just use the Logger class.

In unit and integration tests

This is often a sign that you are missing some asserts and other checks.

More info

Rails logger
Stdlib logger
Better logs for Rails using lograge .
Fully configurable logs for any Ruby application using log4r (look what Mitchell Hasimoto of Vagrant fame says about it ).

Know a good reference on this subject ? Tell us about it!

Your branch must be test infected.


                                branchme.rb:1

Details

Production code :

A	branchme.rb
A	lib/cmakeeditor.rb
A	lib/documentation.rb
A	lib/git.rb
A	lib/l10n.rb
A	lib/l10nstatistics.rb
A	lib/logable.rb
A	lib/project.rb
A	lib/projectsfile.rb
A	lib/release.rb
A	lib/requirement_checker.rb
A	lib/requirements.rb
A	lib/source.rb
A	lib/svn.rb
A	lib/translationunit.rb
A	lib/vcs.rb
A	lib/xzarchive.rb
A	logme.rb
A	tagme.rb
A	tarme.rb

Testing code :

Why

The pull request contains production code and no associated test.

Testing is not closely integrated with development. This prevents you from measuring the development's progress - you can't tell when something starts working or stops working. Using Minitest, Rspec, etc. you can build a test suite inexpensively and incrementally that will help you measure your progress, spot unintended side effects, and focus your development efforts.

Test infection

How to fix

Write some tests linked to a new feature or a bug fix.

More info

Know a good reference on this subject ? Tell us about it!

Avoid copy/paste.


                                read_release_data 
       and 1 other locations

Details

branchme.rb:61
tagme.rb:60

Occured at

60def read_release_data()
61    projects = Array.new
62    File.open('release_data', 'r') do | file |
63        file.each_line do | line |
64            parts = line.split(';')
65            next if parts.size < 3 # If we don't manage 3 parts the line is definitely crap.
66            # 0 = project
67            # 1 = branch
68            # 2 = git rev
69            project = TagProject.new
70            project.project = Project.from_xpath(parts[0])[0]
71            project.project.vcs.branch = parts[1]

61def read_release_data()
62    projects = Array.new
63    File.open('release_data', 'r') do | file |
64        file.each_line do | line |
65            parts = line.split(';')
66            next if parts.size < 3 # If we don't manage 3 parts the line is definitely crap.
67            # 0 = project
68            # 1 = branch
69            # 2 = git rev
70            project = TagProject.new
71            project.project = Project::from_xpath(parts[0])[0]
72            project.project.vcs.branch = parts[1]

Why

Code reuse is a standard practice in modern software development. The downside of code reuse via replication and copy-paste programming is that it leads to code bloat, increasing the technical depth of software products and making maintenance costly and time-consuming.

CMakeEditor # create_language_specific_doc_lists!

How to fix

Depending on where the code is duplicated from, various refactorings are available:

Same method: Extract Local Variable
Same class: Extract Method
Same type: Generalization , Pull Up
Different type: Strategy Pattern , Mixin
Different app: Extract a gem

For switch statements, or if-else-if, these should be replaced with polymorphism.

We don't report these duplications yet, but would be interested to know the associated refactoring:

Same content in Views: Partials
Same presentation logic in Views: Helpers

More info

Know a good reference on this subject ? Tell us about it!

Consider refactoring this complex method.


                                CMakeEditor
                    #
                    create_language_specific_doc_lists! 
       and 5 other locations

Occured at

L10n # get

DocumentationL10n # get

Release # get

CMakeEditor # create_language_specific_doc_lists!

55  def create_language_specific_doc_lists!(dir, language, software_name)
56    if File.exist?("#{dir}/index.docbook")
57      # When there is an index.docbook we mustn't copy the en version as
58      # we have to write our own CMakeLists in order to have things installed
59      # in the correct language directory! Also see kdoctools_create_handbook
60      # arguments.
61      write_handbook(dir, language, software_name)
62    elsif !Dir.glob("#{dir}/*").select { |f| File.directory?(f) }.empty?
63      # -- Recyle en' CMakeLists --
64      enusdir = "#{dir}/../en/"
65      enuscmake = "#{enusdir}/CMakeLists.txt"
66      if File.exist?(enuscmake)
67        # FIXME: naughty
68        FileUtils.cp(enuscmake, dir) unless File.basename(dir) == 'en'
69        Dir.glob("#{dir}/**/**").each do |current_dir|
70          next unless File.directory?(current_dir)
71          next if File.basename(dir) == 'en'
72          dir_pathname = Pathname.new(dir)
73          current_dir_pathname = Pathname.new(current_dir)
74          relative_path = current_dir_pathname.relative_path_from(dir_pathname)
75          # FIXME: this will fail if the cmakelists doesn't exist, which is
76          #        possible but also a bit odd, not sure if we should just
77          #        ignore that...
78          # FIXME: has no test backing I think
79          cmakefile = "#{enusdir}/#{relative_path}/CMakeLists.txt"
80          FileUtils.cp(cmakefile, current_dir) if File.exist?(cmakefile)
81        end
82        Dir.glob("#{dir}/**/index.docbook").each do |docbook|
83          # FIXME: subdir logic needs testing through documentation class
84          # FIXME: this is not tested via our tests
85          dirname = File.dirname(docbook)
86
87          dir_pathname = Pathname.new(dir)
88          current_dir_pathname = Pathname.new(dirname)
89          relative_path = current_dir_pathname.relative_path_from(dir_pathname)
90

L10n # get

107  def get(srcdir)
108    # FIXME: this is later used as destination for the weirdest of reasons...
109    target = "#{srcdir}/po/"
110    Dir.mkdir(target)
111
112    @templates = find_templates(srcdir)
113    log_info "Downloading translations for #{srcdir}"
114
115    languages_without_translation = []
116    has_translation = false
117    # FIXME: due to threading we do explicit pathing, so this probably can go
118    Dir.chdir(srcdir) do
119      queue = languages_queue
120      threads = []
121      THREAD_COUNT.times do
122        threads << Thread.new do
123          Thread.current.abort_on_exception = true
124          until queue.empty?
125            language = queue.pop(true)
126            Dir.mktmpdir(self.class.to_s) do |tmpdir|
127              log_debug "#{srcdir} - downloading #{language}"
128              if templates.count > 1
129                files = get_multiple(language, tmpdir)
130              elsif templates.count == 1
131                files = get_single(language, tmpdir)
132              else
133                # FIXME: needs testcase
134                # TODO: this previously aborted entirely, not sure that makes
135                #       sense with threading
136                next # No translations need fetching
137              end
138
139              # No files obtained :(
140              if files.empty?
141                # FIXME: not thread safe without GIL
142                languages_without_translation << language
143                next
144              end
145              # FIXME: not thread safe without GIL
146              has_translation = true
147
148              # TODO: path confusing with target
149              destination = "po/#{language}"

29  def gather!(srcdir)
30    podir = "#{srcdir}/po/"
31    Dir.chdir(podir) do
32      languages = Dir.glob('*')
33      languages.each do |language|
34        next unless File.directory?(language)
35        Dir.chdir(language) do
36          translated = 0
37          fuzzy = 0
38          untranslated = 0
39
40          Dir.glob('*.po').each do |file|
41            data = `LC_ALL=C LANG=C msgfmt --statistics #{file} > /dev/stdout 2>&1`
42
43            # tear the data apart and create some variables
44            data.split(',').each do |x|
45              if x.include?('untranslated')
46                untranslated += x.scan(/[\d]+/)[0].to_i
47              elsif x.include?('fuzzy')
48                fuzzy += x.scan(/[\d]+/)[0].to_i
49              elsif x.include?('translated')
50                translated += x.scan(/[\d]+/)[0].to_i
51              end
52            end
53          end
54
55          all = translated + fuzzy + untranslated
56          notshown = fuzzy + untranslated
57          shown = all - notshown
58          percentage = ((100.0 * shown.to_f) / all.to_f)
59
60          @stats[language] = {
61            :all => all,
62            :shown => shown,
63            :notshown => notshown,

DocumentationL10n # get

37  def get(srcdir)
38    @docdir = "#{File.expand_path(srcdir)}/doc"
39    FileUtils.mkpath(@docdir)
40
41    docs = []
42    languages_without_documentation = []
43
44    log_info "Downloading documentations for #{srcdir}"
45
46    unless get_en('en')
47      log_warn 'There is no en documentation. Aborting :('
48      return
49    end
50    docs << 'en'
51
52    queue = languages_queue(%w(en))
53    threads = []
54    THREAD_COUNT.times do
55      threads << Thread.new do
56        Thread.current.abort_on_exception = true
57        until queue.empty?
58          language = queue.pop(true)
59          if get_language(language)
60            docs << language
61          else
62            languages_without_documentation << language
63          end
64        end
65      end
66    end
67    ThreadsWait.all_waits(threads)
68
69    if !docs.empty?

115  def get_language(language)
116    destdir = "#{@docdir}/#{language}"
117
118    Dir.mktmpdir(self.class.to_s) do |tmpdir|
119      unless doc_dirs.empty?
120        # FIXME: recyle for single-get?
121        # FIXME: check cmake file for add_subdir that are not optional and warn if there are any
122        @vcs.get(tmpdir, "#{language}/docs/#{@i18n_path}")
123
124        not_translated_doc_dirs = doc_dirs
125        # FIXME: for some reason with plasma-desktop /* didn't work
126        #        yet the tests passed, so the tests seem insufficient
127        doc_selection = Dir.glob("#{tmpdir}/**").select do |d|
128          basename = File.basename(d)
129          if doc_dirs.include?(basename)
130            not_translated_doc_dirs.delete(basename)
131            next true
132          end
133          next false
134        end
135        break if doc_selection.empty?
136
137        FileUtils.mkpath(destdir)
138        doc_selection.each { |d| FileUtils.mv(d, destdir) }
139        CMakeEditor.create_language_specific_doc_lists!(destdir, language, @project_name)
140        return true

Release # get

68  def get
69    log_info "Getting source #{project.vcs}"
70    source.cleanup
71    source.get(project.vcs)
72
73    # FIXME: one would think that perhaps l10n could be disabled entirely
74    log_info ' Getting translations...'
75    # FIXME: why not pass project itself? Oo
76    # FIXME: origin should be validated? technically optparse enforces proper values
77    l10n = L10n.new(origin, project.identifier, project.i18n_path)
78    l10n.get(source.target)
79
80    log_info ' Getting documentation...'

Why

This test reports methods with a Flog metric score that is higher than the threshold. The Flog metric is very similar to the cyclomatic complexity metric but also takes Ruby-specific statements into account. For example, calls to meta-programming methods such as define_method or class_eval are weighted higher than regular method calls.

Methods have a very large number of possible paths. This is bad because the behaviour of code with lots of branches or evals is difficult to understand and test and can also be an important source of errors.

How to fix

Complexity can be reduced in several ways:

By splitting responsibilities

The method may be doing too many different things. A clear indication of that is code like this:

class Rocket
  def launch
    # fire the propellant
    # (lots of code)

    # ignite the afterburners
    # (still more code)

    # remote tower control
    # (even more code)
  end

Comments such as these are good indicators that each of them should probably be a method in itself. Shorter methods with more dedicated roles are easier to test and understand, even if they are private and not reused (yet).

By reducing the usage of meta-programming

Ruby has a lot of dynamic options like eval, method_missing and send. Whilst very powerful, they also make the code more complex, especially for fellow programmers. Try to use them less whenever possible - straightforward code is a boon to all maintainers.

By reducing branching

A method with a lot of if or switch can sometimes be simplified by proper usage of Exception (instead of if guard clauses), polymorphism (instead of switch statements) or lambdas (that can be used as dedicated code snippets to be applied depending on conditions).

More info

Our take on Complexity
Replacing conditionals with polymorphism
Use Exceptions instead of guard clauses
Modularity. Details. Pick one.
Refactoring Notes
The cost of if
Quote from Sandi Metz : "Complexity is only justified if it results in higher levels of abstraction that reduce duplication and increase reusability."

Know a good reference on this subject ? Tell us about it!

Consider refactoring this complex block.


                                L10n
                    #
                    get 
       and 5 other locations

Occured at

L10n # get

L10n # get

L10n # get

L10n # get

116    has_translation = false
117    # FIXME: due to threading we do explicit pathing, so this probably can go
118    Dir.chdir(srcdir) do
119      queue = languages_queue
120      threads = []
121      THREAD_COUNT.times do

29  def gather!(srcdir)
30    podir = "#{srcdir}/po/"
31    Dir.chdir(podir) do
32      languages = Dir.glob('*')
33      languages.each do |language|
34        next unless File.directory?(language)

31    Dir.chdir(podir) do
32      languages = Dir.glob('*')
33      languages.each do |language|
34        next unless File.directory?(language)
35        Dir.chdir(language) do
36          translated = 0

116    destdir = "#{@docdir}/#{language}"
117
118    Dir.mktmpdir(self.class.to_s) do |tmpdir|
119      unless doc_dirs.empty?
120        # FIXME: recyle for single-get?
121        # FIXME: check cmake file for add_subdir that are not optional and warn if there are any

L10n # get

119      queue = languages_queue
120      threads = []
121      THREAD_COUNT.times do
122        threads << Thread.new do
123          Thread.current.abort_on_exception = true
124          until queue.empty?

L10n # get

120      threads = []
121      THREAD_COUNT.times do
122        threads << Thread.new do
123          Thread.current.abort_on_exception = true
124          until queue.empty?
125            language = queue.pop(true)

Why

This check reports blocks with a cyclomatic complexity metric score that is higher than the threshold. The cyclomatic complexity metric counts the number of linearly independent paths throughout the code, adding one point of each:

if
else
unless
while
until
for
rescue
case
when
and
or

How to fix

Reducing cyclomatic complexity means reducing branching. A block with a lot of if or switch can sometimes be simplified by a proper usage of Exception (alternative to if guard clauses), polymorphism (alternative to switch statements) or lambdas (which can be used as dedicated code snippets to be applied depending on conditions).

More often than not, having a block with a large amount of branches probably means that its objective is not (or too broadly) defined.

More info

Know a good reference on this subject ? Tell us about it!

Avoid copy/paste.


                                TestDocumentation
                    #
                    setup 
       and 1 other locations

Details

test/test_documentation.rb:10
test/test_l10n.rb:12

Occured at

10  def setup
11    # FIXME: code copy with l10n
12    @repoDataDir = data('l10nrepo/')
13
14    @i18n_path = 'extragear-multimedia'
15
16    @trunkUrl = 'trunk/l10n-kf5/'
17    @stableUrl = 'branches/stable/l10n-kf5'
18
19    @dir = "tmp_l10n_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
20    @svnTemplateDir = "tmp_l10n_repo_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
21    @svnCheckoutDir = "tmp_l10n_check_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

12    def setup
13        @repoDataDir = data("l10nrepo/")
14
15        @i18n_path = "extragear-multimedia"
16
17        @trunkUrl = "trunk/l10n-kf5/"
18        @stableUrl = "branches/stable/l10n-kf5"
19
20        @dir = "tmp_l10n_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
21        @svnTemplateDir = "tmp_l10n_repo_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
22        @svnCheckoutDir = "tmp_l10n_check_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
23

Why

How to fix

Depending on where the code is duplicated from, various refactorings are available:

Same method: Extract Local Variable
Same class: Extract Method
Same type: Generalization , Pull Up
Different type: Strategy Pattern , Mixin
Different app: Extract a gem

For switch statements, or if-else-if, these should be replaced with polymorphism.

We don't report these duplications yet, but would be interested to know the associated refactoring:

Same content in Views: Partials
Same presentation logic in Views: Helpers

More info

Know a good reference on this subject ? Tell us about it!

Consider refactoring, similar code detected.


                                Svn
                    #
                    run 
       and 1 other locations

Details

lib/git.rb:70
lib/svn.rb:33

Occured at

Svn # run

Git # run

Svn # run

33  def run(args)
34    cmd = "svn #{args.join(' ')} 2>&1"
35    log_debug cmd
36    output = `#{cmd}`
37    unless logger.level != Logger::DEBUG || output.empty?
38      log_debug '-- output --'
39      output.lines.each { |l| log_debug l.rstrip }
40      log_debug '------------'
41    end
42    # Do not return error output as it will screw with output processing.
43    output = '' if $? != 0
44    output

Git # run

70  def run(args)
71    cmd = "git #{args.join(' ')} 2>&1"
72    log_debug cmd
73    output = `#{cmd}`
74    unless logger.level != Logger::DEBUG || output.empty?
75      log_debug '-- output --'
76      output.lines.each { |l| log_debug l.rstrip }
77      log_debug '------------'
78    end
79    # Do not return error output as it will screw with output processing.
80    output = '' if $? != 0
81    output

Why

WARNING: the code is similar not identical. Call parameters from the different sources may differ slightly.

How to fix

Depending on where the code is duplicated from, various refactoring is available:

Same method: Extract Local Variable
Same class: Extract Method
Same type: Generalization , Pull Up
Different type: Strategy Pattern , Mixin
Different app: Extract a gem

For switch statements, or if-else-if, these should be replaced with polymorphism.

We don't report these duplications yet, but would be interested to know the associated refactoring:

Same content in Views: Partials
Same presentation logic in Views: Helpers

More info

Know a good reference on this subject ? Tell us about it!

Avoid copy/paste.


                                TestGit
                    #
                    setup 
       and 1 other locations

Details

test/test_git.rb:34
test/test_release.rb:36

Occured at

TestGit # setup

TestRelease # setup

TestGit # setup

34  def setup
35    # Create a test remote
36    Dir.mkdir('remote')
37    Dir.chdir('remote') do
38      `git init --bare .`
39    end
40    @remotedir = "#{Dir.pwd}/remote"
41
42    setup_repo_content
43
44    # Teardown happens automatically when the @tmpdir is torn down
45  end

TestRelease # setup

36  def setup
37    # Create a test remote
38    Dir.mkdir('remote')
39    Dir.chdir('remote') do
40      `git init --bare .`
41    end
42    @remotedir = "#{Dir.pwd}/remote"
43
44    setup_repo_content
45
46    # Teardown happens automatically when the @tmpdir is torn down
47  end

Why

How to fix

Depending on where the code is duplicated from, various refactorings are available:

Same method: Extract Local Variable
Same class: Extract Method
Same type: Generalization , Pull Up
Different type: Strategy Pattern , Mixin
Different app: Extract a gem

For switch statements, or if-else-if, these should be replaced with polymorphism.

We don't report these duplications yet, but would be interested to know the associated refactoring:

Same content in Views: Partials
Same presentation logic in Views: Helpers

More info

Know a good reference on this subject ? Tell us about it!

Consider refactoring this complex method.


                                TestProjectFile
                    #
                    test_load_http 
       and 7 other locations

Occured at

TestProjectFile # test_load_http

TestDocumentation # test_get_doc

TestL10n # test_get_po

TestBlackboxTarme # test_git

TestCMakeEditor # test_create_handbook_complex

TestProjectFile # test_load_http

 79  def test_load_http
 80    # Revert the xml twiddling from setup and only divert the cache.
 81    # We need the canonical xml url for this test.
 82    file = ProjectsFile.xml_path
 83    ProjectsFile.reset!
 84    setup_caching
 85
 86    etag = '123098'
 87
 88    assert(!File.exist?(@cache_file))
 89    assert(!File.exist?(@cache_file_etag))
 90
 91    # Request against stub, we are expecting our cache files as a result.
 92    stub = stub_request(:any, 'projects.kde.org/kde_projects.xml')
 93    stub.to_return do |_|
 94      {
 95        body: File.read(file),
 96        headers: { 'etag' => etag }
 97      }
 98    end
 99    ProjectsFile.load!
100    assert_not_nil(ProjectsFile.xml_data)
101    assert_not_nil(ProjectsFile.xml_doc)
102    assert(File.exist?(@cache_file), 'cache file missing')
103    assert(File.exist?(@cache_file_etag), 'etag cache missing')
104    remove_request_stub(stub)
105
106    # Now that we have a cache, try to use the cache.
107    prev_mtime = File.mtime(@cache_file)
108    stub = stub_request(:any, 'projects.kde.org/kde_projects.xml')
109    stub.to_return do |request|
110      assert(request.headers.key?('If-None-Match'))
111      { status: [304, 'Not Modified'] }
112    end
113    ProjectsFile.load!
114    assert_not_nil(ProjectsFile.xml_data)
115    assert_not_nil(ProjectsFile.xml_doc)
116    assert(File.exist?(@cache_file), 'cache file missing')
117    assert(File.exist?(@cache_file_etag), 'etag cache missing')
118    remove_request_stub(stub)
119    assert_equal(prev_mtime, File.mtime(@cache_file),
120                 'Cache file was modified but should not have been.')
121
122    # And again, but this time we want the cache to update.
123    prev_mtime = File.mtime(@cache_file)
124    stub = stub_request(:any, 'projects.kde.org/kde_projects.xml')
125    stub.to_return do |request|
126      assert(request.headers.key?('If-None-Match'))
127      {

TestDocumentation # test_get_doc

50  def test_get_doc
51    # en & de
52    d = create_doc
53    d.init_repo_url("file://#{Dir.pwd}/#{@svnTemplateDir}")
54    FileUtils.rm_rf(@dir)
55    FileUtils.cp_r(data('single-pot'), @dir)
56    d.get(@dir)
57    assert(File.exist?("#{@dir}/CMakeLists.txt"))
58    assert(File.exist?("#{@dir}/doc/CMakeLists.txt"))
59    assert(File.exist?("#{@dir}/doc/en/index.docbook"))
60    assert(File.exist?("#{@dir}/doc/en/CMakeLists.txt"))
61    assert(File.exist?("#{@dir}/doc/de/index.docbook"))
62    assert(File.exist?("#{@dir}/doc/de/CMakeLists.txt"))
63
64    # en only (everything works if only doc/ is present in git but not
65    # translated)
66    d = create_doc_without_translation
67    d.init_repo_url("file://#{Dir.pwd}/#{@svnTemplateDir}")
68    FileUtils.rm_rf(@dir)
69    FileUtils.cp_r(data('single-pot'), @dir)
70    d.get(@dir)
71    assert(File.exist?("#{@dir}/CMakeLists.txt"))
72    assert(File.exist?("#{@dir}/doc/CMakeLists.txt"))
73    assert(File.exist?("#{@dir}/doc/en/index.docbook"))
74    assert(File.exist?("#{@dir}/doc/en/CMakeLists.txt"))
75    assert(!File.exist?("#{@dir}/doc/de/index.docbook"))

10  def setup
11    # FIXME: code copy with l10n
12    @repoDataDir = data('l10nrepo/')
13
14    @i18n_path = 'extragear-multimedia'
15
16    @trunkUrl = 'trunk/l10n-kf5/'
17    @stableUrl = 'branches/stable/l10n-kf5'
18
19    @dir = "tmp_l10n_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
20    @svnTemplateDir = "tmp_l10n_repo_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
21    @svnCheckoutDir = "tmp_l10n_check_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
22
23    `svnadmin create #{@svnTemplateDir}`
24    assert(File.exist?(@svnTemplateDir))
25
26    `svn co file://#{Dir.pwd}/#{@svnTemplateDir} #{@svnCheckoutDir}`
27    FileUtils.cp_r("#{@repoDataDir}/trunk", @svnCheckoutDir)
28    FileUtils.cp_r("#{@repoDataDir}/branches", @svnCheckoutDir)
29    Dir.chdir(@svnCheckoutDir) do

12    def setup
13        @repoDataDir = data("l10nrepo/")
14
15        @i18n_path = "extragear-multimedia"
16
17        @trunkUrl = "trunk/l10n-kf5/"
18        @stableUrl = "branches/stable/l10n-kf5"
19
20        @dir = "tmp_l10n_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
21        @svnTemplateDir = "tmp_l10n_repo_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
22        @svnCheckoutDir = "tmp_l10n_check_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
23
24        `svnadmin create #{@svnTemplateDir}`
25        assert(File.exist?(@svnTemplateDir))
26
27        `svn co file://#{Dir.pwd}/#{@svnTemplateDir} #{@svnCheckoutDir}`
28        FileUtils.cp_r("#{@repoDataDir}/trunk", @svnCheckoutDir)
29        FileUtils.cp_r("#{@repoDataDir}/branches", @svnCheckoutDir)
30        Dir.chdir(@svnCheckoutDir) do
31          `svn add *`

TestL10n # test_get_po

58    def test_get_po
59        l = create_l10n
60        l.init_repo_url("file://#{Dir.pwd}/#{@svnTemplateDir}")
61
62        FileUtils.rm_rf(@dir)
63        FileUtils.cp_r(data("single-pot"), @dir)
64        l.get(@dir)
65        assert(File.exist?("#{@dir}"))
66        assert(File.exist?("#{@dir}/CMakeLists.txt"))
67        assert(!File.exist?("#{@dir}/l10n")) # temp dir must not be there
68        assert(File.exist?("#{@dir}/po"))
69        assert(File.exist?("#{@dir}/po/de/amarok.po"))
70
71        FileUtils.rm_rf(@dir)
72        FileUtils.cp_r(data("multi-pot"), @dir)
73        l.get(@dir)
74        assert(File.exist?("#{@dir}"))
75        assert(File.exist?("#{@dir}/CMakeLists.txt"))
76        assert(!File.exist?("#{@dir}/l10n")) # temp dir must not be there
77        assert(File.exist?("#{@dir}/po"))
78        assert(File.exist?("#{@dir}/po/de/amarok.po"))
79        assert(File.exist?("#{@dir}/po/de/amarokcollectionscanner_qt.po"))
80    end

TestBlackboxTarme # test_git

TestCMakeEditor # test_create_handbook_complex

 7    def test_git
 8        origin = "trunk"
 9        version = "1.1.1.1"
10        name = "libdebconf-kde"
11        # FIXME: this really should be done through xzarchive somehow
12        expected_dirname = "#{name}-#{version}"
13        expected_tarname = "#{expected_dirname}.tar.xz"
14
15        ret = system("ruby #{@testdir}/../tarme.rb --origin #{origin} --version #{version} #{name}")
16        assert(ret)
17        assert(File.exist?(expected_tarname))
18        expected_files = %w[
19            .
20            CMakeLists.txt
21            Messages.sh
22            po/de/libdebconf-kde.po
23        ]
24        expected_files.each do |expected_file|
25            assert(File.exist?("#{expected_dirname}/#{expected_file}"), "File #{expected_file} not found in directory")
26        end
27
28        # Move base directory out of the way and extract a canonical version from
29        # the tar. Must have the same files!

60  def test_create_handbook_complex
61    origin_dir = "#{@datadir}/cmakeeditor/#{__method__}"
62    FileUtils.cp_r(Dir.glob("#{origin_dir}/*"), '.')
63    %w(en de fr).each do |lang|
64      CMakeEditor.create_language_specific_doc_lists!("#{Dir.pwd}/#{lang}", lang, 'yolo')
65    end
66    # FIXME: put in testme as assert_files_exist
67    expected_files = %w(
68      CMakeLists.txt
69      fr
70      fr/CMakeLists.txt
71      fr/doc2
72      fr/doc2/CMakeLists.txt
73      fr/doc2/index.docbook
74      en
75      en/CMakeLists.txt
76      en/doc1
77      en/doc1/CMakeLists.txt
78      en/doc1/index.docbook
79      en/doc2
80      en/doc2/CMakeLists.txt
81      en/doc2/index.docbook
82      en/doc3
83      en/doc3/CMakeLists.txt
84      en/doc3/doc3.1
85

102  def test_export
103    tmpDir = Dir.pwd + "/tmp_svn_export_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
104    Dir.mkdir(tmpDir)
105
106    populate_repo
107    s = new_valid_repo
108
109    # Valid target and path
110    ret = s.export("#{tmpDir}/file", '/dir/file')
111    assert_equal(ret, true)
112    assert(File.exist?("#{tmpDir}/file"))
113
114    # Target dir does not exist
115    ret = s.export("#{tmpDir}123/file", '/dir/file')
116    assert_equal(ret, false)
117    assert(!File.exist?("#{tmpDir}123/file"))
118
119    # Invalid path
120    ret = s.export("#{tmpDir}/file", '/dir/otherfile')

Why

How to fix

Complexity can be reduced in several ways:

By splitting responsibilities

The method may be doing too many different things. A clear indication of that is code like this:

class Rocket
  def launch
    # fire the propellant
    # (lots of code)

    # ignite the afterburners
    # (still more code)

    # remote tower control
    # (even more code)
  end

By reducing the usage of meta-programming

By reducing branching

More info

Our take on Complexity
Replacing conditionals with polymorphism
Use Exceptions instead of guard clauses
Modularity. Details. Pick one.
Refactoring Notes
The cost of if
Quote from Sandi Metz : "Complexity is only justified if it results in higher levels of abstraction that reduce duplication and increase reusability."

Know a good reference on this subject ? Tell us about it!

Reduce the number of parameters.


                                Logable
                    ::
                    Methods
                    #
                    create_logger

Logable :: Methods # create_logger

76      @__logger.level = Logger::INFO
77      @__logger.level = Logger::DEBUG if ENV['RELEASEME_DEBUG']
78      logger.formatter = proc do |severity, _datetime, progname, msg|
79        "#{severity} -- #{progname}: #{msg}\n"
80      end
81      # Module classes are not useful, use the actual module name if we are

Why

Let's assume

def my_method(width, height, show_border, show_header)
  # ...
end

when you call it

my_method(400, 50, false, true)

What are the meanings of numbers 400 and 50, the false and true?

How to fix

Parameters as hash options

When you use Hash options replacing standard arguments, you lose clarity in the method signature, but gain clarity whenever you call the method thanks to the pseudo-named parameters by using Hash options.

method signature def my_method(options) height = options[:height] #... end

method call my_method(width: 400, height: 50, show_border: false, show_header: true) my_method(show_header: true, show_border: false, width: 400, height: 50)

You don't need to use the same order in invocation and the key documents the attributes. The invocation is more readable, the implementation should now deal with:

the presence/validation of the parameters,
misspelled keys, and
wrong types.

my_method(show_header: 400, show_border: 50, withs: 400, haights: 50)

Introduce a real model

Group related attributes:

class TableSettings
  attr_accessor :show_header, :show_border
end

Then use them

dimension = Dimension.new(width: 400, height: 50)
table_setting = TableSettings.new(show_header: true, show_border: false)
my_method(dimension, table_settings)

Ruby 2.0 keyword arguments

Ruby 2.0 introduces keyword arguments:

def my_method(width: 400, height: 50, show_border: false, show_header: true)
  # ...
end

More info

Know a good reference on this subject ? Tell us about it!

Omit the parentheses in defs when the method doesn't accept any arguments.


                                XzArchive
                    #
                    initialize 
       and 4 other locations

Occured at

XzArchive # initialize

XzArchive # create

lib/project.rb:70

XzArchive # initialize

38    def initialize()

XzArchive # create

53    def create()

60def read_release_data()

61def read_release_data()

lib/project.rb:70

TestCMakeEditor # test_append_po_install_instructions

70    def resolve_attributes!()

Why

The parentheses in method definition help to differenciate arguments. Don't use them when the method has no arguments at all:

# bad
def save_user()
  # ...
end

How to fix

Just remove the parentheses:

# good
def save_user
  # ...
end

More info

Know a good reference on this subject ? Tell us about it!

Consider refactoring, similar code detected.


                                TestCMakeEditor
                    #
                    test_append_po_install_instructions 
       and 1 other locations

Details

test/test_cmakeeditor.rb:126
test/test_cmakeeditor.rb:158

Occured at

TestCMakeEditor # test_append_optional_add_subdirectory_append

TestCMakeEditor # test_append_po_install_instructions

TestCMakeEditor # test_append_optional_add_subdirectory_append

126    assert_equal(CMakeEditor.create_handbook('xx', 'yolo'), data)
127    assert_has_terminal_newline(data)
128  end
129
130  def test_create_handbook_with_subpath
131    output = CMakeEditor.create_handbook('es', 'kittens/kittentroll')
132    assert(output.include?('${HTML_INSTALL_DIR}/es'))
133    assert(output.include?('SUBDIR kittens/kittentroll'))
134  end
135
136  def test_create_doc_meta_lists
137    Dir.mkdir("#{Dir.pwd}/aa")

158    CMakeEditor.append_po_install_instructions!(Dir.pwd, 'po')
159    # FIXME: lots of code dup like this
160    assert(File.exist?('CMakeLists.txt'))
161    data = File.read('CMakeLists.txt')
162    assert(data.include?("#FOO_SUBDIR\n"))
163    assert(data.include?('ki18n_install(po)'))
164    assert_has_terminal_newline(data)
165    # Make sure the editor doesn't append if it is already there...
166    CMakeEditor.append_po_install_instructions!(Dir.pwd, 'po')
167    data = File.read('CMakeLists.txt')
168    assert(data.scan('ki18n_install(po)').count == 1)
169  end

Why

WARNING: the code is similar not identical. Call parameters from the different sources may differ slightly.

How to fix

Depending on where the code is duplicated from, various refactoring is available:

Same method: Extract Local Variable
Same class: Extract Method
Same type: Generalization , Pull Up
Different type: Strategy Pattern , Mixin
Different app: Extract a gem

For switch statements, or if-else-if, these should be replaced with polymorphism.

We don't report these duplications yet, but would be interested to know the associated refactoring:

Same content in Views: Partials
Same presentation logic in Views: Helpers

More info

Know a good reference on this subject ? Tell us about it!

Consider refactoring, similar code detected.


                                TestGit
                    #
                    setup_repo_content 
       and 1 other locations

Details

test/test_git.rb:22
test/test_release.rb:22

Occured at

TestGit # setup_repo_content

TestRelease # setup_repo_content

TestGit # setup_repo_content

22  def setup_repo_content
23    `git clone #{@remotedir} clone`
24    Dir.chdir('clone') do
25      File.write('abc', 'content')
26      `git add abc`
27      `git commit -a -m 'import'`
28      `git push origin master`
29    end
30  ensure
31    FileUtils.rm_rf('clone')
32  end
33

TestRelease # setup_repo_content

22  def setup_repo_content
23    `git clone #{@remotedir} clone`
24    Dir.chdir('clone') do
25      File.write('file', 'content')
26      `git add file`
27      `git commit -a -m 'import'`
28      `git push origin master`
29    end
30  ensure
31    FileUtils.rm_rf('clone')
32  end
33

Why

WARNING: the code is similar not identical. Call parameters from the different sources may differ slightly.

How to fix

Depending on where the code is duplicated from, various refactoring is available:

Same method: Extract Local Variable
Same class: Extract Method
Same type: Generalization , Pull Up
Different type: Strategy Pattern , Mixin
Different app: Extract a gem

For switch statements, or if-else-if, these should be replaced with polymorphism.

We don't report these duplications yet, but would be interested to know the associated refactoring:

Same content in Views: Partials
Same presentation logic in Views: Helpers

More info

Know a good reference on this subject ? Tell us about it!

Consider refactoring, similar code detected.


                                TestSvn
                    #
                    setup 
       and 1 other locations

Details

test/test_svn.rb:29
test/test_svn.rb:30

Occured at

29    @svn_checkout_dir = "#{Dir.pwd}/tmp_check_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
30    @svn_repo_dir = "#{Dir.pwd}/tmp_repo_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
31    `svnadmin create #{@svn_repo_dir}`
32    assert(File.exist?(@svn_repo_dir))
33  end
34
35  def teardown
36    FileUtils.rm_rf(@svn_repo_dir)
37    FileUtils.rm_rf(@svn_checkout_dir)
38  end
39
40  def populate_repo

30    @svn_repo_dir = "#{Dir.pwd}/tmp_repo_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join
31    `svnadmin create #{@svn_repo_dir}`
32    assert(File.exist?(@svn_repo_dir))
33  end
34
35  def teardown
36    FileUtils.rm_rf(@svn_repo_dir)
37    FileUtils.rm_rf(@svn_checkout_dir)
38  end
39
40  def populate_repo
41    `svn co file://#{@svn_repo_dir} #{@svn_checkout_dir}`

Why

WARNING: the code is similar not identical. Call parameters from the different sources may differ slightly.

How to fix

Depending on where the code is duplicated from, various refactoring is available:

Same method: Extract Local Variable
Same class: Extract Method
Same type: Generalization , Pull Up
Different type: Strategy Pattern , Mixin
Different app: Extract a gem

For switch statements, or if-else-if, these should be replaced with polymorphism.

We don't report these duplications yet, but would be interested to know the associated refactoring:

Same content in Views: Partials
Same presentation logic in Views: Helpers

More info

Know a good reference on this subject ? Tell us about it!

Consider refactoring, similar code detected.


                                TestProjectConfig
                    #
                    test_invalid_vcs 
       and 1 other locations

Details

test/test_project.rb:150
test/test_project.rb:158

Occured at

TestProjectConfig # test_invalid_vcs

TestProjectConfig # test_invalid_vcs_type

TestProjectConfig # test_invalid_vcs

TestProjectConfig # test_invalid_vcs_type

150  def test_invalid_vcs
151    Project.class_variable_set(:@@configdir, data('projects/'))
152    name = 'invalid-vcs'
153    assert_raise NoMethodError do
154      Project.from_config(name)
155    end
156  end
157
158  def test_invalid_vcs_type
159    Project.class_variable_set(:@@configdir, data('projects/'))
160    name = 'invalid-vcs-type'
161    assert_raise RuntimeError do

158  def test_invalid_vcs_type
159    Project.class_variable_set(:@@configdir, data('projects/'))
160    name = 'invalid-vcs-type'
161    assert_raise RuntimeError do
162      Project.from_config(name)
163    end
164  end
165end
166
167class TestProject < Testme
168    def setup
169        # Project uses ProjectsFile to read data, so we need to make sure it

Why

WARNING: the code is similar not identical. Call parameters from the different sources may differ slightly.

How to fix

Depending on where the code is duplicated from, various refactoring is available:

Same method: Extract Local Variable
Same class: Extract Method
Same type: Generalization , Pull Up
Different type: Strategy Pattern , Mixin
Different app: Extract a gem

For switch statements, or if-else-if, these should be replaced with polymorphism.

We don't report these duplications yet, but would be interested to know the associated refactoring:

Same content in Views: Partials
Same presentation logic in Views: Helpers

More info

Know a good reference on this subject ? Tell us about it!

Reduce the number of parameters.


                                FakeClass2
                    #
                    create_logger 
       and 2 other locations

Occured at

FakeClass2 # create_logger

M # create

C # log

FakeClass2 # create_logger

38    logger = Logger.new(log_file)
39    logger.level = log_level
40    logger.formatter = proc do |_severity, _datetime, _progname, msg|
41      "#{msg}\n"
42    end
43    logger

M # create

66  def create
67    logger.level = Logger::INFO
68    logger.formatter = proc do |_severity, _datetime, _progname, msg|
69      "#{msg}\n"
70    end
71  end

C # log

81
82    logger.level = Logger::INFO
83    logger.formatter = proc do |_severity, _datetime, _progname, msg|
84      "#{msg}\n"
85    end
86

Why

Let's assume

def my_method(width, height, show_border, show_header)
  # ...
end

when you call it

my_method(400, 50, false, true)

What are the meanings of numbers 400 and 50, the false and true?

How to fix

Parameters as hash options

method signature def my_method(options) height = options[:height] #... end

method call my_method(width: 400, height: 50, show_border: false, show_header: true) my_method(show_header: true, show_border: false, width: 400, height: 50)

You don't need to use the same order in invocation and the key documents the attributes. The invocation is more readable, the implementation should now deal with:

the presence/validation of the parameters,
misspelled keys, and
wrong types.

my_method(show_header: 400, show_border: 50, withs: 400, haights: 50)

Introduce a real model

Group related attributes:

class TableSettings
  attr_accessor :show_header, :show_border
end

Then use them

dimension = Dimension.new(width: 400, height: 50)
table_setting = TableSettings.new(show_header: true, show_border: false)
my_method(dimension, table_settings)

Ruby 2.0 keyword arguments

Ruby 2.0 introduces keyword arguments:

def my_method(width: 400, height: 50, show_border: false, show_header: true)
  # ...
end

More info

Know a good reference on this subject ? Tell us about it!

Space missing to the left of {


                                TestSource
                    #
                    setup 
       and 10 other locations

Occured at

12        @dir = "tmp_src_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

13        @gitTemplateDir = "tmp_src_git_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

19    @dir = "tmp_l10n_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

20    @svnTemplateDir = "tmp_l10n_repo_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

20        @dir = "tmp_l10n_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

21    @svnCheckoutDir = "tmp_l10n_check_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

21        @svnTemplateDir = "tmp_l10n_repo_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

22        @svnCheckoutDir = "tmp_l10n_check_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

29    @svn_checkout_dir = "#{Dir.pwd}/tmp_check_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

30    @svn_repo_dir = "#{Dir.pwd}/tmp_repo_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

103    tmpDir = Dir.pwd + "/tmp_svn_export_" + (0...16).map{ ('a'..'z').to_a[rand(26)] }.join

Why

Style matters. It improves readability and therefore maintainability. Remember that you code first for another human (it could be you 6 months from now), not for a computer. Space out your code. There is no shortage of disk space.

# bad
sum=1+2
a,b=1,2
1>2 ? true : false; puts 'Hi'
[1,2,3].each{|e| puts e}
 def some_method_with_arguments(arg1,arg2,arg3)
   # ...
 end

How to fix

Space out your code, there is no shortage of disk space.

How about this? Doesn't this look better?

sum = 1 + 2
a, b = 1, 2
1 > 2 ? true : false
puts 'Hi'
[1, 2, 3].each { |e| puts e }

def some_method_with_arguments(arg1, arg2, arg3)
  # ...
end

More info

Know a good reference on this subject ? Tell us about it!

Surrounding space missing for operator '=>'.


                                TestL10n
                    #
                    test_statistics 
       and 9 other locations

Occured at

test/test_l10n.rb:99

92        assert_equal(statistics.stats, {"de"=>{:all=>4,

92        assert_equal(statistics.stats, {"de"=>{:all=>4,

93                                           :shown=>3,

94                                           :notshown=>1,

95                                           :percentage=>75.0},

96                                        "fr"=>{:all=>4,

96                                        "fr"=>{:all=>4,

97                                           :shown=>4,

98                                           :notshown=>0,

test/test_l10n.rb:99