sanger/sequencescape

View on GitHub
app/models/submission/submission_creator.rb

Summary

Maintainability
A
3 hrs
Test Coverage
B
84%

Class SubmissionCreator has 28 methods (exceeds 20 allowed). Consider refactoring.
Open

class Submission::SubmissionCreator < Submission::PresenterSkeleton # rubocop:todo Metrics/ClassLength
  SubmissionsCreaterError = Class.new(StandardError)
  IncorrectParamsException = Class.new(SubmissionsCreaterError)
  InvalidInputException = Class.new(SubmissionsCreaterError)

Severity: Minor
Found in app/models/submission/submission_creator.rb - About 3 hrs to fix

    Complex method Submission::SubmissionCreator#save (50.3)
    Open

      def save # rubocop:todo Metrics/AbcSize
        begin
          ActiveRecord::Base.transaction do
            # Add assets to the order...
            new_order = create_order.tap { |o| o.update!(order_assets) }

    Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

    You can read more about ABC metrics or the flog tool

    Complex method Submission::SubmissionCreator#order_assets (27.7)
    Open

      def order_assets # rubocop:todo Metrics/AbcSize
        input_methods =
          %i[asset_group_id sample_names_text barcodes_wells_text].select { |input_method| send(input_method).present? }
    
        raise InvalidInputException, 'No Samples found' if input_methods.empty?

    Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

    You can read more about ABC metrics or the flog tool

    Submission::SubmissionCreator#order_assets has approx 8 statements
    Open

      def order_assets # rubocop:todo Metrics/AbcSize

    A method with Too Many Statements is any method that has a large number of lines.

    Too Many Statements warns about any method that has more than 5 statements. Reek's smell detector for Too Many Statements counts +1 for every simple statement in a method and +1 for every statement within a control structure (if, else, case, when, for, while, until, begin, rescue) but it doesn't count the control structure itself.

    So the following method would score +6 in Reek's statement-counting algorithm:

    def parse(arg, argv, &error)
      if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0]))
        return nil, block, nil                                         # +1
      end
      opt = (val = parse_arg(val, &error))[1]                          # +2
      val = conv_arg(*val)                                             # +3
      if opt and !arg
        argv.shift                                                     # +4
      else
        val[0] = nil                                                   # +5
      end
      val                                                              # +6
    end

    (You might argue that the two assigments within the first @if@ should count as statements, and that perhaps the nested assignment should count as +2.)

    Submission::SubmissionCreator#wells_on_specified_plate_purpose_for refers to 'sample' more than self (maybe move it to another class?)
    Open

          sample.wells.on_plate_purpose(plate_purpose).order(id: :desc).first ||
            raise(InvalidInputException, "No #{plate_purpose.name} plate found with sample: #{sample.name}")

    Feature Envy occurs when a code fragment references another object more often than it references itself, or when several clients do the same series of manipulations on a particular type of object.

    Feature Envy reduces the code's ability to communicate intent: code that "belongs" on one class but which is located in another can be hard to find, and may upset the "System of Names" in the host class.

    Feature Envy also affects the design's flexibility: A code fragment that is in the wrong class creates couplings that may not be natural within the application's domain, and creates a loss of cohesion in the unwilling host class.

    Feature Envy often arises because it must manipulate other objects (usually its arguments) to get them into a useful form, and one force preventing them (the arguments) doing this themselves is that the common knowledge lives outside the arguments, or the arguments are of too basic a type to justify extending that type. Therefore there must be something which 'knows' about the contents or purposes of the arguments. That thing would have to be more than just a basic type, because the basic types are either containers which don't know about their contents, or they are single objects which can't capture their relationship with their fellows of the same type. So, this thing with the extra knowledge should be reified into a class, and the utility method will most likely belong there.

    Example

    Running Reek on:

    class Warehouse
      def sale_price(item)
        (item.price - item.rebate) * @vat
      end
    end

    would report:

    Warehouse#total_price refers to item more than self (FeatureEnvy)

    since this:

    (item.price - item.rebate)

    belongs to the Item class, not the Warehouse.

    Submission::SubmissionCreator#find_samples_from_text has approx 8 statements
    Open

      def find_samples_from_text(sample_text)

    A method with Too Many Statements is any method that has a large number of lines.

    Too Many Statements warns about any method that has more than 5 statements. Reek's smell detector for Too Many Statements counts +1 for every simple statement in a method and +1 for every statement within a control structure (if, else, case, when, for, while, until, begin, rescue) but it doesn't count the control structure itself.

    So the following method would score +6 in Reek's statement-counting algorithm:

    def parse(arg, argv, &error)
      if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0]))
        return nil, block, nil                                         # +1
      end
      opt = (val = parse_arg(val, &error))[1]                          # +2
      val = conv_arg(*val)                                             # +3
      if opt and !arg
        argv.shift                                                     # +4
      else
        val[0] = nil                                                   # +5
      end
      val                                                              # +6
    end

    (You might argue that the two assigments within the first @if@ should count as statements, and that perhaps the nested assignment should count as +2.)

    Submission::SubmissionCreator#find_samples_from_text refers to 's' more than self (maybe move it to another class?)
    Open

        found_set = Set.new(samples.map { |s| [s.name, s.sanger_sample_id] }.flatten)

    Feature Envy occurs when a code fragment references another object more often than it references itself, or when several clients do the same series of manipulations on a particular type of object.

    Feature Envy reduces the code's ability to communicate intent: code that "belongs" on one class but which is located in another can be hard to find, and may upset the "System of Names" in the host class.

    Feature Envy also affects the design's flexibility: A code fragment that is in the wrong class creates couplings that may not be natural within the application's domain, and creates a loss of cohesion in the unwilling host class.

    Feature Envy often arises because it must manipulate other objects (usually its arguments) to get them into a useful form, and one force preventing them (the arguments) doing this themselves is that the common knowledge lives outside the arguments, or the arguments are of too basic a type to justify extending that type. Therefore there must be something which 'knows' about the contents or purposes of the arguments. That thing would have to be more than just a basic type, because the basic types are either containers which don't know about their contents, or they are single objects which can't capture their relationship with their fellows of the same type. So, this thing with the extra knowledge should be reified into a class, and the utility method will most likely belong there.

    Example

    Running Reek on:

    class Warehouse
      def sale_price(item)
        (item.price - item.rebate) * @vat
      end
    end

    would report:

    Warehouse#total_price refers to item more than self (FeatureEnvy)

    since this:

    (item.price - item.rebate)

    belongs to the Item class, not the Warehouse.

    Submission::SubmissionCreator has at least 28 methods
    Open

    class Submission::SubmissionCreator < Submission::PresenterSkeleton # rubocop:todo Metrics/ClassLength

    Too Many Methods is a special case of LargeClass.

    Example

    Given this configuration

    TooManyMethods:
      max_methods: 3

    and this code:

    class TooManyMethods
      def one; end
      def two; end
      def three; end
      def four; end
    end

    Reek would emit the following warning:

    test.rb -- 1 warning:
      [1]:TooManyMethods has at least 4 methods (TooManyMethods)

    Submission::SubmissionCreator#find_samples_from_text refers to 'not_found' more than self (maybe move it to another class?)
    Open

        raise InvalidInputException, "#{Sample.table_name} #{not_found.to_a.join(', ')} not found" unless not_found.empty?

    Feature Envy occurs when a code fragment references another object more often than it references itself, or when several clients do the same series of manipulations on a particular type of object.

    Feature Envy reduces the code's ability to communicate intent: code that "belongs" on one class but which is located in another can be hard to find, and may upset the "System of Names" in the host class.

    Feature Envy also affects the design's flexibility: A code fragment that is in the wrong class creates couplings that may not be natural within the application's domain, and creates a loss of cohesion in the unwilling host class.

    Feature Envy often arises because it must manipulate other objects (usually its arguments) to get them into a useful form, and one force preventing them (the arguments) doing this themselves is that the common knowledge lives outside the arguments, or the arguments are of too basic a type to justify extending that type. Therefore there must be something which 'knows' about the contents or purposes of the arguments. That thing would have to be more than just a basic type, because the basic types are either containers which don't know about their contents, or they are single objects which can't capture their relationship with their fellows of the same type. So, this thing with the extra knowledge should be reified into a class, and the utility method will most likely belong there.

    Example

    Running Reek on:

    class Warehouse
      def sale_price(item)
        (item.price - item.rebate) * @vat
      end
    end

    would report:

    Warehouse#total_price refers to item more than self (FeatureEnvy)

    since this:

    (item.price - item.rebate)

    belongs to the Item class, not the Warehouse.

    Submission::SubmissionCreator#build_submission! has approx 7 statements
    Open

      def build_submission!

    A method with Too Many Statements is any method that has a large number of lines.

    Too Many Statements warns about any method that has more than 5 statements. Reek's smell detector for Too Many Statements counts +1 for every simple statement in a method and +1 for every statement within a control structure (if, else, case, when, for, while, until, begin, rescue) but it doesn't count the control structure itself.

    So the following method would score +6 in Reek's statement-counting algorithm:

    def parse(arg, argv, &error)
      if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0]))
        return nil, block, nil                                         # +1
      end
      opt = (val = parse_arg(val, &error))[1]                          # +2
      val = conv_arg(*val)                                             # +3
      if opt and !arg
        argv.shift                                                     # +4
      else
        val[0] = nil                                                   # +5
      end
      val                                                              # +6
    end

    (You might argue that the two assigments within the first @if@ should count as statements, and that perhaps the nested assignment should count as +2.)

    Submission::SubmissionCreator#save has approx 15 statements
    Open

      def save # rubocop:todo Metrics/AbcSize

    A method with Too Many Statements is any method that has a large number of lines.

    Too Many Statements warns about any method that has more than 5 statements. Reek's smell detector for Too Many Statements counts +1 for every simple statement in a method and +1 for every statement within a control structure (if, else, case, when, for, while, until, begin, rescue) but it doesn't count the control structure itself.

    So the following method would score +6 in Reek's statement-counting algorithm:

    def parse(arg, argv, &error)
      if !(val = arg) and (argv.empty? or /\A-/ =~ (val = argv[0]))
        return nil, block, nil                                         # +1
      end
      opt = (val = parse_arg(val, &error))[1]                          # +2
      val = conv_arg(*val)                                             # +3
      if opt and !arg
        argv.shift                                                     # +4
      else
        val[0] = nil                                                   # +5
      end
      val                                                              # +6
    end

    (You might argue that the two assigments within the first @if@ should count as statements, and that perhaps the nested assignment should count as +2.)

    Complex method Submission::SubmissionCreator#build_submission! (23.1)
    Open

      def build_submission!
        submission.built!
      rescue AASM::InvalidTransition
        submission.errors.add(:base, 'Submissions can not be edited once they are submitted for building.')
      rescue ActiveRecord::RecordInvalid => e

    Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

    You can read more about ABC metrics or the flog tool

    Complex method Submission::SubmissionCreator#create_order (23.1)
    Open

      def create_order # rubocop:todo Metrics/AbcSize
        order_role = OrderRole.find_by(role: order_params.delete('order_role')) if order_params.present?
        new_order =
          template.new_order(
            study: study,

    Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

    You can read more about ABC metrics or the flog tool

    Submission::SubmissionCreator#build_submission! calls 'submission.errors' 3 times
    Open

        submission.errors.add(:base, 'Submissions can not be edited once they are submitted for building.')
      rescue ActiveRecord::RecordInvalid => e
        e.record.errors.full_messages.each { |message| submission.errors.add(:base, message) }
      rescue Submission::ProjectValidation::Error => e
        submission.errors.add(:base, e.message)

    Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

    Reek implements a check for Duplicate Method Call.

    Example

    Here's a very much simplified and contrived example. The following method will report a warning:

    def double_thing()
      @other.thing + @other.thing
    end

    One quick approach to silence Reek would be to refactor the code thus:

    def double_thing()
      thing = @other.thing
      thing + thing
    end

    A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

    class Other
      def double_thing()
        thing + thing
      end
    end

    The approach you take will depend on balancing other factors in your code.

    Submission::SubmissionCreator#order_assets calls 'input_methods.first' 2 times
    Open

        case input_methods.first
        when :asset_group_id
          { asset_group: find_asset_group }
        when :sample_names_text
          { assets: wells_on_specified_plate_purpose_for(plate_purpose, find_samples_from_text(sample_names_text)) }

    Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

    Reek implements a check for Duplicate Method Call.

    Example

    Here's a very much simplified and contrived example. The following method will report a warning:

    def double_thing()
      @other.thing + @other.thing
    end

    One quick approach to silence Reek would be to refactor the code thus:

    def double_thing()
      thing = @other.thing
      thing + thing
    end

    A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

    class Other
      def double_thing()
        thing + thing
      end
    end

    The approach you take will depend on balancing other factors in your code.

    Submission::SubmissionCreator::InvalidInputException has no descriptive comment
    Open

      InvalidInputException = Class.new(SubmissionsCreaterError)

    Classes and modules are the units of reuse and release. It is therefore considered good practice to annotate every class and module with a brief comment outlining its responsibilities.

    Example

    Given

    class Dummy
      # Do things...
    end

    Reek would emit the following warning:

    test.rb -- 1 warning:
      [1]:Dummy has no descriptive comment (IrresponsibleModule)

    Fixing this is simple - just an explaining comment:

    # The Dummy class is responsible for ...
    class Dummy
      # Do things...
    end

    Submission::SubmissionCreator#save calls 'e.message' 2 times
    Open

          order.errors.add(:base, e.message)
        rescue SubmissionsCreaterError, Asset::Finder::InvalidInputException => e
          order.errors.add(:base, e.message)

    Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

    Reek implements a check for Duplicate Method Call.

    Example

    Here's a very much simplified and contrived example. The following method will report a warning:

    def double_thing()
      @other.thing + @other.thing
    end

    One quick approach to silence Reek would be to refactor the code thus:

    def double_thing()
      thing = @other.thing
      thing + thing
    end

    A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

    class Other
      def double_thing()
        thing + thing
      end
    end

    The approach you take will depend on balancing other factors in your code.

    Submission::SubmissionCreator::SubmissionsCreaterError has no descriptive comment
    Open

      SubmissionsCreaterError = Class.new(StandardError)

    Classes and modules are the units of reuse and release. It is therefore considered good practice to annotate every class and module with a brief comment outlining its responsibilities.

    Example

    Given

    class Dummy
      # Do things...
    end

    Reek would emit the following warning:

    test.rb -- 1 warning:
      [1]:Dummy has no descriptive comment (IrresponsibleModule)

    Fixing this is simple - just an explaining comment:

    # The Dummy class is responsible for ...
    class Dummy
      # Do things...
    end

    Submission::SubmissionCreator#save calls 'order.errors.add(:base, e.message)' 2 times
    Open

          order.errors.add(:base, e.message)
        rescue SubmissionsCreaterError, Asset::Finder::InvalidInputException => e
          order.errors.add(:base, e.message)

    Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

    Reek implements a check for Duplicate Method Call.

    Example

    Here's a very much simplified and contrived example. The following method will report a warning:

    def double_thing()
      @other.thing + @other.thing
    end

    One quick approach to silence Reek would be to refactor the code thus:

    def double_thing()
      thing = @other.thing
      thing + thing
    end

    A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

    class Other
      def double_thing()
        thing + thing
      end
    end

    The approach you take will depend on balancing other factors in your code.

    Submission::SubmissionCreator::IncorrectParamsException has no descriptive comment
    Open

      IncorrectParamsException = Class.new(SubmissionsCreaterError)

    Classes and modules are the units of reuse and release. It is therefore considered good practice to annotate every class and module with a brief comment outlining its responsibilities.

    Example

    Given

    class Dummy
      # Do things...
    end

    Reek would emit the following warning:

    test.rb -- 1 warning:
      [1]:Dummy has no descriptive comment (IrresponsibleModule)

    Fixing this is simple - just an explaining comment:

    # The Dummy class is responsible for ...
    class Dummy
      # Do things...
    end

    Submission::SubmissionCreator#save calls 'order.errors' 4 times
    Open

          order.errors.add(:base, e.message)
        rescue SubmissionsCreaterError, Asset::Finder::InvalidInputException => e
          order.errors.add(:base, e.message)
        rescue ActiveRecord::RecordInvalid => e
          e.record.errors.full_messages.each { |message| order.errors.add(:base, message) }

    Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

    Reek implements a check for Duplicate Method Call.

    Example

    Here's a very much simplified and contrived example. The following method will report a warning:

    def double_thing()
      @other.thing + @other.thing
    end

    One quick approach to silence Reek would be to refactor the code thus:

    def double_thing()
      thing = @other.thing
      thing + thing
    end

    A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

    class Other
      def double_thing()
        thing + thing
      end
    end

    The approach you take will depend on balancing other factors in your code.

    Submission::SubmissionCreator#order_fields calls 'order.input_field_infos' 2 times
    Open

        order.request_type_ids_list = order.request_types.map { |rt| [rt] } if order.input_field_infos.flatten.empty?
        order.input_field_infos.reject { |info| per_order_settings.include?(info.key) }

    Duplication occurs when two fragments of code look nearly identical, or when two fragments of code have nearly identical effects at some conceptual level.

    Reek implements a check for Duplicate Method Call.

    Example

    Here's a very much simplified and contrived example. The following method will report a warning:

    def double_thing()
      @other.thing + @other.thing
    end

    One quick approach to silence Reek would be to refactor the code thus:

    def double_thing()
      thing = @other.thing
      thing + thing
    end

    A slightly different approach would be to replace all calls of double_thing by calls to @other.double_thing:

    class Other
      def double_thing()
        thing + thing
      end
    end

    The approach you take will depend on balancing other factors in your code.

    Submission::SubmissionCreator has missing safe method 'build_submission!'
    Open

      def build_submission!

    A candidate method for the Missing Safe Method smell are methods whose names end with an exclamation mark.

    An exclamation mark in method names means (the explanation below is taken from here ):

    The ! in method names that end with ! means, “This method is dangerous”—or, more precisely, this method is the “dangerous” version of an otherwise equivalent method, with the same name minus the !. “Danger” is relative; the ! doesn’t mean anything at all unless the method name it’s in corresponds to a similar but bang-less method name. So, for example, gsub! is the dangerous version of gsub. exit! is the dangerous version of exit. flatten! is the dangerous version of flatten. And so forth.

    Such a method is called Missing Safe Method if and only if her non-bang version does not exist and this method is reported as a smell.

    Example

    Given

    class C
      def foo; end
      def foo!; end
      def bar!; end
    end

    Reek would report bar! as Missing Safe Method smell but not foo!.

    Reek reports this smell only in a class context, not in a module context in order to allow perfectly legit code like this:

    class Parent
      def foo; end
    end
    
    module Dangerous
      def foo!; end
    end
    
    class Son < Parent
      include Dangerous
    end
    
    class Daughter < Parent
    end

    In this example, Reek would not report the Missing Safe Method smell for the method foo of the Dangerous module.

    Submission::SubmissionCreator#order_params performs a nil-check
    Open

          @order_params[:multiplier].nil?

    A NilCheck is a type check. Failures of NilCheck violate the "tell, don't ask" principle.

    Additionally, type checks often mask bigger problems in your source code like not using OOP and / or polymorphism when you should.

    Example

    Given

    class Klass
      def nil_checker(argument)
        if argument.nil?
          puts "argument isn't nil!"
        end
      end
    end

    Reek would emit the following warning:

    test.rb -- 1 warning:
      [3]:Klass#nil_checker performs a nil-check. (NilCheck)

    Submission::SubmissionCreator#find_assets_from_text doesn't depend on instance state (maybe move it to another class?)
    Open

      def find_assets_from_text(assets_text)

    A Utility Function is any instance method that has no dependency on the state of the instance.

    Submission::SubmissionCreator#product_lines doesn't depend on instance state (maybe move it to another class?)
    Open

      def product_lines

    A Utility Function is any instance method that has no dependency on the state of the instance.

    Complex method Submission::SubmissionCreator#find_samples_from_text (20.4)
    Open

      def find_samples_from_text(sample_text)
        names = sample_text.split(/\s+/)
        samples = Sample.includes(:assets).where(['name IN (:names) OR sanger_sample_id IN (:names)', { names: names }])
    
        name_set = Set.new(names)

    Flog calculates the ABC score for methods. The ABC score is based on assignments, branches (method calls), and conditions.

    You can read more about ABC metrics or the flog tool

    Submission::SubmissionCreator#build_submission! has the variable name 'e'
    Open

      rescue ActiveRecord::RecordInvalid => e
        e.record.errors.full_messages.each { |message| submission.errors.add(:base, message) }
      rescue Submission::ProjectValidation::Error => e

    An Uncommunicative Variable Name is a variable name that doesn't communicate its intent well enough.

    Poor names make it hard for the reader to build a mental picture of what's going on in the code. They can also be mis-interpreted; and they hurt the flow of reading, because the reader must slow down to interpret the names.

    Submission::SubmissionCreator#save has the variable name 'o'
    Open

            new_order = create_order.tap { |o| o.update!(order_assets) }

    An Uncommunicative Variable Name is a variable name that doesn't communicate its intent well enough.

    Poor names make it hard for the reader to build a mental picture of what's going on in the code. They can also be mis-interpreted; and they hurt the flow of reading, because the reader must slow down to interpret the names.

    Submission::SubmissionCreator#find_samples_from_text has the variable name 's'
    Open

        found_set = Set.new(samples.map { |s| [s.name, s.sanger_sample_id] }.flatten)

    An Uncommunicative Variable Name is a variable name that doesn't communicate its intent well enough.

    Poor names make it hard for the reader to build a mental picture of what's going on in the code. They can also be mis-interpreted; and they hurt the flow of reading, because the reader must slow down to interpret the names.

    Submission::SubmissionCreator#orders has the variable name 'o'
    Open

        submission.try(:orders).map { |o| OrderPresenter.new(o) }

    An Uncommunicative Variable Name is a variable name that doesn't communicate its intent well enough.

    Poor names make it hard for the reader to build a mental picture of what's going on in the code. They can also be mis-interpreted; and they hurt the flow of reading, because the reader must slow down to interpret the names.

    Submission::SubmissionCreator#save has the variable name 'e'
    Open

        rescue Submission::ProjectValidation::Error => e
          order.errors.add(:base, e.message)
        rescue SubmissionsCreaterError, Asset::Finder::InvalidInputException => e
          order.errors.add(:base, e.message)
        rescue ActiveRecord::RecordInvalid => e

    An Uncommunicative Variable Name is a variable name that doesn't communicate its intent well enough.

    Poor names make it hard for the reader to build a mental picture of what's going on in the code. They can also be mis-interpreted; and they hurt the flow of reading, because the reader must slow down to interpret the names.

    There are no issues that match your filters.

    Category
    Status