Be part of JetBrains PHPverse 2026 on June 9 – a free online event bringing PHP devs worldwide together.

Shivamyadav's avatar

Is it good having approximately 900 lines of a function?

My senior uses the repository patterns to work with the controller business logics. In the function he has the create and update method to create/ update the resource and other side effects to perform actions whenever resource gets created or updated and both were approximately 900 of lines.

That was really too hard to understand it and overwhelming for me to just looking at the no of the lines in the function.

How long no of lines any function could have?

0 likes
16 replies
Glukinho's avatar

It is totally a matter of taste. However 900 lines seems far too many.

Some say a class/method/function should fit a screen without scrolling.

1 like
ranto's avatar

I second this. It really a matter of taste.
Sometimes you cannot avoid writting a large function and then never have the time to really write a readable function.

I prefer small functions and this is what I do when I encounter a massive function inside the codebase:

  1. I make sure all the task I had to do are done. I never refactor old code when I have business things to ship.
  2. Identify code blocks on the larger function that do a single task. Eg: check for some condition, do a business logic, etc
  3. Move those to a separate function. This could be a private function on the Repository or an action if you like the action pattern, etc. This might heavely deppend on your preferences or codebase standars.
  4. Make sure I don't break any test.

Just don't overcomplicate things. Write code for the person who is going to read it 3 months lated.

kevinbui's avatar

900 lines are way too much for a function and that should be refactored to tiny ones.

According to the most comprehensive resource on clean Code, the book Clean Code by Uncle Bob Martin. Each function should only contain 3-4 lines, at the same level of abstraction.

"The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that."

Robert C. Martin

I must admit 3-4 lines per function is a bit hard to achieve. We can aim for at most 10 lines per function for the most part.

Small functions are crucial to clean code and readability. Looking at the Laravel source code, thousands of people can grasp and contribute. Monster functions and chunks of code are only maintainable to whoever wrote them in the first place.

The unfortunate reality is, the majority of developers don't care much about clean code or small functions. Still, I always strive for them.

Shivamyadav's avatar

You can have a look at this a shorter function somewhere 400 of lines.

Tray2's avatar

Looks like that senior used to be a Cobol programmer.

I guess there are no tests written for it either, but if there are, I would ctrl-a delete, and start from scratch.

If not, the I would start by extraction each segment of the if statements to it's own method.

For example

    if ($data['user']['send_email']) {
                $skipResetPassword = array_key_exists('skip_reset_password', $data['user']);
                $withDelay = (array_key_exists('invite_email_with_delay', $data['user']) ? $data['user']['invite_email_with_delay'] : true);
                
                $this->sendInviteEmail($modelResource, $setInviteFlag, $skipResetPassword, $withDelay);
            }

would become something like this

function sendInviteEmailIfNeeded($modelResource, $setInviteFlag, $data)
{
  if (! $data['user']['send_email']) return;

  $skipResetPassword = array_key_exists('skip_reset_password', $data['user']);
  $withDelay = (array_key_exists('invite_email_with_delay', $data['user']) ? $data['user']['invite_email_with_delay'] : true);
  $this->sendInviteEmail($modelResource, $setInviteFlag, $skipResetPassword, $withDelay);
}
Shivamyadav's avatar

Yeah, there's no any test written for a single piece of code..

Shivamyadav's avatar

Function to create the user

Glukinho's avatar

You may more or less understand what's going on inside the function right now. But after a month or half a year you'll be totally lost, for sure.

JussiMannisto's avatar

Anyone trying to follow that method is going to have a really hard time. And that someone might be you two years from now.

Right now you're doing everything from control flow to low-level operations within the create() method. I'd refactor it so it only handles the top-level logic, and all heavy lifting is delegated to local methods. This makes the code more readable and also naturally comments it. Example:

if (array_key_exists('profile_image', $data['user'])) {
	$storageUsed = $this->uploadImage($data);
}

If you need to modify the $data array in a subtask, you can pass it as a reference:

protected function doSomething(array &$data): void {
	$data['user']['something'] = true;
}
ian_h's avatar

Jesus christ!

If that's your senior engineer writing that code today, they need to be sacked, ASAP!

That's not code written by a senior! They might have been there a long time, but they are not a senior engineer.

1 like

Please or to participate in this conversation.