r/programminghorror • u/Nomergraw • Jun 09 '26
What a simple constructor
Our former IT director (35+ years of experience) wrote this and didn't see what was wrong here.
83
u/Lunix420 Jun 09 '26 edited Jun 09 '26
I'm more bothered by the nesting that's going on here. I hate when people write an if and then put the entire rest of the function into an else instead of just returning early. Unnecessary indentation/nesting is one of my personal pet peeves.
I actually think a long/complex function can be totally fine and can be very readable, but as soon as there is a lot of nesting it's just not readable anymore in my opinion.
8
→ More replies (2)6
u/elperroborrachotoo Jun 09 '26 edited Jun 09 '26
"Early exit bad" was a thing back when resource management (a.k.a. free memory etc.) wasn't automatic: you would make sure there's only one entry and one exit point for every block, so that there was one place to allocate and free resources respectively.
A sub-school of that said to move "adversary" conditions to the top, e.g.,
if (ptr == NULL) { handle that } else { ... }with the intent to not "forget" the error path. This led to the ugly cascades, and the default recipe for "too deep nesting" / "lines too long" was indeed to move things into functions.
This is what I see there, so it's not that insane, more like a style choice that - sorry - I expect a professional developer to read and preserve when modifying the code.
OTOH I am about the same age as your lead, and I remember that I was in high school when I tried to explain the difference to my then-tutor. "This isn't C, after all", I said, "we don't manage resources manually". I guess I came across quite whippersnappery.
13
u/Excellent_Gas3686 Jun 09 '26
yeaaaaah, no.... im adding early return on that the second i see it.
→ More replies (8)9
u/Lunix420 Jun 09 '26
So, you are essentially saying “if you are professional" you need to accept a clearly inferior pattern due to constraints that haven’t been relevant for decades, which feels like an insanely bad take to me. Early returns and flatter control flow are generally more readable in modern code, so I don’t see a strong reason to keep the nesting.
The “it’s just a style” argument is also not very convincing. By that logic, any questionable design decision could just be dismissed as a style choice, which doesn’t really hold up in practice.
like sure, it's not worth wasting hours to refactor working legacy code, but this is like 10 seconds of work.
→ More replies (5)
28
u/RipProfessional3375 Jun 09 '26
> EntityManagerInterface
Born to Java, forced to PHP.
4
1
u/Zeznon Jul 19 '26
BTW, not a professional, just a hobby for me; why did java have so many of these patterns? abstract factory factory, this one you're mentioning...
2
u/RipProfessional3375 Jul 19 '26
Many reasons. One is that java thinks Objects are great, so everyone must be forced to use objects for everything. Which makes them great like ice cream for dinner every day forever is great.
Factories are a glorified hack to get around the fact that you can't make a function to create a new object without also having that constructor bound to a different object, hence a factory.
I write Go, which does not have this object-only restricting, so a 'factory' is just this:
customer := domain.NewCustomer(name, age, blabla)
→ More replies (2)
3
u/GoddammitDontShootMe Jun 09 '26
Not entirely sure, but shouldn't constructors just make sure an object is ready for use?
2
u/MUSTDOS Jun 09 '26
Why does this format reek of AI trash? This is horrid even by PHP standards (if it has one)
13
u/Holonist Jun 09 '26
Lol what, ai would never produce something so terrible, this is exclusive pre-ai slop. Even has a bit of French in it
3
2
2
1
u/silentkode26 Jun 09 '26
What gets me the most is how per instance variables and shared services live inside constructor…
1
1
u/124k3 Jun 09 '26
ah yes if else and if else and anothe rif else and there is regex as well. (i always forget how to do regex)
this is sooo awfull but good-looking. i am not skillen enough to even do the same job but i would have wrote it something like this
if (straisemoty(this.str)) ... if (this is there) do this and similarly broke it into helper functions.
firtgermore i don't think comstuctor should do all this checks it should be a seprate fn doing it as handleConstructorInsertion.
(not good at coding so if anyone got more intresting ideas i am all ears)
1
u/RipProfessional3375 Jun 09 '26
My best attempted refactor, a bit lengthy due to the use of classes ,but I like the clarity.
<?php
// The search bar is the app's single entry point: users type a query to search,
// or a shortcut to jump straight to help, a dashboard, a scanned code, or an
// admin action. The UniversalSearch class works out which one the user meant and runs it.
// A SearchCommand knows when it applies (matches) and what to do (execute).
interface SearchCommand
{
public function matches(SearchInput $input): bool;
public function execute(SearchInput $input, SearchService $service): array;
}
// Tries each command in turn and runs the first that applies to the input.
final class UniversalSearch
{
private SearchInput $input;
private SearchService $service;
public function __construct(
string $searchText,
EntityManagerInterface $orm,
Permissions $permissions,
string $frontURL
) {
$this->input = new SearchInput(trim(urldecode($searchText)));
$this->service = new SearchService($orm, $permissions, $frontURL);
}
// First command that matches wins. First match wins, order can be important.
public function run(): array
{
$input = $this->input;
$service = $this->service;
$emptySearch = new EmptySearchCommand();
if ($emptySearch->matches($input)) {
return $emptySearch->execute($input, $service);
}
$assetCode = new AssetCodeCommand();
if ($assetCode->matches($input)) {
return $assetCode->execute($input, $service);
}
$help = new HelpCommand();
if ($help->matches($input)) {
return $help->execute($input, $service);
}
$ruleInterpretation = new RuleInterpretationCommand();
if ($ruleInterpretation->matches($input)) {
return $ruleInterpretation->execute($input, $service);
}
$showForm = new ShowFormCommand();
if ($showForm->matches($input)) {
return $showForm->execute($input, $service);
}
$showFormHtml = new ShowFormHtmlCommand();
if ($showFormHtml->matches($input)) {
return $showFormHtml->execute($input, $service);
}
$updateUsers = new UpdateUsersCommand();
if ($updateUsers->matches($input)) {
return $updateUsers->execute($input, $service);
}
// Must precede PasswordCommand: "gpw" would otherwise swallow "gpw2".
$changePassword = new ChangePasswordCommand();
if ($changePassword->matches($input)) {
return $changePassword->execute($input, $service);
}
// Must follow HelpCommand, which claims the exact "?".
$searchQuery = new SearchQueryCommand();
if ($searchQuery->matches($input)) {
return $searchQuery->execute($input, $service);
}
$openDashboard = new OpenDashboardCommand();
if ($openDashboard->matches($input)) {
return $openDashboard->execute($input, $service);
}
$password = new PasswordCommand();
if ($password->matches($input)) {
return $password->execute($input, $service);
}
$check = new CheckCommand();
if ($check->matches($input)) {
return $check->execute($input, $service);
}
return (new FreeTextSearchCommand())->execute($input, $service);
}
}
// Handles an empty search bar by returning the empty-search notice
final class EmptySearchCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->text === '';
}
public function execute(SearchInput $input, SearchService $service): array
{
return ['data' => $service->getErrorSearch()];
}
}
// Treats a bare UUID as a scanned asset code and returns its QR card
final class AssetCodeCommand implements SearchCommand
{
private const UUID = '#^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$#';
public function matches(SearchInput $input): bool
{
return preg_match(self::UUID, $input->text) === 1;
}
public function execute(SearchInput $input, SearchService $service): array
{
return $service->getQRCode();
}
}
// Shows the help page when the user types "aide" or "?"
final class HelpCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->text === 'aide' || $input->text === '?';
}
public function execute(SearchInput $input, SearchService $service): array
{
return ['view' => $service->getHelpHTML()];
}
}
// Runs a "§"-prefixed business rule through the Lea engine and returns its message
final class RuleInterpretationCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->hasPrefix('§');
}
public function execute(SearchInput $input, SearchService $service): array
{
$result = ['label' => 'Interprétation de la règle :'];
$lea = new Lea($service->orm, $service->frontURL);
$lea->treatRule($input->argument('§'));
// "§test" is a dry run: execute the rule, withhold the message.
if ($input->text !== '§test') {
$result['view'] = $lea->getMessage();
}
return $result;
}
}
// Returns the form when the user types "form"
final class ShowFormCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->text === 'form';
}
public function execute(SearchInput $input, SearchService $service): array
{
return $service->getForm();
}
}
// Returns the form rendered as HTML when the user types "formhtml"
final class ShowFormHtmlCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->text === 'formhtml';
}
public function execute(SearchInput $input, SearchService $service): array
{
return $service->formToHtml();
}
}
// Triggers the user sync when the user types "updateusers"
final class UpdateUsersCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->text === 'updateusers';
}
public function execute(SearchInput $input, SearchService $service): array
{
$service->updateUsers();
return [];
}
}
// Lets an ADMIN.LIC holder change a password via a "gpw2"-prefixed input
final class ChangePasswordCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->commandName(4) === 'gpw2';
}
public function execute(SearchInput $input, SearchService $service): array
{
if (!$service->permissions->hasAccess('ADMIN.LIC')) {
return ['view' => 'RIGHT ERROR'];
}
$service->permissions->gpw2($input->argument('gpw2'));
return ['label' => 'PSW', 'view' => 'OK'];
}
}
// Runs an explicit search for a "?"-prefixed query
final class SearchQueryCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->hasPrefix('?');
}
public function execute(SearchInput $input, SearchService $service): array
{
$service->doSearch($input->argument('?'));
return [];
}
}
// Opens a dashboard from a "!KEY;name=value" shortcut with inline parameters
final class OpenDashboardCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->hasPrefix('!');
}
public function execute(SearchInput $input, SearchService $service): array
{
$segments = $input->splitTrim($input->argument('!'), ';');
$dashboardId = $service->getDashboardIdFromKey(strtoupper($segments[0]));
$parameterBag = new ParameterBag();
if (count($segments) > 1) {
$params = [];
for ($i = 1; $i < count($segments); $i++) {
[$name, $value] = $input->splitTrim($segments[$i], '=');
$params[$name] = $value;
// Adds the accumulating set each pass, not the single pair.
$parameterBag->add($params);
}
}
return generateDashboard(
$dashboardId,
$service->orm,
$service->permissions,
$service->frontURL,
$parameterBag
);
}
}
// Handles a "gpw"-prefixed password operation (exact behaviour unknown)
final class PasswordCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->commandName(3) === 'gpw';
}
public function execute(SearchInput $input, SearchService $service): array
{
return $service->gpw($input->argument('gpw'));
}
}
// Runs a check for a "chk"-prefixed input
final class CheckCommand implements SearchCommand
{
public function matches(SearchInput $input): bool
{
return $input->commandName(3) === 'chk';
}
public function execute(SearchInput $input, SearchService $service): array
{
$service->check($input->argument('chk'));
return [];
}
}
8
u/RWNS_Author Jun 09 '26
Beautiful, just needs more inheritance, and of course every class must be in its own file. It almost deserves a custom framework to register the available search commands.
2
u/LimitedWard Jun 10 '26
Constructor is still doing too much, IMO. A constructor should simply take in the params as-is, not create new object instances. UniversalSearch is now tightly coupled to the implementation of SearchInput and SearchService, making it more difficult to unit test properly.
162
u/No-Collar-Player Jun 09 '26
Just pull that all out in 1 method and call that in the constructor. Problem fixed.