Episode 2: I Was a Junior Developer and I Must Be Stopped

Welcome back.

Last episode, we reviewed a Laravel function that used three loops to reconstruct an array it already had, fired two database queries per item instead of one, and named a variable $r_arr with the confidence of a man who has never once been held accountable for anything.

It was a good one, some of us liked it.

So today, we are reviewing my hotel invoice generator. It is a PHP class called ExcelController. It imports guest data from an uploaded Excel file and exports a personalised invoice for each guest, packed into a zip file for download.

It was never merged.

The room numbers are completely random.

My senior opened the PR. He read through it. He left seven comments. He never raised his voice. He never sent a follow-up Slack. He just reviewed it with the quiet, deliberate calm of a man who has already accepted his fate and is simply choosing, each day, to keep showing up anyway.

Here it is:

$spreadsheet = IOFactory::load($file); $numSheets = $spreadsheet->getSheetCount(); if ($numSheets > 1) { return redirect()->back()->with('error', 'Excel file has more than one sheet.'); }

Excel::import(new DataImport, $file); $this->export($filedata, $user, $config); return redirect()->back()->with('success', 'File imported successfully.'); } catch (\Exception $e) { DB::rollBack(); return redirect()->back()->with('error', 'File import failed: ' . $e->getMessage()); } }

