Improving ruby code

Question

people said to me that I have an wrong code at this point. The main idea of this method is to create 1 event if user pressed once, and a row of events if user pressed daily or weekly. Everything works well, but code is so bulky.

  def create
@event = Event.new(event_params
@event.start_time = DateTime.parse(params[:start_time], "%Y-%m-%d %H:%i")
@event.end_time = DateTime.parse(params[:end_time], "%Y-%m-%d %H:%i")
@event.user_id = current_user.id
@event.update_attributes(:repeat_id => @event.id) if @event.save
respond_to do |format|
  if @event.save
    format.html { redirect_to persons_profile_path }
  else
    format.html { render :new }
    format.json { render json: @event.errors, status: :unprocessable_entity }
  end
interval = 60 if @event.repeat =='daily'
interval = 20 if @event.repeat =='weekly'
#creating row of events
if @event.repeat != 'once'
  (1..interval).each do |i|
    @event = Event.new(event_params)
    @event.start_time = DateTime.parse(params[:start_time], "%Y-%m-%d %H:%i")
    @event.end_time = DateTime.parse(params[:end_time], "%Y-%m-%d %H:%i")
    @event.user_id = current_user.id
    if @event.repeat =='daily'
      @event.start_time = DateTime.parse(@event.start_time.to_s) + i.day
      @event.end_time = DateTime.parse(@event.end_time.to_s) + i.day
    end
    if @event.repeat =='weekly'
      @event.start_time = DateTime.parse(@event.start_time.to_s) + i.week
      @event.end_time = DateTime.parse(@event.end_time.to_s) + i.week
    end
    @event.update_attributes(:repeat_id => k) if @event.save
    @event.save
  end
end

I can't find another solutions. Any ideas? Dont pay attention on parse methods on date, its needed to JS.

I am not sure what you mean by a row of events, also it would appear that you are missing a ) on the second line. My main question is, does the code not work or is the issue just that it is written inefficiently? If it is that later this should be moved to code review, if its the former you need to add more details. — C dot StrifeVII
– C dot StrifeVII, Commented Oct 15, 2016 at 12:56
There is a ton of duplication in this code which is confusing its meaning. One of the first things to do is try and perform an operation like DateTime.parse on a particular piece of data once, then re-use that value. You can also collapse your if into a case to figure out what offset to use, then add that offset on to both values. — tadman
– tadman, Commented Oct 15, 2016 at 16:43

user3456978 · Accepted Answer · 2016-10-15 14:02:05Z

2

I think the issue is that you are using the controllers to do too much work and it's hard to read. Typically controllers look like:

def create
  @client = Client.new(params[:client])
  if @client.save
    redirect_to @client
  else
    render "new"
  end
end

Take a moment and think about your data. You create events. You want your events to appear in a row if the create button is pressed daily or weekly. So we are thinking about how to display it, that's a css/html issue, not something relating to controllers.

The reason people are saying your code as wrong is because you are doing something very atypical. If you submitted this code for school or a project you would get a low score because you are making everyone else work hard to understand your work, instead of following the same basic style that everyone was trained in.

answered Oct 15, 2016 at 14:02

user3456978

2561 silver badge13 bronze badges

Sign up to request clarification or add additional context in comments.

Collectives™ on Stack Overflow

Improving ruby code

1 Answer 1

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Related