Rubocop: return early instead of wrapping in guard clause

Based on https://github.com/bbatsov/ruby-style-guide#no-nested-conditionals This handles a few of the cases.

Rubocop: return early instead of wrapping in guard clause
Based on https://github.com/bbatsov/ruby-style-guide#no-nested-conditionals This handles a few of the cases.
Laust Rud Jacobsen
1 parent f89b6066
Showing 6 changed files with 65 additions and 57 deletions Show diff stats
.rubocop.yml
.rubocop_todo.yml
app/controllers/application_controller.rb
app/controllers/apps_controller.rb
app/models/app.rb
lib/overrides/hoptoad_notifier/hoptoad_notifier.rb
@@ -23,6 +23,9 @@ Style/IndentationConsistency:
   # modifiers.
   EnforcedStyle: rails
+Style/GuardClause:
+  MinBodyLength: 10
+
 Style/AccessModifierIndentation:
   EnforcedStyle: outdent
@@ -75,11 +75,6 @@ Style/EachWithObject:
     - 'app/models/notice_fingerprinter.rb'
     - 'lib/recurse.rb'
-# Offense count: 27
-# Configuration parameters: MinBodyLength.
-Style/GuardClause:
-  Enabled: false
-
 # Offense count: 873
 # Cop supports --auto-correct.
 # Configuration parameters: EnforcedStyle, SupportedStyles.
@@ -23,10 +23,10 @@ protected
   # Check if the current_user is admin or not and redirect to root url if not
   #
   def require_admin!
-    unless user_signed_in? && current_user.admin?
-      flash[:error] = "Sorry, you don't have permission to do that"
-      redirect_to_root
-    end
+    return if user_signed_in? && current_user.admin?
+
+    flash[:error] = "Sorry, you don't have permission to do that"
+    redirect_to_root
   end
   def redirect_to_root
@@ -93,13 +93,16 @@ class AppsController &lt; ApplicationController
 protected
   def initialize_subclassed_notification_service
+    notification_type = params[:app].
+      fetch(:notification_service_attributes, {}).
+      fetch(:type, nil)
+    return if notification_type.blank?
+
     # set the app's notification service
-    if params[:app][:notification_service_attributes] && (notification_type = params[:app][:notification_service_attributes][:type])
-      available_notification_classes = [NotificationService] + NotificationService.subclasses
-      notification_class = available_notification_classes.detect { |c| c.name == notification_type }
-      if notification_class.present?
-        app.notification_service = notification_class.new(params[:app][:notification_service_attributes])
-      end
+    available_notification_classes = [NotificationService] + NotificationService.subclasses
+    notification_class = available_notification_classes.detect { |c| c.name == notification_type }
+    if notification_class.present?
+      app.notification_service = notification_class.new(params[:app][:notification_service_attributes])
     end
   end
@@ -112,32 +115,38 @@ protected
   # email_at_notices is edited as a string, and stored as an array.
   def parse_email_at_notices_or_set_default
-    if params[:app] && (val = params[:app][:email_at_notices])
-      # Sanitize negative values, split on comma,
-      # strip, parse as integer, remove all '0's.
-      # If empty, set as default and show an error message.
-      email_at_notices = val.gsub(/-\d+/, "").split(",").map { |v| v.strip.to_i }.reject { |v| v == 0 }
-      if email_at_notices.any?
-        params[:app][:email_at_notices] = email_at_notices
-      else
-        default_array = params[:app][:email_at_notices] = Errbit::Config.email_at_notices
-        flash[:error] = "Couldn't parse your notification frequency. Value was reset to default (#{default_array.join(', ')})."
-      end
+    return if params[:app].blank?
+
+    val = params[:app][:email_at_notices]
+    return if val.blank?
+
+    # Sanitize negative values, split on comma,
+    # strip, parse as integer, remove all '0's.
+    # If empty, set as default and show an error message.
+    email_at_notices = val.gsub(/-\d+/, "").split(",").map { |v| v.strip.to_i }.reject { |v| v == 0 }
+    if email_at_notices.any?
+      params[:app][:email_at_notices] = email_at_notices
+    else
+      default_array = params[:app][:email_at_notices] = Errbit::Config.email_at_notices
+      flash[:error] = "Couldn't parse your notification frequency. Value was reset to default (#{default_array.join(', ')})."
     end
   end
   def parse_notice_at_notices_or_set_default
