How do I refactor a if else in a method with many repeated boolean value?

[tangara goh]

Hi guys,

I am back to another refactoring which I really need some serious help because my boss thinks I am very bad in my programming and also the job duration is very short and I need to look for another job again

The code structure goes like this :

Sorry this is C# code again so the syntax may be off.... and I know NUTS about C# ....but what to do I really need the money....which is better than being a cashier....

boolean MatchedTarget(String TargetName, String Text){
if (!(Another Boolean method)))))
{
return false;
}
else if (Boolean method) && (!Text.Equals(TargetName)
{
return false;
}
else if (Boolean method) && !(Text.Trim().Equals(TargetName).Trim())
{
return false;
}
else if (Boolean method) && (Text.Euqls(TargetName.IgnoreCase))
{
return true;
}
else if (Boolean method) && (Text.Euqls(TargetName.ToLowerCase()))
{
return true;
}



and the list goes on ..

How do I make the code more effective since there is so many conditions combined with this Boolean method that is true and ! Boolean method?

Thanks.

[Campbell Ritchie]

Can you reduce the multiple Boolean expressions to one expession? Can you look for methods in the String class?

...
return myMethod() && myText != null && myText.strip().equalsIgnoreCase("Campbell");
...

The String#strip() method is probably better than trim() but it only works in Java11+. I challenge you to find a reliable way to avoid the null test.

[Campbell Ritchie]

A few minuts ago, I wrote:. . .  it only works in Java11+. . . .

Then I noticed you said C#. Sorry; that won't work full stop. But the concept will.

[tangara goh]

Campbell Ritchie wrote:Can you reduce the multiple Boolean expressions to one expession? Can you look for methods in the String class?

...
return myMethod() && myText != null && myText.strip().equalsIgnoreCase("Campbell");
...

The String#strip() method is probably better than trim() but it only works in Java11+. I challenge you to find a reliable way to avoid the null test.

I will try that Thanks Campell.

Can I know this if else structure mus it ends with a else as the last statement, given the above code that I have provided because there is a lot of true and false that is mixed with this !boolean method and boolean method == true and again follows by alot of true and false in this boolean method is true.

I feel that I am totally lost!

Please help me on this.

Thanks again.

[Carey Brown]

This isn't anywhere near real code. You have unbalanced parens. And, I would think Equals() would return a boolean, so what does Equals().Trim() do?

[tangara goh]

Carey Brown wrote:This isn't anywhere near real code. You have unbalanced parens. And, I would think Equals() would return a boolean, so what does Equals().Trim() do?

This is the c# portion in which I replaced it with .trim Java syntax.  Cos C# one is troublesome - still need to do some replaceOf method ..

I am more concerned how do I go about refactoring the code by putting the ! Boolean with the conditions that return true and false and another Boolean method with different conditions that return true and false.

I hope to get some some guidance about this part.

Tks.

[Carey Brown]

tangara goh wrote:

Carey Brown wrote:This isn't anywhere near real code. You have unbalanced parens. And, I would think Equals() would return a boolean, so what does Equals().Trim() do?

This is the c# portion in which I replaced it with .trim Java syntax.  Cos C# one is troublesome - still need to do some replaceOf method ..

I am more concerned how do I go about refactoring the code by putting the ! Boolean with the conditions that return true and false and another Boolean method with different conditions that return true and false.

I hope to get some some guidance about this part.

Tks.

In Java you can't trim() a boolean, so what is this doing? Or is it bogus code?

[Campbell Ritchie]

tangara goh wrote:. . . I hope to get some some guidance about this part. . . .

I have already given you some guidance. You simply have to translate it into C#.

[Tim Holloway]

Actually "else" is redundant if the result is a return statement.

But it looks like the coarse structure is something like this:

boolean MatchedTarget(String TargetName, String Text){
  if (!(Another Boolean method))))) {
    return false;
  }

 if (Boolean method) {
    if (!Text.Equals(TargetName) {
      return false;
     }
    if (!(Text.Trim().Equals(TargetName).Trim()) {
       return false;
    }
    if  (Text.Equals(TargetName.IgnoreCase)) {
       return true;
    }
    if ((Text.Equals(TargetName.ToLowerCase())) {
       return true;
   }
}
// else
   return ????;

I prefer a more compact arrangement of braces, myself, but that's personal preference. Note that I only ask a question once. I haven't looked at the finer-grained tests, although I suspect that this is checking names the hard way. Probably you could collapse it way down by creating a comparison value that's pre-trimmed and normalized to just lower case, but I haven't looked that close.

And of course, you can compact a statement set like:

    if (!Text.Equals(TargetName) {
      return false;
     }
    if (!(Text.Trim().Equals(TargetName).Trim()) {
       return false;
    }

into

    if (!Text.Equals(TargetName)
      && (!(Text.Trim().Equals(TargetName).Trim()) {
       return false;
    }

Although now that I look at it, doesn't the trimmed test also satisfy the un-trimmed test?

I highly recommend studying symbolic logic. When you're dealing with things like IF NOT A AND NOT B, there's this thing called "DeMorgan's Theorem".

[fred rosenberger]

I haven't looked too closely, but couldn't you use some nested if statements?

boolean MatchedTarget(String TargetName, String Text){
if (!(Another Boolean method)))))
{
return false;
}
else if (Boolean method)
{  
    if (!Text.Equals(TargetName) ||!(Text.Trim().Equals(TargetName).Trim()) || (Text.Euqls(TargetName.IgnoreCase)) ||(Text.Euqls(TargetName.ToLowerCase()))
    {
            return true;
    }
}

even further, couldn't you "normalize" you text and TargetName outside your loop?  
normalizedText = text.Trim().toLowerCase()
normalizedTarge = TargetName.trim().toLowerCase();

and then just compare them once?  Note: i am sure I don't have that syntax exactly right...just trying to illustrate the idea.

[Campbell Ritchie]

tangara goh wrote:. . . Can I know this if else structure mus it ends with a else . . . .

I don't think you need an if‑else in the first place. I haven't seen anything to suggest that my technique can't be adapted to your requirements.
If each if contains return xyz; then you don't need an else at the end. You can get away with a plain simple return. But, as you will see in the old Sun Style Guide, it isn't good style to write

if (xyz)
{
   return true;
}
else
{
   return false;
}

Not even if you can abbreviate it to

if (xyz)
{
   return true;
}
return false;

This is what you want to write:-

return xyz;

[Carey Brown]

public class BooleanRefactor
{
	boolean Original( boolean A, boolean B, boolean C, boolean D,
			boolean E, boolean F )
	{
		if( !A )
		{
			return false;
		}
		else if( B && !C )
		{
			return false;
		}
		else if( B && !D )
		{
			return false;
		}
		else if( B && E )
		{
			return true;
		}
		else if( B && F )
		{
			return true;
		}
		return false; // implied that the series must end in 'false' (?)
	}
	
	boolean Refactored( boolean A, boolean B, boolean C, boolean D,
			boolean E, boolean F )
	{
		return A && B && C && D && (E || F);
	}
}

[Tim Holloway]

Or, just to combine everyone's suggestion (assuming that all those minor tests were to see if a space-trimmed name matched in a case-independent way)

/**
 * Check for whether we need to compare target and string
 * and return results.
 * @param TargetName The target name to compare against
 * @param Text The text to compare
 * @return <b>false</b> unconditionally if "anotherboolean",
 * otherwise if booleanMethod, return results of comparing target to text.
 * otherwise return ???
 */
boolean MatchedTarget(String TargetName, String Text) {
  if (!(Another Boolean method)))))
  {
    return false;
  }

  if (Boolean method)
  {  
     if ( TargetName.IgnoreCase ) {
          //  case-insensitive
          return Text.Equals(TargetName).Trim().ToLowerCase()
                 .Equals  (Text.Trim().ToLowerCase());
     } else {
          // case-sensitive
          return Text.Equals(TargetName).Trim()
                .Equals (Text.Trim());
     }
  }

  // else:
  return ???
}

I just realized that this is supposed to be C#, not Java, but Doxygen can process JavaDoc-style comments, so I'll leave them.

[Carey Brown]

Tim Holloway wrote:Or, just to combine everyone's suggestion (assuming that all those minor tests were to see if a space-trimmed name matched in a case-independent way)

If there's a method to the madness of the string comparisons I don't see it, but if it's there it would probably be best to be isolated to its own method.

[Junilu Lacar]

I feel like a broken record but one thing I see that's missing from this picture is any clarity in what exactly this code actually does. To me, you don't really gain much from re-structuring the if-else statements OP has given. Until you know what each boolean expression actual means you still haven't attained one of the main goals of refactoring which is to make the code easy to understand.

@OP: by giving hypothetical code that contains no semantics (meaning), there's really not a whole lot people can suggest other than hypothetical reorganization and simplification of the structure of the code. This is probably the least bang you can get for any effort you spend in refactoring.

Give us actual code to work with to which we can attach some meaningful semantics. That will be much more useful and fruitful, in my opinion.

[tangara goh]

Campbell Ritchie wrote:

tangara goh wrote:. . . Can I know this if else structure mus it ends with a else . . . .

I don't think you need an if‑else in the first place. I haven't seen anything to suggest that my technique can't be adapted to your requirements.
If each if contains return xyz; then you don't need an else at the end. You can get away with a plain simple return. But, as you will see in the old Sun Style Guide, it isn't good style to write

if (xyz)
{
   return true;
}
else
{
   return false;
}

Not even if you can abbreviate it to

if (xyz)
{
   return true;
}
return false;

This is what you want to write:-

return xyz;

What if I put all the true first before I put all the false and returning the false ? Will it be ok ?

I did a refactoring last night and it looks like this :

boolean MatchedTarget(string TargetName, Paragragh paragraph, string text)
{
if (Boolean Method) && conditions || conditions
{
return true;
}
if (! Boolean Method) && conditions || conditions
{
return false
}
else return !Boolean method
}

But, I told my boss I would upload my code to the git so I guess if he is only to mock at it ...let it be...I need to look for another job for sure ...can't wait till the day I pack and go while other temp guys still staying around for more months...

I'lll come back here and learn.

Thank you guys.

[Campbell Ritchie]

tangara goh wrote: . . . What if I put all the true first before I put all the false and returning the false ? Will it be ok ?

Yes, if the semantics of the “after” formula is the same as the “before” formula. You haven't actually told us what the formulae are, so I can't tell you. Do you know any Boolean algebra?

I did a refactoring last night and it looks like this . . .

I am afraid I still don't like that sort of code.

Thank you guys.

That's a pleasure

[Campbell Ritchie]

fred rosenberger wrote:. . . nested if statements?

Maybe yes, but there is no need after if (xyz) return true; Because such code transfers control back to its calling method, the else is unnecessary. If we overlook the missing code path which might fail to reach a return..., this sort of code

boolean MatchedTarget(String TargetName, String Text){
if (!(Another Boolean method)))))
{
    return false;
}
else if (Boolean method)
{  
    if (!Text.Equals(TargetName) ||!(Text.Trim().Equals(TargetName).Trim()) ||
            (Text.Euqls(TargetName.IgnoreCase)) ||(Text.Euqls(TargetName.ToLowerCase()))
    {
        return true;
    }
}

becomes

boolean MatchedTarget(String TargetName, String Text){
if (!(Another Boolean method)))))
    {
        return false;
    }  
    if (!Text.Equals(TargetName) ||!(Text.Trim().Equals(TargetName).Trim()) ||
                (Text.Euqls(TargetName.IgnoreCase)) ||(Text.Euqls(TargetName.ToLowerCase()))
    {
        return true;
    }
}

