I personally prefer the first method of just IsAdmin(User user)
It's much easier to use, and if your criteria for IsAdmin changes at a later date (perhaps based on roles, or isActive), you don't need to rewrite your method signature everywhere.
It's also probably more secure as you aren't advertising what properties determine if a user is an Admin or not, or passing around the password property everywhere. And syrion makes a good point that what happens when your id
doesn't match the name
/password
?
The length of code inside a function shouldn't really matter providing the method does its job, and I'd much rather have shorter and simpler application code than helper method code.
Kind of echoing Basile here but with a slightly more negative tone. I'd say "yes" too, but be careful with that if you are allowing silent violations of preconditions. As a personal example, I worked in a C codebase that did this kind of stuff through an API:
int f(struct Foo* foo)
{
if (!foo)
return error;
...
return success;
}
Perhaps even with a debug log in the middle. Yet it was a massive codebase, tens of millions of lines of code, over 30 years of development, and with pretty shoddy standards. The warnings in places like logs were filled to the brim with developers having developed the habit of ignoring the output.
I found, most unfortunately, when trying to change such error checks into obnoxiously-loud assertions that there were thousands of places in the system that were violating these preconditions and simply ignoring the errors (if there were any reported, some functions just returned without doing anything in those cases). The developers just worked around it and kept trying stuff and adding more code until their code "worked", blissfully unaware of their original mistake that they were passing nulls to a function that did not allow it.
As a result, I strongly recommend doing like this:
void MyClass::setMyString(QString input)
{
assert(!input.isNull() && "Error: setMyString() received NULL argument.");
m_MyString = input;
emit myStringChanged();
}
This will bring your application to a grinding halt if the assertion fails, showing you the precise source file and line number in which the assertion failed along with this message.
This is for cases where your function is not allowed to receive null (in your case, "null" here means "empty string"). For ones that are allowed, you might simply do something else, but it wouldn't be reporting a bug (for that, use the loud and obnoxious assert
which cannot be ignored).
An example of where you might use if
might be like this:
bool do_something(int* out_count = 0)
{
...
if (out_count)
*out_count = ...; // write the result back.
return true;
}
For ones that are not allowed to receive a null, use references when possible. However, you might still encounter "nulls" if you accept smart pointers, e.g., or if you expand your definition of "null" to include empty strings as another example. In those cases, assert
your preconditions liberally if you can. assert
has the advantage of not only bringing your application to a grinding halt (good thing), but also pointing out where it was brought to a grinding halt even outside of a debugging session (but still with a debug build, e.g.), and it's also an effective means of documentation.
qDebug
is a little too silent usually. A team with good habits that never allows those outputs to be ignored might get away with keeping those to zero. But an assert
will protect you even against the sloppiest teams by bringing the entire process to a grinding halt, making these programmer errors impossible to ignore. Another minor benefit of assert
is that it can't slow down your release (production) builds, while if (...) qDebug(...)
will.
For errors caused by external sources outside of your control (ex: failing to connect to a server that was down, failing to allocate a huge block of memory, trying to read from a file the user tried to load that was corrupt, etc), throw exceptions, but not for programmer bugs like accessing an array out of bounds or receiving a null in a place that never should receive null (unless you're working in a very mission-critical software where the software attempts to gracefully recover from even programmer bugs as opposed to making them as easy to detect and reproduce as possible). For these cases, assert
.
Best Answer
What about creating a class to hold your arguments? This class would contain both
open
andclose
parameters and either of them could beNULL
. Then, there will be only onestrip
method with above class as argument and method will decide if it wants to useopen
/close
if they are set.