Putting extensive business logic in Rails controllers is bad and you should stop doing it.

I suppose I should demonstrate why that's true with an example.

The following is academic, trite, and completely based on a real life situation. The names and places have been changed out of a misguided sense of loyalty.

DHH Middle School in Southern Illinois decided to let its star programmer, Doug, develop a new web-based system for registering new students. The first feature he developed was adding those students through a simple form. He followed a Rails example and added this to StudentsController:

def create
		@student = Student.new(params[:student])

		if @student.save
				redirect_to(@student, :notice => 'Student was successfully created.')
		else
				render :action => "new"
		end
end

Just your standard Rails code and Doug even wrote tests. The next feature on Doug's TODO list is assigning classes. For the "Assign Classes" page Doug again wrote idiomatic Rails code, this time time in the ClassesController:

def create
		@student = Student.find(params[:student_id])
		params[:class_ids].each do |class_id|
				klass = Classes.find(class_id)
				@student.classes << klass
		end
end

Again this is perfectly straightforward Rails code. He adds a nested route for the classes controller, and then adds before_each hooks to make sure an administrator is logged in, and even makes the next button an Ajax submission with a cool sliding effect. The school is suitably impressed and begins using the system right away. A week into school they realize they have a problem. Students that register late do so at the district offices, where they are given a sheet of paper with their classes, then they go directly to the classes they are registered for. Since the district doesn't use the same system as the school, they can't just enter it in there, they need a new feature:

When a new student shows up to class the teacher goes to their iPad, this is a very expensive school, where they are already logged in. When a teacher enters a new student it should automatically assign the student to the teacher's class. If the student is already registered, because this is a Junior High so the students have more than one teacher, then it just assigns them to that class.

Doug is exceptionally bright, but a little naive, and while he knows better than to copy and paste the create code from ClassesController into the StudentsController he also has only seen business rules expressed in Rails controllers. His first attempt is this:

def create
		if Student.create(params[:student])
				if logged_in_user?(:teacher)
						controller = ClassesController.new
						controller.create
				end
				render :index
		else
				flash[:error] = "Couldn't Save Student"
				render :create
		end
end

Rails programmers are immediately yelling "YOU CAN'T USE CONTROLLERS THAT WAY!" and they are right, you can't. The params hash won't have the id for the student, I don't think that constructor works, etc. ActionControllers are a Rails object and are bound to it, but your business rules aren't, and putting that logic here is a violation of the Single Responsibility Principle. It's not obvious at first because so much of the functionality of controllers is tied up in base classes, but the controllers (and routers, and rest of actionpack's) responsibility is taking requests and sending them where they need to go. It's not business logic, so we can't really reuse them in this manner

The Rails app won't start, so Doug tries a different approach. Here he goes haywire. He changes his JavaScript code to look at the returned response, and redirect if necessary:

function PostNewStudent() {
		$.post("students/create", {
				onSuccess: function(response) {
						if (response.isLoggedInAsTeacher) {
								redirect_to("classes/create?name=class_name");
						}
				}
		});
}

Of course this requires the normal student control to return JSON or HTML depending on the request:

def create
		@student = Student.new(params[:student])

		respond_to do |format|
				format.html do
						if @student.save
								redirect_to(@student, :notice => 'Student was successfully created.')
						else
								render :action => "new"
						end
				end
				format.js do
						if @student.save
								render :json => {"isLoggedInAsTeacher" : logged_in_as_teacher }
						else
								render :nothing => true
						end
				end
		end
end

So now the business rule that a new student should be automatically assigned to the class they are in shared amongst two controllers and a view, and the code requires two languages to debug. This is how a mess gets made. What Doug should have done was extract the student creation and class assignment into PORO. At that point they could have made a new controller with a new action, and simply delegated to a new object that cleanly expressed that business rule. Something like this:

class StudentCreator

		def self.create!(params)
				student = Student.new(params)
				student.save!
				return student
		end
end

class ClassAssigner

		def self.assign!(student_id, class_name)
				student = Student.find(student_id)
				student.classes << Classes.find_by_name(class_name)
		end
end

class MidYearStudent
		def self.student_entered_class(student_params, class_name)
				student = StudentCreator.create(student_params)
				ClassAssigner.assign(student.id, class_name)
		end
end

class MidYearStudentController

		def create
				MidYearStudent.student_entered_class(params[:student], logged_in_user.class_name)
		end
end

There's several ways I could write these classes. I could drop the class members in favor of creating new objects and requesting their error status and ActiveRecord models. I could choose class names that better fit a metaphor of the system but I don't really have one here. I could move assign into student and only use the ClassAssigner service in the new controller action and stick to ActiveRecord in the original controllers, etc. All these options are available because I freed myself from the Rails metaphor and entered the business logic domain. By staying within the Rails controllers Doug bound his business logic to the Rails framework, which is very good at making web sites but doesn't know anything about managing a school system.

Cohesion

This reads a little like YARR* and to a certain extent it is, but it's aimed at the community of Rails app builders not Rails itself. Rails programmers talk a lot about coupling. They try to decouple their views from their models and controllers, and while they don't always succeed it's clearly on their minds. What they don't talk about is cohesion which is obvious when one looks at Rails controllers. Their typical design includes tons of concerns from the database to user authentication and data caching. Some devs "fix" this by moving logic into Rails models, but that just changes the problem. Now your models aren't cohesive! Look at poor Doug's controllers and views, responsible for all sorts of concerns. Now look at your controllers and views. How many concerns do they have? Are your business rules easy to identify? Are your tests fast? Keep your Rails in Rails, your ActiveRecord in ActiveRecord, and your business logic in yet more objects. Your app will be more flexible, tests will be faster, and the code will be easier to follow.

* Yet Another Rails Rant

** Doug's name used without permission, and any resemblance to a real Doug is amazingly coincidental