-    if params[:app][:notification_service_attributes] && (val = params[:app][:notification_service_attributes][:notify_at_notices])
-      # Sanitize negative values, split on comma,
-      # strip, parse as integer, remove all '0's.
-      # If empty, set as default and show an error message.
-      notify_at_notices = val.gsub(/-\d+/, "").split(",").map { |v| v.strip.to_i }
-      if notify_at_notices.any?
-        params[:app][:notification_service_attributes][:notify_at_notices] = notify_at_notices
-      else
-        default_array = params[:app][:notification_service_attributes][:notify_at_notices] = Errbit::Config.notify_at_notices
-        flash[:error] = "Couldn't parse your notification frequency. Value was reset to default (#{default_array.join(', ')})."
-      end
+    return if params[:app][:notification_service_attributes].blank?
+
+    val = params[:app][:notification_service_attributes][:notify_at_notices]
+    return if val.blank?
+
+    # Sanitize negative values, split on comma,
+    # strip, parse as integer, remove all '0's.
+    # If empty, set as default and show an error message.
+    notify_at_notices = val.gsub(/-\d+/, "").split(",").map { |v| v.strip.to_i }
+    if notify_at_notices.any?
+      params[:app][:notification_service_attributes][:notify_at_notices] = notify_at_notices
+    else
+      default_array = params[:app][:notification_service_attributes][:notify_at_notices] = Errbit::Config.notify_at_notices
+      flash[:error] = "Couldn't parse your notification frequency. Value was reset to default (#{default_array.join(', ')})."
     end
   end
@@ -152,16 +152,17 @@ class App
   # Copy app attributes from another app.
   def copy_attributes_from(app_id)
-    if (copy_app = App.where(_id: app_id).first)
-      # Copy fields
-      (copy_app.fields.keys - %w(_id name created_at updated_at)).each do |k|
-        send("#{k}=", copy_app.send(k))
-      end
-      # Clone the embedded objects that can be changed via apps/edit (ignore errs & deploys, etc.)
-      %w(watchers issue_tracker notification_service).each do |relation|
-        if (obj = copy_app.send(relation))
-          send("#{relation}=", obj.is_a?(Array) ? obj.map(&:clone) : obj.clone)
-        end
+    copy_app = App.where(_id: app_id).first
+    return if copy_app.blank?
+
+    # Copy fields
+    (copy_app.fields.keys - %w(_id name created_at updated_at)).each do |k|
+      send("#{k}=", copy_app.send(k))
+    end
+    # Clone the embedded objects that can be changed via apps/edit (ignore errs & deploys, etc.)
+    %w(watchers issue_tracker notification_service).each do |relation|
+      if (obj = copy_app.send(relation))
+        send("#{relation}=", obj.is_a?(Array) ? obj.map(&:clone) : obj.clone)
       end
     end
   end
@@ -6,18 +6,18 @@ HoptoadNotifier.module_eval do
   class << self
     private def send_notice(notice)
       # Log the error internally if we are not in a development environment.
-      if configuration.public?
-        app = App.find_or_initialize_by(name: "Self.Errbit")
-        app.github_repo = "errbit/errbit"
-        app.save!
-        notice.send("api_key=", app.api_key)
+      return unless configuration.public?
-        # Create notice internally.
-        report = ErrorReport.new(notice.to_xml)
-        report.generate_notice!
+      app = App.find_or_initialize_by(name: "Self.Errbit")
+      app.github_repo = "errbit/errbit"
+      app.save!
+      notice.send("api_key=", app.api_key)
-        logger.info "Internal error was logged to 'Self.Errbit' app."
-      end
+      # Create notice internally.
+      report = ErrorReport.new(notice.to_xml)
+      report.generate_notice!
+
+      logger.info "Internal error was logged to 'Self.Errbit' app."
     end
   end
 end