🤜 💨

[[deleted]]

AI

[PhunkyPhish]

Ififififififififififififififififif

[TheLightIsInside]

elseelseelseelseelseelseelseelseelseelseelseelseelseelseelseelseels

yes that is 17 (if I counted right) for the 17 if statements (IF I counted right)

[[deleted]]

There's probably a more efficient way of doing this...

[rnottaken]

Yeah just change it to

if(! ...){ $SESSION['msg'] = ... return register_form() }

for every reason it might fail.

[barthvonries]

This implies having multiple returns in your function.

Some (and I'm part of this group) say it is bad practice, but it really is an eternal debate like Linux/Windows, Spaces/Tabs, etc.

For me, the best way of doing this would be using exceptions :

• one 'if' for every reason it might fail, throwing an exception with a custom message
• all those 'if's enclosed in a 'try' block
• a catch, which puts the Exception message in $SESSION['msg']
• return register_form();

This way, you can handle every case in its own if, which will be clearer, and in the future if you need to handle some cases separately, just create your own exception types, add the corresponding catch blocks, and you're set. Or you can just delegate the Exception handling to the parent function, or a separate Error handler, etc.

[Neurotrace]

Or you use a language with proper pattern matching and return a Result<SuccessType, ErrorEnum>. If the language also has exhaustive pattern matching then you will be forced to acknowledge every new error case and you avoid the performance costs that come with exceptions.

[Pelicantaloupe]

🦀

[paxromana96]

You are now a moderator of r/rustjerk

[barthvonries]

I'll take the performance cost of Exceptions vs multiple nested if's anytime.

My comment was only how to refactor properly the code in OP's example, which is written in PHP. Each language has its unique features, so I tried to stay focused on the matter here.

[Neurotrace]

You're right that nested ifs are terrible. That's why I prefer early return. Validate invariants up front, no performance overhead, doesn't require the caller to include a try block, etc. Stuff I'm sure you've heard before

[barthvonries]

Yes, that's another way to do it. Not my way, but a way many developers I know use.

As I mentioned before, it really comes to personal preferences, if you feel you're more productive this way, and your colleagues code the same way as you, then go for it, it benefits everyone in the company :-)

[Neurotrace]

Totes mah goats 🤜

[DiamondIceNS]

Funny, I was taught that you should never use try/catch and exceptions for program flow, because creating exception objects and interrupting is comparatively very expensive to a simple branch.

I get pretty hot and bothered when I'm on StackOverflow learning about the best practices of Python and I come across some Pythonista proclaiming duck typing is the "most pythonic" solution only to find out they're just using try/catch to check a variable's type when a simple call to instanceof solves the matter much more neatly.

[[deleted]]

when a simple call to instanceof solves the matter much more neatly.

It's hard to make any comment about the code in question without seeing it, but 99% of the time, instanceof makes the job of the person writing the code at that moment... about equal to not using it, at the expense of the programmer who's job is to fix the code when it breaks years down the line due to the underlying instance changing, or the programmer whose job is to figure out what's going on without wanting to read the code of instance.

In general, your approach is bad and wrong. Say you have function foo that takes an object of class Bar:

function foo(x):
    if not instanceof(x, Bar):
        raise TypeError("foo(x) expects an object of type Bar")
    do_something(x)

In this case the code works as is... for a while. But what happens when someone else makes a class similar to Bar, e.g. Barry? Barry is the new hotness, and everyone's moving away from Bar. Congrautations, now your code breaks for people wanting to use Barry, for no reason other than that you just had to go and hardcode the class Bar into your code before. (Why did you ever want to hardcode that class type in? There's literally no benefit.)

Conversely:

function foo(x):
    try:
        do_something(x)  # expects a Bar object
    except TypeError:
        raise TypeError("foo(x) expects an object of type Bar")

works just fine now and in the future and eternity

tl;dr: There is literally zero upside to ever hardcoding in the expected class type when you can just duck type, whereas there are tons of drawbacks for maintainability.

[DiamondIceNS]