public function export($filedata, $user, $config) { $getfile_name = $user->where('id', 1)->pluck('default_file')[0];

$file_path = base_path("/storage/app/main_file/$getfile_name"); if (file_exists($file_path)) { $file = $file_path; } $file = base_path("/storage/app/main_file/main_file.xls");

$num_copies = count($filedata::all()) - 1; $spreadsheet = IOFactory::load($file);

$guest_names = $filedata::all()->pluck('guest_name'); $invoice_number = $filedata::all()->pluck('reservation_number'); $payment_date = $filedata::all()->pluck('payment_date'); $room = "Room" . " " . rand(1, 19); $nights_stayed = $filedata::all()->pluck('number_nights_stayed'); $price = $filedata::all()->pluck('total_revenue');

$zip = new ZipArchive; $zip_name = now()->format('Ymd_His') . '.zip'; if ($zip->open(storage_path('app/' . $zip_name), ZipArchive::CREATE) === TRUE) { for ($i = 1; $i <= $num_copies; $i++) { $zip_file_name = 'Guest' . $i . '.xlsx'; $worksheet = $spreadsheet->getActiveSheet();

$message_index = $i - 1;

$_guestNameRow = $config->where('id', 1)->pluck('guest_name')[0] ?? 'A21'; $_invoiceNumRow = $config->where('id', 1)->pluck('invoice_number')[0] ?? 'B15'; $_dateRow = $config->where('id', 1)->pluck('payment_date')[0] ?? 'B17'; $_roomRow = $config->where('id', 1)->pluck('room')[0] ?? 'B21'; $_quantity = $config->where('id', 1)->pluck('nights_stayed')[0] ?? 'C21'; $_unitPrice = $config->where('id', 1)->pluck('unit_price')[0] ?? 'D21'; $_amountPrice = $config->where('id', 1)->pluck('amount_price')[0] ?? 'E21'; $_amountTotal = $config->where('id', 1)->pluck('amount_price_row_total')[0] ?? 'D34'; $_unitTotal = $config->where('id', 1)->pluck('unit_price_total')[0] ?? 'E34';

$spreadsheet->getActiveSheet()->setCellValue($_guestNameRow, $guest_names[$message_index] ?? null); $spreadsheet->getActiveSheet()->setCellValue($_invoiceNumRow, $invoice_number[$message_index] ?? null); $spreadsheet->getActiveSheet()->setCellValue($_dateRow, $payment_date[$message_index]); $spreadsheet->getActiveSheet()->setCellValue($_roomRow, $room); $spreadsheet->getActiveSheet()->setCellValue($_quantity, $nights_stayed[$message_index] ?? null); $spreadsheet->getActiveSheet()->setCellValue($_unitPrice, ((float)$price[$message_index] / (int)$nights_stayed[$message_index]) ?? null); $spreadsheet->getActiveSheet()->setCellValue($_amountPrice, $price[$message_index] ?? null); $spreadsheet->getActiveSheet()->setCellValue($_unitTotal, ((float)$price[$message_index] / (int)$nights_stayed[$message_index]) ?? null); $spreadsheet->getActiveSheet()->setCellValue($_amountTotal, $price[$message_index] ?? null);

$writer = new Xlsx($spreadsheet); ob_start(); $writer->save('php://output'); $file_contents = ob_get_clean(); $zip->addFromString($zip_file_name, $file_contents); } $zip->close();

header("Content-Type: application/zip"); header("Content-Disposition: attachment; filename=$zip_name"); header("Content-Length: " . filesize(storage_path('app/' . $zip_name))); readfile(storage_path('app/' . $zip_name)); exit; } } }`

Enter fullscreen mode

Exit fullscreen mode

Take it in. Sit with it. Let it wash over you like a wave you saw coming from a mile away and decided not to move for.

Now. Let's go through it together. I recommend water. I recommend a snack. I recommend telling someone you love them before we start, just in case.

The import() Method, Which Does Not Import

public function import(Request $request, ImportedFile $filedata, User $user, Excelconfig $config)

Enter fullscreen mode

Exit fullscreen mode

Four injected dependencies. FOUR. Request. ImportedFile. User. Excelconfig. A full starting lineup. An ensemble cast. A heist crew assembled with purpose and intention.

And what does import() do with them? It validates the file exists, hands it to Excel::import(), and then — immediately, in the next breath, before the import has even had time to feel good about itself — calls $this->export() and passes all four dependencies over like a baton.

import() does not import. import() is a method that watches another method work and then takes credit at standup. import() is the guy who responds "yeah, we shipped that" while making eye contact with the person who actually shipped it, daring them to say something.

Three of the four injected dependencies are not used in import() at all. They exist solely to be forwarded. They showed up for a job they were not given, were immediately redirected, and had to pretend that was the plan all along.

My senior's very first comment on this PR was: "why does import() call export()?"

Not aggressive. Not horrified. Just a quiet, five-word question with a question mark that somehow conveyed the weight of every PR he had ever reviewed before this one.

I did not have a good answer. I replied anyway. The reply was not good.

The Rollback That Has Nothing to Roll Back

Enter fullscreen mode

Exit fullscreen mode

There is no DB::beginTransaction() anywhere in this class.

I want you to understand the full scope of what has happened here. DB::rollBack() is an instruction to undo a database transaction. A transaction that was never opened. There is nothing to undo. There has never been anything to undo. The database received this rollback instruction, checked its records, found no open transaction, shrugged in whatever way databases are capable of shrugging, and moved on.

This is not a bug. It's a philosophy. It's the worldview of someone who believes that calling rollBack() near a database operation is close enough. The vibes are correct. The implementation is a void.

It's like calling refund() on a purchase you never made. It's like issuing a formal apology for something you didn't do. It's like cancelling a reservation at a restaurant you never booked, and then waiting at home for confirmation that you don't have a table.

My senior's comment here was two words: "no transaction."

Two words. No question mark. No exclamation. Two words, written by a man conserving his energy for what he was about to find next.

User ID 1. Just User ID 1. Only Ever User ID 1.

$getfile_name = $user->where('id', 1)->pluck('default_file')[0];

Enter fullscreen mode

Exit fullscreen mode

This system has one user. His name is User ID 1. He is the client, the admin, the guest, and apparently the sole known resident of the hotel this invoice generator was built for. He has no colleagues. He has no hierarchy above or below him. He simply is, hardcoded and eternal, where('id', 1), forever.

Multi-tenancy is a concept this code has never heard of. Role-based access control is a myth, like dragons, or good variable names. There is only User ID 1. He is in the controller. He is in the database. He is in my code. He is everywhere. He always will be.

To be clear: this was a single-client internal tool. There technically was only one user. But still. To look at a codebase, decide that ->where('id', 1) is fine to hardcode into a controller, and commit it — that's not just a shortcut. That's a statement of values.

The if Block That Tried So Hard and Was Immediately Betrayed

Enter fullscreen mode

Exit fullscreen mode

We check whether a dynamic file exists. If it does, we assign it to $file. This is good. This is correct. This is the code doing exactly what code is supposed to do.

And then — on the very next line, with no condition, no ceremony, no acknowledgment whatsoever of what just happened — we overwrite $file with a hardcoded path to main_file.xls.

The if block is a ghost. It built $file with love and intention, watched it get immediately overwritten, and now haunts the codebase unable to affect anything, just flickering the lights occasionally to let us know it was here once and it mattered and we chose not to listen.

Every single guest received the same template. The dynamic file path never made it. The if block knew. It stood there and watched and couldn't stop it.

I think about this if block sometimes. I think about its optimism. I think about how it ran, found the file, did its job perfectly, and then got erased before it could even tell anyone.

We're not so different, the if block and I.

Six Queries. Same Table. In a Row. Like a Person Who Has Completely Lost the Plot.

Enter fullscreen mode

Exit fullscreen mode

Six. Six times. $filedata::all(). The same query. SELECT * FROM imported_files. Six full round trips to the database before the loop has even started. Six times I loaded the entire table into memory, grabbed one column off it, and threw the rest away, and then immediately did it again.*

The correct code is one query followed by five collection operations:

Enter fullscreen mode

Exit fullscreen mode

That's it. That's the whole fix. Load it once. It stays in memory. Use it five times. The database does not need to hear from you six times about the same table in the same second. The database is not a service counter. You don't need to take a new number each time.

If the database could file a complaint it would have. It did not, because databases are more professional than this code deserved.

The Room Number Is Random. I Need You To Understand That The Room Number Is Random.

$room = "Room" . " " . rand(1, 19);

Enter fullscreen mode

Exit fullscreen mode

I am going to say it plainly and I need you to receive it plainly:

The room number is randomly generated.

This is a hotel invoice generator. Its entire reason for existing is to produce legally adjacent documents that guests may use for expense reimbursement, business travel records, or billing disputes. A guest stays in Room 7. They receive an invoice. The invoice says Room 11. They ask for a corrected invoice. We regenerate it. It says Room 4. We regenerate it again. Room 17.

There is no floor. There is no ceiling on the chaos. There is only rand(1, 19), spinning eternally, indifferent to consequences, assigning rooms with the energy of a hotel that has completely given up on the concept of room assignments as a coherent system.

The data is there, in the uploaded file. The actual room number exists. It is a column. It was imported. It is sitting in the database. And instead of retrieving it, past me looked at the data, decided rand(1, 19) captured the spirit of the thing, and moved on.

My senior's comment on this line was: "the room number is random."

Not a question. Not a fix suggestion. Just a statement. An observation delivered by someone who had clocked what they were looking at, accepted it, and decided that documentation was the only appropriate response.

I replied: "yes I'll fix it."

He replied: "ok."

That was the entire conversation. We both agreed, silently, that there was nowhere productive to go from there.

Nine Queries Per Guest. Inside The Loop. Per Guest. Nine. Per Guest. Inside The Loop.

Enter fullscreen mode

Exit fullscreen mode

Nine. $config->where('id', 1). Nine times. Per iteration. Inside the loop.

This is the same row. It is always the same row. It was the same row on the first iteration. It will be the same row on the hundredth iteration. The config table does not update between guests. The laws of physics have not changed. The database did not go anywhere. The row is still there, completely unchanged, waiting patiently to be fetched for the 847th time.

100 guests: 906 total queries. And that's on top of the 6 we fired before the loop even started, which we haven't forgiven yet.

The fix is to fetch the config once. Before the loop. Assign it to a variable. Use that variable. The database would send a fruit basket. The database would cry. Not from sadness. From relief.

My senior left his longest comment here. Four sentences. Query optimization. N+1 problems. Eager loading. The works. It was generous. It was thorough. It was the kind of comment you leave when you genuinely believe the person on the other side can be better.

I read it. I understood approximately 60% of it. I said "thanks, will fix."

The PR was closed the next day. Not merged. Closed.

$message_index, a Variable That Means $i - 1 and Wants You to Know It

$message_index = $i - 1;

Enter fullscreen mode

Exit fullscreen mode

The loop starts at 1. Arrays start at 0. We need $i - 1. This is fine. This is correct. I have no complaints about the arithmetic.

What I have complaints about is the name.

$message_index. In a hotel invoice generator. There are no messages. There has never been a message. No message is sent, received, composed, parsed, or gestured at anywhere in this codebase. But the variable needed a name, and the name it received was $message_index, with the full confidence of someone who is naming things based on vibes and hoping nobody asks follow-up questions.

It's not wrong. It functions. But it's like naming a file document1_FINAL_v3_USE_THIS_ONE_actually_final.xls. Technically a name. Technically communicating something. Not the something it thinks it's communicating.

The Ghost of PHP 4, Given the Keys to a Laravel Application

Enter fullscreen mode

Exit fullscreen mode

We are inside a Laravel controller. Laravel has a response system. It has response()->download(). It is one line. It is documented. It handles headers automatically. It does not require you to manually compute Content-Length. It does not require you to call readfile() like you're writing a blog post about PHP in 2005. And it absolutely does not require you to call exit to kill the entire PHP process when you're done.

But we didn't do that. Instead we reached back through time, found PHP 4, handed it a zip file, said "you know what to do," watched it do a little dance with raw header() calls, and then exited out of existence.

exit. Not return. exit. We terminated the process. We didn't return a response. We didn't let Laravel wrap up gracefully. We just... left. Walked out mid-conversation. The framework stood there, holding the door open, waiting for a response object that was never coming.

The Final Tally

things that went wrong:

• import() that exists to not import: ✅
• DB::rollBack() haunting a transaction that was never born: ✅
• if block built $file perfectly then watched it die immediately: ✅
• SELECT * fired 6 times on the same table, consecutively, on purpose: ✅
• 9 config queries per guest inside the loop, 100 guests = 906 total: ✅
• room numbers: rand(1, 19). not the actual room. a random room. always: ✅
• $message_index: means $i-1, works nowhere, explains nothing: ✅
• exit. just exit. in a Laravel controller. in the year of our lord: ✅*

PR comments left by senior: 7 PR comments addressed: 0 PR status: closed`

