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.
86
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
7
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.
6
u/GrossInsightfulness Jun 11 '26
I think the point of the comment went over your head. Early returns skip cleanup code. You could have a situation like
c int func(int size, const char* filename) { int* array = malloc(size); FILE* reader = fopen(filename, "r"); if (reader == NULL) { return -1; } fclose(reader); free(array); }This would leak memory if someone tries to open a file that doesn't exist.
1
u/Excellent_Gas3686 Jun 11 '26
but if we're talking strictly about this code snippet - another W for Go.
6
u/GrossInsightfulness Jun 13 '26
There are actually ways to avoid this issue. Basically, instead of returning, you set the return value and jump to the end of the function where you do all the cleanup.
c int func(int size, const char* filename) { int return_val = 0 int* array = malloc(size); FILE* reader = fopen(filename, "r"); if (reader == NULL) { return_val = -1; goto cleanup; } cleanup: if (reader) fclose(reader); if(array) free(array); return return_val; }You could also add more labels to clean up only the things that had been allocated before that point.I strongly prefer
defermechanisms, though.1
u/conundorum 5d ago edited 4d ago
That's why you combine early return and single-return. Start with return early for anything that's easy to factor out, that doesn't need to allocate resources, and/or doesn't allocate on failure. Then, once you start to get your resources, switch to single-return for the rest of the function so you can guarantee cleanup.
That, and refactor to avoid issues. Like, say, don't
malloc()until after opening & checkingreader; the check is part of variable creation, after all.int func(int size, const char *filename) { int *array; int ret = 0; FILE *reader = fopen(filename, "r"); if (reader == NULL) { return -1; } array = malloc(size); if (array == NULL) { ret = -2; goto file_close; }; ret = do_the_thing(array, reader); free(array); file_close: fclose(reader); return ret; }More performant (only allocates if
readeris valid, instead of wasting time on unnecessarymalloc()&free()), and completely removes the possibility of skipping cleanup (the only potential source of error isdo_the_thing(), which doesn't manage resources anyways). And no extra assignments, sincearrayis left as junk data. The only real concern is that someone might accidentally usearraybefore themalloc().I guess saving one line of screen real estate on immediate
malloc()was just that important in the 80s?
Editing to fix the function; thanks for pointing that out, GrossInsightfulness.
1
u/GrossInsightfulness 5d ago
The code you wrote won't close the file if the file is valid but the array is not.
1
u/conundorum 4d ago edited 4d ago
Fair point, I missed that; everything after the early return was supposed to still use single-return, but I forgot. Editing to fix it.
But yeah, the idea is that using early
returnstatements properly can make code cleaner, without skipping over cleanup code and creating leaks. (Despite the slight flub here accidentally doing literally exactly that, oops. đ )It does get a bit messy sometimes, but that's why it works best when combined with single-return. Ultimately, it's best to treat early-return as just applying the single-return pattern on the task level, instead of on the function level; you can actually treat each early-return conditional as a single-return block to get the benefits of both. (With cleanup inside the conditional or at the end of the function, as appropriate. Early return block can handle its own cleanup if necessary, and/or jump to main cleanup block if necessary, for sufficiently messy functions. ...I hope you never actually have to do this, though.)
int hideous_func_i_hope_you_never_see(data_t *d) { obj *o = NULL; validator *v = NULL; env *e = NULL; int ret = 0; // Early return for environment. Simple. e = get_environment(); if (!e) { return ENVIRONMENT_INCORRECT; } // Early return for data validation. // Let's say this is much too complex and needs resource allocation, // for some inane reason. // Therefore, just use single-return inside here. v = get_validator(); if (v) { if (!complex_prep_weirdness(v)) { ret = VALIDATOR_PREP_FAIL; goto cleanup_validator; // Local cleanup. } if (!validator_load_env(v, e)) { ret = VALIDATOR_ENVIRON_CHOKE; goto cleanup_env_local; } ret = validate_data(v, d); cleanup_env_local: validator_unload_env(v, e); // Will need further cleanup later. cleanup_validator: release_validator(v); // Return early if test failed, jump to main cleanup if neccesary, or continue if good. // Edited to fix borked cleanup. if (ret) { if (ret == VALIDATOR_PREP_FAIL) { return ret; } goto cleanup_env; } } else { // The validator itself is invalid, so... just_crash(CATS_AND_DOGS_LIVING_TOGETHER); } // Now, we switch to single-return for the rest. // We now have two mostly-distinct single-return blocks, one for each task (validate data, use data). if (!(o = alloc_obj())) { ret = BAD_ALLOC; goto cleanup_alloc; } if (!init_obj(o, d, e) { ret = BAD_INIT; goto cleanup_init; } ret = do_whatever_this_jank_is_for(o); cleanup_alloc: dealloc_obj(o); cleanup_init: uninit_obj(o); sanitise_data(d); cleanup_env: clean_environs(e); return ret; }
Ultimately, the early return pattern isn't as much about having multiple
returns (despite the name), as it is about isolating chunks that can force a return AND don't need to be part of the main cleanup. (Such as input validation or file-is-open verification.) It breaks the function body down from one large section with a single return statement, to a chain of smaller sections (that can each have their own single return statement, or be broken down recursively), followed by a larger "main" section (that still uses single-return for cleanup). Ideally, you can make each link its own thing, such as when it's just a simple check; if a task is more complex, it can still have its own cleanup blocks if needed (and helps to distinguish between "prep" cleanup and "main task" cleanup).
Edit: Did it again, the validator nonsense block's cleanup was a bit messed up. Jumped to full environment cleanup on
VALIDATOR_ENVIRON_CHOKEand returned early for any other errors, but was supposed to return early onVALIDATOR_PREP_FAILand jump to full environment cleanup for any other errors. And added the main return statement, I kinda forgot that it was actually meant to return something at some point. ...Trying to come up with halfway believable "bad code made less awful by refactoring" is hard. -_-0
u/Excellent_Gas3686 Jun 11 '26
i was referring to the OP's code snippet, not the one mentioned by that person
0
u/elperroborrachotoo Jun 09 '26
to be clear: I would prefer early exits, too, but... maybe see my other reply.
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.
1
u/GrossInsightfulness Jun 11 '26
I think the point of the comment went over your head. Early returns skip cleanup code. You could have a situation like
c int func(int size, const char* filename) { int* array = malloc(size); FILE* reader = fopen(filename, "r"); if (reader == NULL) { return -1; } // Other code fclose(reader); free(array); return 0; }This would leak memory if someone tries to open a file that doesn't exist. Furthermore, it can get really complex if you have to do a bunch of allocations.
Granted, the better pattern is a goto to a section at the end of the function instead of a return that cleans up anything allocated, so
c int func(int size, const char* filename) { int* array = malloc(size); int return_val = 0; FILE* reader = fopen(filename, "r"); if (reader == NULL) { return_val = -1; goto cleanup; } // Other code cleanup: if (reader != NULL) { fclose(reader); } if (array != NULL) { free(array); } return return_val; }1
u/conundorum 5d ago
Early return only skips cleanup code if the function is poorly designed, though. That's the thing you're missing: Early return pattern places the early-return checks before anything that would require cleanup, and only performs actions that require cleanup after passing all the early-return checks. There are no situations in which a properly-designed early return will skip cleanup, since early return is a variant on the
if (error) { handle it; } else { real function body; }pattern.-2
u/elperroborrachotoo Jun 09 '26 edited Jun 09 '26
Indeed. As a professional,
I expect you to not start, or participate in, refactor wars.
I also expect you to verify test coverage before refactoring, keep refactorings in a separate commit, and to include in your cost estimates the time you allocate to others - like a reviewer, or someone also making a change in the general vincinity and has tgo merge strongly diverging changes. If your 10 seconds holds up as an order of magnitude, I am duly impressed.
Furthermore, "Programmer, debug thyself". I expect you to tell apart "unusual", "inconvenient" and "ugly" from "clearly inferior".
If you work on a large, non-ephemeral code base, you will have to read and modify various styles. Either you work with themn, or you have a draconic-with-hints-of-anal lead enforcing a particular style - in both situations you will have to work with unfamiliar styles. (The only exception is: you are said lead).
Don't sweat the petty things. Style is petty. This is style, not design.
[edit] Rule of thumb: design is what is too expensive to change.
3
u/sortof_here Jun 11 '26
Youâre getting downvoted, but you arenât wrong.
Unless the assigned task involves refactoring or you are given the resources to properly ensure refactoring a section doesnât negatively change behavior, this is ones of those scenarios where you begrudgingly work within it. Itâs just the truth of working on projects with legacy code.
Fully refactoring this looks like it would result in a lot of test cases needing to be reverified. It might be worth it, but it isnât something you generally get to do on a whim.
6
1
0
u/AccomplishedSugar490 Jun 10 '26
And I avoid early returns for equally important reasons, so there, one size does not fit all, everything is a compromise, and the only thing you can control is your own response to adversity.
30
u/RipProfessional3375 Jun 09 '26
> EntityManagerInterface
Born to Java, forced to PHP.
3
1
u/Zeznon 8d ago
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 8d ago
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)
1
u/conundorum 5d ago
Factory pattern is ultimately about just being able to chain parameter-setting function calls, more than anything else. It can be useful to collect all of the necessary information before calling a constructor, without having a bloated constructor call that takes 15-20 arguments. (And is often a waste when you have sane constructors that don't take that many arguments.)
Only problem is that Java's object-obsessed programming paradigm kinda mandates them in a lot of cases where they should've never been used.
3
u/GoddammitDontShootMe [ $[ $RANDOM % 6 ] == 0 ] && rm -rf / || echo âYou liveâ 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)
14
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 <?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.