You can sort out the missing return problem by getting rid of the second if:-

boolean MatchedTarget(String TargetName, String Text){
    if (!(Another Boolean method)))))
    {
        return false;
    }  
    return (!Text.Equals(TargetName) ||!(Text.Trim().Equals(TargetName).Trim()) ||
                (Text.Euqls(TargetName.IgnoreCase)) ||(Text.Euqls(TargetName.ToLowerCase()));
}

You can get rid of the first if by cancelling out its double negative:-

boolean MatchedTarget(String TargetName, String Text){
    return (Another Boolean method) && (!Text.Equals(TargetName) ||!(Text.Trim().Equals(TargetName).Trim()) ||
                (Text.Euqls(TargetName.IgnoreCase)) ||(Text.Euqls(TargetName.ToLowerCase()));
}

I think a few excess bang signs have crept in there But you can simplify the multiple equals calls into one and go back to my first suggestion.

[Tim Holloway]

Constructs in the form:[code]
if ( x and/or ... ) {
  return truefalse;
}
else if ( ! x andor ... {
  return truefalse;
}
are undesirable for a number of reasons. First, as we've said, the "else" isn't required when the "if" unconditionally returns, so it's just clutter.

More seriously, asking the same question twice (if x and if ! x) is a great way to introduce bugs. Especially when the code it later maintained. Always try to ask a question only once.

[tangara goh]

Yes, Tim,
In fact, now I am being driven to insanity because of this if x and not !x thing.  Actually, if it is !x, then there is no need to check further...but how do I make it happen in a boolean method ?

[Carey Brown]

	boolean Refactored( boolean A, boolean B, boolean C, boolean D,
			boolean E, boolean F )
	{
		return A && B && C && D && (E || F);
	}
	
	boolean booleanMethod() { return true; }
	boolean anotherBooleanMethod() { return true; }
	
	boolean MatchedTarget( String targetName, String text )
	{
		return anotherBooleanMethod()
				&& booleanMethod()
				&& text.equals( targetName )
				&& text.trim().equals( targetName )
				&& ( text.equalsIgnoreCase( targetName )
					|| text.equals( targetName.toLowerCase() ) // useless test, test above covers this
				);
	}

EDIT:
Now that's it's written cleaner here the intent has become lost because a few  of the tests overlap and contradict each other. Now, where are the original requirements for this code?

[Campbell Ritchie]

tangara goh wrote:. . . how do I make it happen in a boolean method ?

Start by reading all the replies you have got, especially my first reply.

[tangara goh]

Campbell Ritchie wrote:

fred rosenberger wrote:. . . nested if statements?

Maybe yes, but there is no need after if (xyz) return true; . . .
But you can simplify the multiple equals calls into one and go back to my first suggestion.

what if there is a null inside which I need to handle ? I can't possibly use exceptions handling right ? or should I use try catch if say the text is NULL ?

[Campbell Ritchie]

NULL is a predefined constant in C/C++; you mean null, surely.
Did you understand my first post in this discussion? You quoted it, but I don't get the impression you really understood it.

[Tim Holloway]

Campbell Ritchie wrote:NULL is a predefined constant in C/C++; you mean null, surely.
Did you understand my first post in this discussion? You quoted it, but I don't get the impression you really understood it.

Actually, NULL is a macro defined (if memory serves) in stdlib.h and more than once for various reasons I've had to define it manually. "null" is improper usage as C tradition is that manifest constants should have names that are all uppercase. Or at least the original kind done via pre-processor before "const" became part of the language.

[tangara goh]

Tim Holloway wrote:

Campbell Ritchie wrote:NULL is a predefined constant in C/C++; you mean null, surely.
Did you understand my first post in this discussion? You quoted it, but I don't get the impression you really understood it.

Actually, NULL is a macro defined (if memory serves) in stdlib.h and more than once for various reasons I've had to define it manually. "null" is improper usage as C tradition is that manifest constants should have names that are all uppercase. Or at least the original kind done via pre-processor before "const" became part of the language.

Yap.  Thank you guys for your help.

I think I finally resolve all the errors...but it just made me feel if I am suitable to do coding at all
I mean I really spent lots of effort on the problem which I think any experienced person can do it within 10minutes ?
And really my foundation is just so shaky.... cos this is really quite fundamental in any cs ...
I feel that my future is very very bleak....

[Campbell Ritchie]

Please show us your final solution.

[tangara goh]

Campbell Ritchie wrote:Please show us your final solution.

I'd love to but right right I just realised I didn't finish the code at all after I was pointed out that I missed out certain cases

And really I am so careless...really not cut out to do coding

The thing is I have problem in executing a case where I need to find the TargetName at the end of a sentence but I can't use .Contains because if I do I will make other cases failed.

So far, I tried to make use of substring like this

if (boolean method x == true)
&&  TargetName.Substring(text.Length - TargetName.Length, text.Length)).Equals(TargetName)))
{
return false
}

