5
\$\begingroup\$

Basically I'm displaying an array of images on a page with checkboxes. The idea is when a user is editing a post and the checkbox is checked, the image will get deleted. Each image also has an order_id and description.

I'm pretty new to Ruby and programming and I'm looking for a way to simplify the following code.

Codeclimate is saying:

Cyclomatic complexity for update is too high

lessons_controller.rb:

def update

@lesson.attachments.each.with_index do |attachment, index|

  unless params[:delete_array].nil?
    if params[:delete_array][index].to_i == attachment.id && params[:delete_array][index].to_i != 0
      puts "DELETE: #{attachment.attachment_file_name}"
      attachment.destroy
    end
  end

  unless attachment.nil?
    unless params[:order_array].nil?
      if params[:order_array][index].to_i != attachment.order
        attachment.order = params[:order_array][index]
        puts "ORDER: #{attachment.order} = #{params[:order_array][index]}"
        @save = true
      end
    end

    unless params[:description_array].nil?
      if params[:description_array][index] != attachment.description
        attachment.description = params[:description_array][index]
        puts "DESCR: #{attachment.description} = #{params[:description_array][index]}"
        @save = true
      end
    end
  end
end

if @save == true
  @lesson.attachments.each(&:save)
end

  respond_to do |format|
  if @lesson.update(lesson_params)
    format.html { redirect_to @lesson, notice: 'Lesson was successfully updated.' }
    format.json { render :show, status: :ok, location: @lesson }
  else
    format.html { render :edit }
    format.json { render json: @lesson.errors, status: :unprocessable_entity }
  end
end
end

view/_form.html.erb

<div class="form-group">
  <div class="col-sm-10 col-md-offset-2">
    <% if @lesson.attachments.any? %>
    <button type="button" id="check_all" class="center"><%="Check all/Uncheck all"%></button><br />

      <% @lesson.attachments.order(:order).each do |attachment| %>
        <div class="col-lg-2 col-sm-3 col-xs-4 lesson-thumbnail center">
          <%= image_tag attachment.attachment.url(:thumb), class: "thumbnail img-responsive center" %>

          <%= check_box_tag 'delete_array[]', attachment.id, checked = false %> Delete

          <%= number_field_tag 'order_array[]', attachment.order %>
          <%= text_field_tag 'description_array[]', attachment.description, placeholder: "Description" %>

        </div>
      <% end %>
    <% else %>
      No attachments found.
    <% end %>
  </div>
</div>
\$\endgroup\$
3
\$\begingroup\$

I try to start from part dealing with removing of checked attachments.

First, I can't find purpose of puts lines. Only if it used for debugging in development.

Second, @lesson.attachments.each.with_index - every time updating single attachment, you need to go through all existing records. I think, that is what 'Codeclimate' "says". So, that structure must be simplified.

Here is how I suggest to rewrite 'deletion' part:

def update

  params[:delete_array].try :each do |id|
    @lesson.attachments.find(id).destroy
  end

# other code

end

Other parts (for params[:order_array] and params[:description_array]) can be refactored in the same way. I hope, you've got the idea.

If you need more detailed explanation of above code or need help with other parts - let me know and I'll explain it in details.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.