Make multiple conditions in if expression more readable

Question

My logic has to be written in this way, but I think the if expression is too long to read.

How can I make it more readable?

 if @tool_cvt_video_info_test.save and check_and_update_firmware_model_and_version(params["camera_ip"]) and save_uploaded(params[:videos])
 format.html { redirect_to @tool_cvt_video_info_test, notice: 'Video info test was successfully created.' }
 format.json { render action: 'show', status: :created, location: @tool_cvt_video_info_test }
 else
 format.html { render action: 'new' }
 format.json { render json: @tool_cvt_video_info_test.errors, status: :unprocessable_entity }
 end

Answer 1

Given the little context you have provided in the question, there isn't much to recommend, other than whitespace changes.

if @tool_cvt_video_info_test.save and
 check_and_update_firmware_model_and_version(params["camera_ip"]) and
 save_uploaded(params[:videos])
 format.html {
 redirect_to @tool_cvt_video_info_test,
 notice: 'Video info test was successfully created.'
 }
 format.json {
 render action: 'show',
 status: :created,
 location: @tool_cvt_video_info_test
 }
else
 format.html { render action: 'new' }
 format.json {
 render json: @tool_cvt_video_info_test.errors,
 status: :unprocessable_entity
 }
end

I am a bit suspicious of this code, though. It's saving some information about a video test, possibly updating the camera's firmware, and saving an uploaded video? That's a lot of functionality to put in one if-condition, and if anything goes wrong, the user probably won't get sufficient feedback as to exactly what failed.

• outoftime: I prefer place logical key words at the begining and check_and_update_firmware_model_and_version(params["camera_ip"]) and and save_uploaded(params[:videos]). IMHO: this way is more readable.
• Flambino: I'd agree that there certainly seems to be too much going on in the code. If the 3 save/update operations must all complete for anything to be valid, it should probably be handled as a transaction.

Answer 2

First off, I would also strongly suggest you avoid the and keyword and prefer && unless you have a very specific reason for favoring and over &&. Look here if you're interested in seeing why a lot of ruby programmers think this way.

That being said, the easiest way to clean up big conditionals like this without much refactoring is by assigning intermediary variables (optimize for readability):

persisted = @tool_cvt_video_info_test.save
firmware = check_and_update_firmware_model_and_version(params["camera_ip"])
saved_upload = save_uploaded(params[:videos])
if persisted && firmware && saved_upload
 format.html { redirect_to @tool_cvt_video_info_test, notice: 'Video info test was successfully created.' }
 format.json { render action: 'show', status: :created, location: @tool_cvt_video_info_test }
else
 format.html { render action: 'new' }
 format.json { render json: @tool_cvt_video_info_test.errors, status: :unprocessable_entity }
end

I think that makes for a bit more readable code, but I'd agree with other comments in this thread that this block of code seems to be doing too much. I'd suggest seeing if you can separate the logic used to validate and persist this data from the places you determine the controller response.

Cleaner separation of purpose will make it easier to avoid things like temporary variables and make it easier to spot places where complex logic can be sequestered into a private method (for instance, a method called save_or_update_cvt_video_info(params))

• Damien Wilson: Good catch! Corrected!