But, it is not working....

Really, I have been working on this for the past few hours and I am not getting it ...I think it is really demoralising...

[Campbell Ritchie]

Don't use == true or similar, which is poor style and rror‑prone because you might write = true or similar by mistake.
What is the logic behind the use of substrings? It doesn't look right to me. And where does return false; in line 4 come from?
Stop messing around with code. You are simply tying yourself in knots. Write down what you require on paper, with your computer turned off.

[tangara goh]

Campbell Ritchie wrote:Don't use == true or similar, which is poor style and rror‑prone because you might write = true or similar by mistake.
What is the logic behind the use of substrings? It doesn't look right to me. And where does return false; in line 4 come from?
Stop messing around with code. You are simply tying yourself in knots. Write down what you require on paper, with your computer turned off.

Hi Campbell,

Sorry for the late reply as I was fighting fire at my end..

I didn't use == true in my actual code...It is used here cos I thought it was more clear to you guys.

I am back here yet with another question which I am curious to know :
The thing is that there are new added conditions which makes the return type false and there isn't a better way to do it than like this below:

bool MatchedTarget(String TargetName, String text)
{
if (!(Another Boolean method)))))
{
return false;
}
else if (Boolean method) && (text.Equals(TargetName)
{
return true;
}else if (!text.Equals(TargetName)
{
return false;
}
return true;
}

My question is the end return true will refer to which condition ?  Will it refer to

 if (Boolean method) && (text.Equals(TargetName)

I hope you can let me know how things work here cos I am quite confused about this part.

Thank you guys once again for your help.

[Campbell Ritchie]

tangara goh wrote:. . . I didn't use == true . . . I thought it was more clear to you guys.

But we believe people when they show us their code. We see people who really do write == true all the time.

. . . the return type false

But false isn't a return type; it is result and the return type is bool/boolean/Boolean.

and there isn't a better way to do it than like this below: . . . My question is the end return true will refer to which condition ?  Will it refer to

 if (Boolean method) && (text.Equals(TargetName)

. . .

I really feel as if I were going round in circles here You seem to be repeating yourself again. Or are you repeatedly testing the same thing? The multiple returns where you don't seem to know what you want are dreadfully error‑prone. Have you analysed that code or done a truth table for it? You still haven't shown me anything better than

return otherMethod() &&
       text != null  &&
       text.Trim().Equals(TargetName);

I am trying to remember where I first saw that sort of code.
Did you ever work out how you could get rid of that test for nullity? Do you still need Trim?

Thank you . . .

That's a pleasure