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
19 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
DigitalArtisan's avatar

In One Sentence

This function is a monolithic user provisioning workflow that creates a user and orchestrates:

  • authentication setup
  • billing
  • subscriptions
  • permissions
  • divisions
  • expert profiles
  • Stripe integration
  • storage tracking
  • notifications
  • marketplace workflows
  • organization-specific business rules

all in one place.

Architectural Observation

This method is doing too many responsibilities and would benefit from decomposition into services like:

  • UserCreator
  • ProfileImageService
  • ExpertProvisioner
  • MarketplaceWorkflowService
  • SubscriptionManager
  • DivisionAssignmentService
  • InviteService

Right now it’s effectively:

“Create user + entire business lifecycle orchestration.”

DigitalArtisan's avatar

With a long function you get a long answer:

This method is doing far too much in one transaction boundary:

  • organization context resolution
  • feature flag evaluation
  • user normalization
  • image upload/storage accounting
  • subscription/plan upgrades
  • expert provisioning
  • division orchestration
  • Stripe integration
  • notifications/events
  • permissions
  • storage ledger
  • account subscription logic
  • invite workflow
  • client-specific hacks

The biggest improvement is to convert this into an orchestration/service pipeline where create() becomes readable and declarative.

Refactored create() (main function only)

Recommended Architecture

1. UserCreationContextFactory

Purpose: Centralize all environment/session/org/feature-flag logic.

Responsibilities

  • org resolution
  • marketplace flags
  • white label flags
  • exception clients
  • usage plan lookup
  • setting lookups

Example

class UserCreationContextFactory
{
    public function build(array $data): UserCreationContext
    {
    }
}

DTO

class UserCreationContext
{
    public int $organizationId;

    public bool $isMarketplace;

    public bool $isDistributedMarketplace;

    public bool $isWhiteLabel;

    public bool $isExceptionClient;

    public ?int $organizationUsagePlanId;

    public array $settings;
}

2. UserPayloadBuilder

Purpose: Normalize and enrich raw request payload.

Responsibilities

  • default activation state
  • employee ID password logic
  • notification defaults
  • metadata injection
  • role normalization

Stub

class UserPayloadBuilder
{
    public function build(
        array $data,
        UserCreationContext $context
    ): array {
    }
}

3. ProfileImageService

Purpose: Own all image concerns.

Responsibilities

  • upload image
  • assign default avatars
  • storage calculation
  • dispatch optimization jobs

Stub

class ProfileImageService
{
    public function process(
        array &$payload,
        UserCreationContext $context
    ): ProfileImageResult {
    }

    public function dispatchOptimization(
        User $user,
        ProfileImageResult $result
    ): void {
    }
}

Result DTO

class ProfileImageResult
{
    public bool $processed = false;

    public float $storageUsed = 0;
}

4. UserPlanService

Purpose: Isolate subscription complexity.

Responsibilities

  • learner capacity checks
  • GTM subscription handling
  • plan upgrades

Stub

class UserPlanService
{
    public function prepareForCreation(
        UserCreationContext $context
    ): UserPlanResult {
    }

    public function finalizeUpgrade(
        UserCreationContext $context,
        UserPlanResult $result
    ): void {
    }
}

5. CorporateUserService

Purpose: Handle corporate-specific onboarding.

Responsibilities

  • manager role assignment
  • division creation
  • corporate subscription events

Stub

class CorporateUserService
{
    public function handle(
        User $user,
        array $payload,
        UserCreationContext $context
    ): void {
    }
}

6. ExpertProvisioningService

Purpose: Encapsulate expert creation logic.

Responsibilities

  • expert profile creation
  • commission lookup
  • marketplace approval flow
  • division generation
  • approval notifications
  • replication events

Stub

class ExpertProvisioningService
{
    public function handle(
        User $user,
        array $payload,
        UserCreationContext $context
    ): void {
    }
}

7. StripeCustomerService

Purpose: External billing integration.

Stub

class StripeCustomerService
{
    public function attachCustomer(User $user): void
    {
    }
}

8. DivisionAssignmentService

Purpose: All division mapping logic.

Stub

class DivisionAssignmentService
{
    public function assign(
        User $user,
        array $payload
    ): void {
    }
}

9. PermissionService

Purpose: Permission matrix orchestration.

Stub

class PermissionService
{
    public function assign(User $user): void
    {
    }
}

10. StorageLedgerService

Purpose: Accounting-only concern.

Stub

class StorageLedgerService
{
    public function recordProfileImageUsage(
        User $user,
        ProfileImageResult $result
    ): void {
    }
}

11. InviteWorkflowService

Purpose: Email invitation orchestration.

Responsibilities

  • delayed invites
  • reset-password rules
  • resend handling

Stub

class InviteWorkflowService
{
    public function send(
        User $user,
        array $payload,
        UserCreationContext $context
    ): void {
    }
}

12. UserExpirationService

Purpose: Remove hardcoded org hacks from main flow.

Stub

class UserExpirationService
{
    public function handle(
        User $user,
        UserCreationContext $context
    ): void {
    }
}

13. EntityAllocationService

Stub

class EntityAllocationService
{
    public function dispatch(User $user): void
    {
    }
}

14. ManagerLimitService

Stub

class ManagerLimitService
{
    public function assign(
        User $user,
        array $payload
    ): void {
    }
}

Additional Refactoring Recommendations

Replace array_key_exists() everywhere

Move to typed DTOs/request objects.

Instead of:

array_key_exists('send_email', $data['user'])

Prefer:

$request->shouldSendInvite()

Eliminate client/subdomain conditionals

This:

if (in_array(session('org.sub_domain'), ...))

should become strategy classes:

ClientBehaviorResolver::for($organization)

Examples:

  • JishaBehavior
  • ChronosBehavior
  • GoGetterzBehavior

This removes the giant conditional tree.

Introduce Domain Events

Several operations should happen asynchronously after commit.

Examples:

UserCreated
UserProvisioned
ExpertCreated
CorporateUserCreated
MarketplaceManagerRegistered

Then listeners handle:

  • Stripe
  • emails
  • image processing
  • notifications
  • allocations

Your transaction becomes dramatically smaller.

Biggest Structural Problem

The current method mixes:

  • domain logic
  • infrastructure
  • persistence
  • feature flags
  • integration APIs
  • client-specific business rules
  • async orchestration

into one procedural flow.

The ideal end state is:

CreateUserAction
    -> validate
    -> enrich
    -> persist
    -> dispatch domain events

Everything else becomes listeners/services.

Please or to participate in this conversation.