Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following logic:

bool a;
bool b;
int v = 0;

//code here to set the values of a and b...

if(b)
{
    v = a ? 1 : 2;
}
else
{
     v = a ? 0 : 1;
}

I believe that there may be a way to minimize this logic (or rewrite it in a more clever way) that I am not thinking of.

share|improve this question
1  
I find this question too abstract; it can't be answered in a meaningful way. What do a, b, and v represent? The clever way is often the wrong way. –  codesparkle Feb 3 '13 at 9:04
1  
this example is probably just something he typed up that mimics his real code. I've had to do the same thing when I'm working on proprietary code and need help trying to make my logic better. if i could post my code I would, but there are a few occasions where I can't. –  Robert Snyder Feb 3 '13 at 21:14
    
Also on that note I would like to inject a personal preference of mine. If you do have code that affects the return value, I find it easier to set the value when I do checks for things. This way I can always highlight that code and click Refactor my IDE will (usually) put something like v=CheckForSomeVariable() and that will make my code easier to read. That is personal preference though. –  Robert Snyder Feb 3 '13 at 21:26
add comment

3 Answers

up vote 2 down vote accepted

You can also avoid the else blocks entirely by realizing that !b=0, b=1 and a=0, !a=1, like:

bool a;
bool b;
int v = 0;  //a && !b (0 + 0 = 0)

//code here to set the values of a and b...

if (b) 
    v++;    //a && b  (0 + 1 = 1)
if (! a)
    v++;    //!a && b (1 + 1 = 2), !a && !b (1 + 0 = 1)
share|improve this answer
add comment
int v = b ? (a ? 1 : 2) : (a ? 0 : 1);

EDIT #1:

As per svick, parsed out conditional operator on separate lines:

int v = b
    ? (a ? 1 : 2)
    : (a ? 0 : 1);

EDIT #2:

Here's one I don't like, but it has no branching whatsoever:

int v = Convert.ToInt32(b) + Convert.ToInt32(!a);
share|improve this answer
1  
I want to avoid ternanary inside ternary because it seems unreadable at least to me –  Rhs Jan 31 '13 at 20:11
    
You may wish to update your question to state that fact then. –  Jesse C. Slicer Jan 31 '13 at 20:12
    
It might help if you put each of the nested ternaries on its own line. –  svick Jan 31 '13 at 20:20
add comment

This post reminds me of part of a "how to write clean code" and one of the challenges that the speaker proposed was to not use if statements on behavioral code. Obviously if statements on logical code is only (for lack of a better word) logical. So what this implies is that you would use polymorphic classes to give you the desired output. so as a super simple example i'll use the standard math example.

consider this code

public int SomeFunction()
{
    int a=3;
    int b=4;

    int c;
    //some logical operations
    //that gives a value to c

    switch(c)
    {
        case 1:
            return a+b;
        case 2:
            return a-b;
        case 3:
            return a*b;
        case 4:
            return a^b;
        default:
            return 0;
    }
}

as you can see from this code it does 2 things. Which doing more than one thing breaks the single responsibility principle. And although this is a simple example readability is not that impaired. So a suggested way to fix this is to use a factory for the logic, and polymorphism for the operations. This would work out good because then everything can stick with the SRP, and makes debugging a little bit easier. Keep in mind because of how simple this example is it may feel like it is too complicated of a process, but a principle is simple idea that you can build off of. So lets build this. First our polymorphic class, we'll make a parent class called MathOperator. it has a constructor that takes 2 numbers, and has a abstract method Operate that returns a number. Next we make a MathFactory. This class has nothing but static methods to get what we desire. One of the methods is GetProperMathOperator(int). It would replace the switch statement in the previous example with var mathOperation = MathFactory.GetProperMathOperator(c); then we can simply just return mathOperation.Operate(); so our code would look something like this

public int SomeFunction()
{
    int c;
    //some logical operations
    //that gives a value to C

    var mathOperation = MathFactory.GetProperMathOperator(c);
    mathOperation.SetValues(3,4);

    return mathOperation.Operate();
}

but for testing purposes that code is a little difficult because this class depends on 2 other classes. So it would be better to take that code out and move it to the calling class. So to make it easier to test this function we would want to modify the method to ask for the proper MathOperator. Now you just set the values to what you need, and return the operatored number.

public int SomeFunction(MathOperator mathOperation)
{
    mathOperation.SetValues(3,4);
    return mathOperation.Operate();
}

So does this answer the OP's question? not directly. But since his code looks more like it is an abbreviated example of what his code really does, then I would think that he could use this principle to refactor his code into something that is a little cleaner, and would make the function easier to test and debug.

share|improve this answer
    
I'd upvote this if it didn't introduce a temporal coupling. It's just too easy to forget calling SetValues. Why not pass the values as parameters to Operate directly? –  codesparkle Feb 3 '13 at 9:00
    
because in the OP's post he mentioned that there was some code in his method that ended up setting the value of a and b. Even though they were bool values it still affected what the returned value is. so the SetValues portion was to reflect that. Another reason for it was that if a person was doing TDD then they would know how that function is supposed to operate, and what it was supposed to return based on the tests. –  Robert Snyder Feb 3 '13 at 19:56
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.