Enter fullscreen mode

Exit fullscreen mode

The Verdict

This code never shipped. The PR was closed. My senior reviewed every line of it, the phantom rollback, the six identical queries, the rand(1, 19) that could have ruined an expense report, all of it — and he left real comments with real explanations, like someone who looked at this disaster and somehow still saw a person who could learn.

He didn't laugh in the thread. He didn't forward it to anyone. He didn't bring it up in the next sprint. We agreed, without ever agreeing, to never speak of it, the way you agree to never speak of certain things that happen at work parties, certain things that happen on difficult deployments, certain things you open in a PR at 11am on a Tuesday and close by lunch.

But here is what I think about, now that I am far enough away from it to think:

He reviewed it anyway. That's the part that stays with me. He could have closed it in thirty seconds. He didn't. He went through it. He explained the N+1 problem to someone who didn't fully understand it yet. He noted the rollback. He pointed at the room number, stated plainly that it was random, and waited.

That's not a small thing. That is, in fact, the whole thing.

If you're a senior reading this: I see you. I know what it costs. Every comment you write on a PR from someone still figuring it out is a small act of faith that they will eventually figure it out. That faith is not nothing.

If you're a junior reading this: somewhere there is a folder. Not a literal one, maybe. But your senior has a folder. Full of PRs that keep them up. You are in that folder. The random room numbers are in that folder.

Do better. They're rooting for you. They reviewed the whole thing. They always do.

Episode 3 coming when I've recovered from writing this one — and when I feel ready to explain why I named a function doTheThing(). I'm not ready yet. 🏅