Is it considered readable to call methods inside the IF condition? [closed]

Closed. This question is opinion-based. It is not currently accepting answers.

---

开发者_JAVA百科

Want to improve this question? Update the question so it can be answered with facts and citations by editing this post.

Closed 1 year ago.

Improve this question

Is this way of writing IF conditions considered good coding style in Java and C# languages or not?

if (checkIfIdInFirstRange()){
    //call Range1 handling method
}else if(checkIfIdInSecondRange()){
    //call Range2 handling method
}else{
    //call error handling method
}

I'm wondering about the method inside the IF condition itself, or would it be better to make it like:

int idInRange = getIdInRange();
//handle isInRange

---

I think this is fine.

Even better is if you phrase your methods like a question, or flows with the if statement

if (thisConditionIsTrue()){
    // Do this
}elseif(anotherConditionIsTrue()){
    // Do this instead
}elseif(isThisParameterOkay(someParameter)){
    // Yeh do this
}

Some hardcore purists will even say that if you have > 3 levels of indentation, your method is too nested and should be split into smaller methods.

---

Doing this is IMHO good coding practice as long as the method calls don't have any side effects.

e.g.

if checkIfIdInFirstRange() this is OK:

private bool checkIfIdInFirstRange()
{
  return firstRange.Contains(ID);
}

But doing this might create confusion:

private bool checkIfIdInFirstRange()
{
  SomeStringProperty = "totally new value that no caller would ever expect after a call to this method";

  return firstRange.Contains(ID);
}

---

Another possible solution - depending on the actual type of your problem at hand - could be to define an interface / base class and use polymorphism.

example:

internal abstract class A
{
  public void DoSomething(int ID)
  { 
     if(IsInRange(ID))
       DoSomethingProtected(ID);
  }

  protected abstract bool IsInRange(int ID);

  protected abstract void DoSomethingProtected(int ID);
}


internal class B : A
{
  private List<int> firstRange = new List<int> { 42, 23, 5};
  protected override bool IsInRange(int ID)
  {
     return firstRange.Contains(ID); 
  }
  protected override void DoSomethingProtected(int ID)
  {
    Console.WriteLine("{0}", ID);
  } 
}


public class Program
{
  public static void Main(string[] args)
  {
     B foo = new B();
     foo.DoSomething(3);
     foo.DoSomething(42);
  }
}

CAUTION: code written without IDE to hand.

---

Yes. It would be much more readable if you used just a little whitespace. Bunching it up like that makes it hard to tell where things begin and end and makes else if() look like a function call.

if ( checkIfIdInFirstRange() ) {
    //call Range1 handling method
} 
else if ( checkIfIdInSecondRange() ) {
    //call Range2 handling method
}
else {
    //call error handling method
}

Making the extra variable is likely to make code harder to read since you have to define them all before the if/else stack. However, it all depends on the case. Sometimes it might be better to use a variable if you will be using an expensive function many times or if you can make the variable have a more descriptive name than the function.

---

Actually it is also required if you want to test multiple methods and use short-circuit evaluation.

For instance, this is safe:

if (isResourceAvailable() && isResourceValid()) {
    ...
}

while this may no be:

bool resAvailable = isResourceAvailable();
bool resValid = isResourceValid(); // can you call that alone?

if (resAvailable  && resValid ) {
    ...
}

---

It is good style as long as the methods you call don't just do something that would be clearer if it was coded in place:

if ( a > 0 && a < 10 ) doSomething();

is better than

if ( isInRange(a, 0, 10) ) doSomething();

---

Eh, it's mostly up to the coder but declaring the int is a bit more readable.

---

It's OK to write methods in the IF condition statement. But if the method will be used more than one time, you should first use a local variable to store the return value and use the variable as the IF condition

---

You can aim at writing function / method names that will make the code more readable where they are used. Like:

if (idInFirstRange()){
    //call Range1 handling method
}else if(idInSecondRange()){
    //call Range2 handling method
}else{

Also, usual convention for functions returning bool is that they start with is - isIdInFirstRange

Lastly, try avoiding such if-else ( and switch ) ladder. Try to use Dictionaries in such cases. ( https://stackoverflow.com/questions/6506340/if-if-else-or-switch-case/6506403#6506403 )

---

Although this practice is not wrong or condemned by any best practices guidelines, if you have more than one call within if condition it can be a bit difficult to know which call refused to enter if statement during a debug session.

if (idInFirstRange() && idInSecondRange()){
    //call Range handling method
    //if didn't reach this point, who was the responsible (idInFirstRange or idInSecondRange)? 
}else if(idInSecondRange()){
    //call Range2 handling method
}else{
    //call something else
}