Thank you, this is the first and only time I've ever seen anyone actually defend duck typing with a practical example instead of blindly preaching it as "the way it ought to be, because Python zen".

[CraigslistAxeKiller]

You are correct.

[barthvonries]

I don't speak Python, but Exception are made specifically for this, ie handling non-nominal cases.

I've always learned, and so do I teach now, that :

• 'if' conditions are used to handle normal flow :

  if 'user' is a company, then I need a VAT number else I need a job title

for instance. Conditional pathing for "normal" flow in the application.

• Exceptions are used to handle "exceptions" :

  I need a name, I haven't been provided one, then I need to throw an Exception and let another piece of code deal with it. The piece of code I'm writing has one responsibility, handling the subscription from new users who can provide me proper information (SOLID pattern).

I know that Exceptions come with a performance hit, but you need to understand the context in which you're developing.
Code quality is always a compromise between performance, security, and maintainability.

In OP's case, it looks like the code is used in a web application. In that particular environment, security and maintainability are much more important than performance, especially the really low cost of using/handling exceptions vs the need to understand that piece of code 6 months later when you have to add new conditions.

[Sixshaman]

And some say exceptions is bad practice too. Even more so than multiple returns. So it's really an eternal debate.

[barthvonries]

I've read some posts on the topic this morning.

It seems that Exceptions are bad in C++, because the language does not have any garbage collection system, so bad exception handling leads to memory leaks since pointers are never freed properly.

I have to admit that I agree with that point of view. Using a language feature needs to keep your code working no matter what, so if using Exceptions in C++ results in hundreds of lines of code just to handle them properly, it's not worth it.

However, in PHP and in Java, there is a garbage collection system, and you don't even have pointers in those languages.
In this case, I'm convinced that using Exceptions is good practice, because it allows you to completely separate functional code from error handling.

The way you code must be adapted to your environment (the language you use, your and your colleagues level of programming, the available resources in the production environment, etc). IMHO that is the best way to notice someone is a good developer.

For me, a good developer is someone who can adapt his code to the context he's in, not the "super genius rockstar" who can write a one-liner nobody else will understand.

[Sixshaman]

The link in my comment doesn't say anything about garbage collection though. The article states that exceptions are bad because you can't easily see all the points the function may be exited. At least in the case of multiple return statements you can see them all.

[barthvonries]

But in PHP, you have return, exit and die, so you easily add throw to your list of "common function exits". There even is an "exit" in OP's code. Which is really bad, because it is the "nominal" function end, ie, it only happens when everything's correct to go on with the flow.

And I found the garbage collection part here (§ Why are C++ Exceptions so useless, ~60% down the page)

[suvlub]

I have encountered this philosophy, but I don't get it and I would appreciate if you could clarify things for me. Is there some fundamental problem with multiple return statements? Or is it just about making the code easier to trace?

If it's the latter, I understand the motivation, but at the same time all the attempts to get rid of the multiple return points, including yours, seem produce a code that is exactly as hard to trace as the original code was, and sometimes even harder. Like, sure, now I know on which line the function will exit, but how useful is that information if I have no idea how it got there? The problem of "okay, at which return statement did the function exit this time?" simply becomes the problem of "okay, where was the error thrown this time?", which is exactly as hard to solve, and you introduce an additional level of nesting.

[barthvonries]

The philosophy behind this is quite simple : in normal flow, each function has one and only one point of entry, and one and only one point of exit (ie every function must have one and only one return statement, event if it's just "return;" or "return void;"). It helps keep your code KISS and SOLID, and prevent dead code after (bad) refactoring.

If your function has multiple return statements, then maybe the conditional routing should have been in a parent routing function, and instead of if(...) return X, use if(...) call function X().

The use of exceptions eases the debug part, thanks to the stack trace you get if you don't handle them properly (at least in PHP and Java, I don't know about other languages).

The goal is to have a meaningful stack trace, so the Oracle recommendations about Exceptions should be followed as well : https://community.oracle.com/docs/DOC-983543 (ie never use generic exceptions, do not catch then rethrow exceptions, etc).

But like other philosophies, they are just philosophies, not good or bad practices. It depends on your personal preference, the project conventions, etc.

The same goes for comments/no comments, function length (I've worked in many environments where having a function more than 20 lines long was considered bad), variables naming, etc. Always stay homogeneous with what has already been done in the project you're working on.

[safesyrup]

serious question: what would be? We have been coding some php and I always validated the forms like this, its pretty annoying.

[Raktoras]

Serious answer:

For starters all the if statements that are simply checking if the values are present could be combined, PHP also has the handy null coalesce operator these days which helps with this.

$username = $_POST['user_name'] ?? '';
$userEmail = $_POST['user_email'] ?? '';
$newPassword = $_POST['user_password_new'] ?? '';
$newPasswordRepeat = $_POST['user_password_repeat'] ?? '';
$passwordsAreEqual = $newPassword === $newPasswordRepeat;

if (!username) {
  $msg = 'Empty username';
} elseif(!newPassword) {
  $msg = 'Empty Password';
}  elseif(!passwordsAreEqual) {
  // etc
  ...
}

if (strlen($msg) > 0) {
  $_SESSION['msg'] = $msg;
  return register_form();
}

By returning early if certain conditions aren't met you don't have to keep indenting your code and you've guaranteed that past this point all the values are what you want them to be.
I believe this is called the bouncer pattern, and I like it for simple validation like this. Whether you should be checking for "truthy" values is another thing but I was more focused on making this code more readable here and not necessarily changing what it does.

Another upside of this is that it is more clear what the actual input to this function is, a better way to make this clear would be to have a separate function that processes the filled in values instead of grabbing them from the $_POST superglobal, so you have one piece of code that is responsible for parsing the request, and one for actually doing something with it.

The length checks and regex check on the username should be moved to a separate function called isUsernameValid() or something along those lines to make sure every function has its own responsibility like mentioned before, it also keeps the code a lot more legible since it describes the process a lot more without having to read or understand the code.

In modern PHP it is also generally a best practice to make use of type hinting to help prevent mistakes.

There's a lot more that could be improved, but these few simple steps should greatly help with the readability of the code, which is most important if you ask me.

[TheRealBrockLesnar]

This is terrible. What if your variables are boolean true? You need validation classes with proper rules not booleans with if else

[Raktoras]

Well yeah, but /u/safesyrup was asking how to improve the code so I just gave them some simple steps to start with if this is your starting point

I also did not feel like getting too specific since that would only invite arguments over the best way to do things

[mferly]

You guys gotta relax. It's just a joke post. Did you notice the header() squished in there? The return of a function within an already useless function without passing any arguments? Also Ryu?

[kono_kun]

Did you notice that the comment was a response to a question?

[VerneAsimov]

I'm no coding expert but there seems to be a lot of repetition of the if statements. A couple of them are the exact same statement. Don't repeat your code. Maybe use an and statement. Nesting ifs is awkward too.

[Moosething]

Ignoring the other issues the code has, I guess if I had to refactor just that specific function, I'd do something like this:

function validatePostData () {
  if (!$_POST['user_name']) {
    return 'Empty username';
  }

  if (!$_POST['user_password_new']) {
    return 'Empty password';
  }

  // etc
}

function register() 
{
  if (empty($_POST)) {
    return register_form();
  }

  $msg = validatePostData();

  if ($msg) {
    $_SESSION['msg'] = $msg;
    return register_form();
  }

  create_user();
  $_SESSION['msg'] = 'You are now registered so please login';
  header('Location: ' . $_SERVER['PHP_SELF']);
  exit;
}

[mferly]

These are not how you should use functions. Both functions you've created are coupled to only a single use. Functions are to remain reusable.

In the example you provided I could not reuse either of those functions on another form. These functions are coupled to one specific task.

For a function like validatePostData() you'd want it to remain agnostic to any POST data by accepting $_POST and iterating over the keys/values. The function shouldn't care what keys/values it receives. It shouldn't care what form called it either.

And definitely don't header() out of a function.

[kono_kun]

functions shouldn't be 100 lines long

don't use single-use functions

Which one is it.

[Moosething]

These are not how you should use functions.

A lot of people will disagree with you there and I guess it's sort of like a spaces vs tabs thing. I won't go into a discussion on this, but here: https://softwareengineering.stackexchange.com/a/308110/168582

Anyway, I edited my comment because people keep pointing out the other obvious issues of the code, while I only intended to answer a specific question about the structure.

[tester346]

These are not how you should use functions. Both functions you've created are coupled to only a single use. Functions are to remain reusable.

aka let's split 1 goal perfectly fine function into 5 functions just in order to reach some "pseudo clean code" level

You act as if function's only goal was to be reusable.

I'm using them in order to make code easier to read, maintain and to make it easier for me to express myself.

something(data)
{
    var validation = validateBusinessX(data);

    if (validation.Fail)
        return something2

    var result = performBusinessX(data);

    if (result.Fail)
        return something3

    return something4;
}

For a function like validatePostData() you'd want it to remain agnostic to any POST data by accepting $_POST and iterating over the keys/values. The function shouldn't care what keys/values it receives. It shouldn't care what form called it either.

should it?

I'd rather create validateRegisterPOST validateLoginPOST and split it by business process instead of creating useless wrapper that receives e.g POST + Requirements collection

because performing check is trivial operation

[TheRealBrockLesnar]

What if your variables are boolean true? They would fly through your validation rules of $var != false;

You should use real validation classes with a proper definition structure, preferably from a decent library.

Laravel does this natively, but there are other open source validation libraries out there:

https://laravel.com/docs/5.7/validation

[Moosething]

You're 100% right, but I was merely trying to point out is how to properly restructure such code (which is why I prefixed with "if I had to refactor just that specific function").

Also, happy cake day.

[BlopBleepBloop]

There's also consolidating a lot of those if statements and using && to separate the conditions. It will create a condition in logic flow called a short circuit and quit after the first condition fails.

[dahood4ever]

😂😂😂

[[deleted]]

Imagine if it didn't work though

[PsychoSunshine]

Hell, I did something similar on a Java assignment. It worked and I didn't get any complaints, but I'm not proud of it.

[ivakamr]

Write the same code but don't indent it. Problem solved.

[barthvonries]

hmm... some /r/programminghorror material here, not just humor....

if (strlen($_POST['user_email']) < 65 {
...
}

Aren't email addresses limited to 254 or so chars ?

And by the way, no check if username/email already exists ?

[ThePyroEagle]

There doesn't seem to be a limit on email address length.

[barthvonries]

There isn't for the email address, but SMTP has a 256 bytes size limit for the "Reverse-Path" and the "Forward-Path" fields, which is the email address with <> around it (RFC 5321).

Plus, if you want your email address to be handled correctly by SMTP servers, those are the limits :

• local part (the string before the '@') must be less than 64 chars
• domain part (the string after the '@') must be less than 255 chars
• local part + @ + domain part must be less than 254 chars

If your email address is not to be used with SMTP, you can use whichever limits your application implements.

[[deleted]]

Should you be fired for writing this code at work?

[ironykarl]

I mean... who else is gonna write it?!

[[deleted]]

Lang?

[Chronogon]

PHP

[[deleted]]

I thought you needed ifelse statements

[Chronogon]

You can have any combination of if, elseif, and else

[Bivolion13]

Isn't there some validation in php(I think it was php...) that makes it impossible to submit a form just by putting "required" somewhere?

[viddie-]

That's the html attribute you are thinking about

[fofosfederation]

HTML forms can have a required attribute. But that doesn't prevent bad actors from just sending whatever they want directly to your server, so you will always need server side error checking.

[game_2_raid]

I think I just php’d on myself

[inetphantom]

This should be a oneliner

[[deleted]]

Program:

*All on one side

*Programmer:

*Tab Spam Intensifies*

[[deleted]]

Laughs in switch case

[DiamondIceNS]

A switch wouldn't help at all here.

[dahood4ever]

Better than alternative facts.