1
\$\begingroup\$

Mod note: This question is being discussed on meta

I tried to create a method, that given 3 int numbers and a bool returns true if they are in strict increasing order (by the same amount) like (1, 2, 3), (3, 7, 11) or (0, 4, 8). Except if equalOk is set to true, then equality of two adjacent elements is allowed, such as (5,5,7) or (3,3,3) which should also return true;.

Is there any redundant code in my solution and how could it be done simpler?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace InOrderEqual
{
    class Program
    {
        static void Main(string[] args)
        {
            InOrderEqual(2, 5 , 11, false);  //true  
            InOrderEqual(0, 5, 10, false);  //true
            InOrderEqual(5, 4, 3, true);   //false
            InOrderEqual(5, 5, 7, true);   //true
            InOrderEqual(1, 2, 3, true);   //true
            InOrderEqual(12, 12, 12, false); // false
            InOrderEqual(5, 8, 11, false); // true
            Console.ReadLine();
        }
        public static bool InOrderEqual(int a, int b, int c, bool equalOk)
        {
            bool isEqual = false;
            if (b == a || (a == b && a == c))
            {
                isEqual = true;
                if (equalOk == true)
                {
                    Console.WriteLine(true);
                    return true;
                }
                else Console.WriteLine(false);
                return false;
            }


            else if (( a + a) - b == (b + b) - c|| b - a == c - b && c > b && b > a)
            {
                Console.WriteLine(true);
                return true;
            }

            Console.WriteLine(false);
            return false;
        }
    }
}
\$\endgroup\$

closed as off-topic by 200_success May 12 '15 at 2:48

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ Consider writing this as two different functions. \$\endgroup\$ – Pete Becker May 11 '15 at 14:08
  • 2
    \$\begingroup\$ 2,5,11 should be false. \$\endgroup\$ – IEatBagels May 11 '15 at 18:42
  • 1
    \$\begingroup\$ Unfortunately, it has been determined that this code is not working correctly. Furthermore, the question was so unclear / misunderstood, and that it is unsalvageable, due to the nature of the existing answers. \$\endgroup\$ – 200_success May 12 '15 at 2:55

10 Answers 10

10
\$\begingroup\$

Bug / unexpected behaviour

A call like

InOrderEqual(7, 7, 5, true);   

will unexpectedly return true.


Constructs like

if(condition)
{
    return someValue;
}
else
{
    return someOtherValue;
}  

makes the else redundant. So it is easier to read like

if(condition)
{
    return someValue;
}
return someOtherValue;

The isEqual variable is never used expect for initializing to false and setting one time to true but it is not evaluated. You can just remove it.


The writing to the console shouldn't be done inside the InOrderEqual() because it isn't the responsibility of this method to do any output.


A better way would be

    public static bool InOrderEqual(int a, int b, int c, bool equalOk)
    {
        if (a < b && b < c)
        {
            return true;
        }
        if (!equalOk) 
        {
            return false;
        }

        return ((a==b && a<=c) || (a <= b && b==c));

    }
\$\endgroup\$
  • 1
    \$\begingroup\$ if ( a < b && b < c) does return true even if they are not in strict increasing order. like ( 10, 30, 100 ) would still return true which is not allowed. \$\endgroup\$ – coze May 11 '15 at 10:15
  • \$\begingroup\$ As much as I dislike ternaries, it seems that would be a good place for one. \$\endgroup\$ – RubberDuck May 11 '15 at 11:58
  • 8
    \$\begingroup\$ @coze Those are strictly increasing. I think you mean they're not an arithmetic sequence, which is something different. You should probably edit your question to make that clear. \$\endgroup\$ – Kyle May 11 '15 at 13:26
  • 1
    \$\begingroup\$ The code in this answer, which is checking if the numbers are [strictly] increasing, can be reduced to this one-liner: return (a < b && b < c) || (equalOk && (a==b && a<=c || a<=b && b==c)) \$\endgroup\$ – Mitja May 11 '15 at 13:41
  • 1
    \$\begingroup\$ -1 : overly complex - not needed (Brandon's answer below is much more succinct). \$\endgroup\$ – Ditto May 11 '15 at 15:00
21
\$\begingroup\$

The requirements are that (1) b - a == c - b and that (2) a < b < c, except when equalOk is true, in which case (3) a <= b <= c. If (1) is true, then it is sufficient to just check a < b for (2) and a <= b for (3).

private bool InOrderEqual(int a, int b, int c, bool equalOk) {
    if (b - a != c - b) return false; // (1)
    if (equalOk) return a <= b; // (3)
    return a < b; // (2)
}

Originally, equidistance was not an explicit requirement.

Original answer:

The requirements are that a < b < c, except when equalOk is true, in which case a <= b <= c. There's no need for anything more complex than this:

private bool InOrderEqual(int a, int b, int c, bool equalOk) {
    if (equalOk) return a <= b && b <= c;
    return a < b && b < c;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You could simplify it even further to return equalOk? (a <= b && b <= c) : (a < b && b < c); \$\endgroup\$ – Malvolio May 11 '15 at 21:25
  • 1
    \$\begingroup\$ This doesn't have the same functionality as the OP's code. \$\endgroup\$ – Simon Forsberg May 12 '15 at 0:53
  • \$\begingroup\$ This is incorrect. The requirements are that (b-a) == (c-b) && (b-a) > 0 when equalOk is false. \$\endgroup\$ – Nick Udell May 18 '15 at 15:38
  • \$\begingroup\$ The equidistant requirement was edited into the original question after my answer. We could add the requirement to my code by doing: if ((b - a) != (c - b)) return false; if (equalOk) return a <= b; return a < b; Checking b < c or b <= c would be unnecessary since we've established (a,b) and (b,c) are equidistant. \$\endgroup\$ – Brandon May 21 '15 at 21:16
10
\$\begingroup\$

I don't know if it is for testing purposes but you shouldn't write ouput tot the console in the method itself.


if (equalOk == true)

The comparison is redundant as equalOk is a bool which can only be true or false. Thus, this can be rewritten as:

if (equalOk)

In following code, the else statement is also redundant. If the previous check succeeds, the return will be called and the else will never execute. If it fails, the second return will be called, no need for an else statement.

if (equalOk == true)
{
    return true;
}
else
    return false;

becomes:

if (equalOk == true)
{
    return true;
}
return false;

and can even be shortened to simply this, because you're simply returning a boolean corresponding to the equalOk value:

return equalOk;

Edit:

If I understand your logic, it can be shortened to:

if (c > a && b - a == c - b)
{
    return true;
}
if (allowEqual)
{
    return (a == b && b <= c) || (a <= b && b == c);
}
return false;
\$\endgroup\$
  • \$\begingroup\$ If I'm not mistaken we need to add ` || (a + a)-b == (b + b) - c)` at the first if statement from your edit. If not (2, 5, 11, false/true) would return false which is not intended. \$\endgroup\$ – coze May 11 '15 at 13:03
8
\$\begingroup\$
InOrderEqual()

Methods should express an action or a state, this doesn't do that. How about.. VerifyOrder? Or IsInOrder?


InOrderEqual(2, 5 , 11, false)

I would suggest using a named argument for readability:

InOrderEqual(2, 5 , 11, equalOk: false)

I don't like "ok", it seems indecisive. I'd change that to "equalityAllowed".


You never use isEqual.


Your code is buggy. Try InOrderEqual(5, 5, 3, equalOk: true) and see what it returns.

This is because of this rather senseless statement:

if (b == a || (a == b && a == c))

If b == a and equalOk is set to true, suddenly you assume the third argument is in order as well.


if (equalOk == true)

shortened as

if (equalOk)

Console.WriteLine(true);

Do not print to the console from your domain, leave that to whoever is calling your method. Now you're printing and returning data.


else Console.WriteLine(false);
return false;

Nasty! This might not be doing what you think it is. Translated to proper code, this says

else 
{
    Console.WriteLine(false);
}
return false;

Notice how the return statement falls outside the else block? Always use parenthesis to properly denote a block and avoid confusion/logical errors.


else if (( a + a) - b == (b + b) - c|| b - a == c - b && c > b && b > a)

This could use some commentary behind its approach and some examples. I shouldn't have to parse this entire expression in my head just to know what you're doing. Perhaps split it up in intermediate variables? Properly name the resulting variables and that would help a lot as well.


Your code is not extensible to more (or less) than 3 parameters even though this shouldn't be so hard.

Below is an example how I would do it with 3 hardcoded parameters:

public static bool InOrderEqual(int a, int b, int c, bool equalOk)
{
    bool firstAndSecondInOrder = (a < b || (equalOk && a <= b));
    bool secondAndThirdInOrder = (b < c || (equalOk && b <= c));
    return firstAndSecondInOrder && secondAndThirdInOrder;
}

Or with multiple values:

public static bool InOrderEqual(bool equalOk, params int[] values)
{
    for(var i = 1; i < values.Length; i++)
    {
        if(!(values[i] > values[i - 1] || (values[i] >= values[i -1] && equalOk)))
        {
            return false;
        }
    }
    return true;
}
\$\endgroup\$
  • \$\begingroup\$ If you're trying to implement something for a specific domain, stick with the names of the given field (in this case math). The flag decides whether the comparison is strict (as in strictly increasing set) or not, so should be named accordingly. \$\endgroup\$ – Voo May 11 '15 at 12:33
6
\$\begingroup\$

By the Single Responsibility Principle, the InOrderEqual() function should just return true or false, and not print it too.

There are mechanical simplifications that can be applied:

public static bool InOrderEqual(int a, int b, int c, bool equalOk)
{
    // bool isEqual = false;  // Write-only variable is pointless
    if (b == a)               // Redundant: || (a == b && a == c)
    {
        return equalOK;       // if-else is needlessly complicated
    }
    else
    {                         // if-else is needlessly complicated
        return (a + a) - b == (b + b) - c
                   ||
               b - a == c - b && c > b && b > a;
    }
}

Now, I am highly skeptical that this code works at all. For example, it is clear from the first branch that InOrderEqual(x, x, y, true) is true, regardless of the values of x and y.

\$\endgroup\$
4
\$\begingroup\$

As far a I can see we have two checks that we want to perform a vs. b and b vs. c and we only need to perform the second if we pass the first (short circuit evaluation FTW)

We have two possible tests x < y and x <= y. As the same test is used for each check in a given run we can use a delegate to store it.

This gives us:

private bool InOrderEqual(int a, int b, int c, bool equalOk) {
    var comp = equalOk
                ? (Func<int, int, bool>)((x, y) => x <= y)
                : ((x, y) => x < y);
    return comp(a, b) && comp(b, c);
}
\$\endgroup\$
4
\$\begingroup\$

I find it easier is you get the delta between the 3 numbers and use that for each of your comparison.

int delta1, delta2;

delta1 = b - a;
delta2 = c - b;

// We want increasing
if(delta1 < 0 || delta2 < 0)
    return false;

// Check for equality
if(equalOk && (delta1 == 0 || delta2 == 0))
    return true;

// They are increasing by the same amount
if(delta1 != 0 && delta1 == delta2)
    return true;

return false;
\$\endgroup\$
4
\$\begingroup\$

Your method name is misleading. InOrderEqual, in my opinion, would verify if your inputs are sorted. You should rethink the method's name to consider that there's the same increment between each values.

If equalOK is set to false, you can use this simple boolean expression to validate your numbers :

return a < b && !(a == b || b == c) && c == (b + (b - a))

Using this, you verify that a and b are sorted, and that c respects the increment. All you need to verify is that there is no equality.

if equalOK is set to true,

return a < b && ((a == b || b == c) && b <= c)

You don't need to check anything else, considering that wether you have 5,5,100000, 5,5,6, 3,9,9 or 3,1000,1000, the incremented value doesn't mather since there's only 3 numbers, all you need to check is that b is smaller or equal to c.

Now, let's put this all togheter in a clean way :

public static bool InOrderEqual(int a, int b, int c, bool equalOk)
{       
    //That was to check in both cases
    if(a > b)
    {
        return false;
    }

    //There's an equality.
    if((a == b || b == c) && b <= c)
    {
        return equalOk;
    }

    //We only need to check the increment.
    return c == (b + (b - a));
}
\$\endgroup\$
  • \$\begingroup\$ If a==b or b==c then there's no reason to check for a==b && b==c. You're already inside the if block by then. \$\endgroup\$ – RubberDuck May 12 '15 at 0:09
  • \$\begingroup\$ You're right I didn't figure it out. It bugs at an input 2,2,1,true I think. I'll fix \$\endgroup\$ – IEatBagels May 12 '15 at 0:33
4
\$\begingroup\$

The "simple" idiomatic C# approach (in my opinion)

Instead of restricting your method to work on exactly 3 elements you could think of the 3 elements as a sequence of values, to start with an array of ints.

int[] values = new int[] {1, 2, 3};

Then you have to compare each element to the next one:

public static bool InOrderEqual(int[] values, bool equalOk)
{
    //We use i + 1 < values.Length so accessing i + 1 is always in range.
    for (int i = 0; i + 1 < values.Length; i++)
    {
        if (equalOk)
        {
            /* 
             * value <= nextValue implies that if
             * value > nextValue the condition is not met.
             */
            if (values[i].CompareTo(values[i + 1]) > 0)
            {
                return false;
            }
        }
        else
        {
            /* 
             * value < nextValue implies that if
             * value >= nextValue the condition is not met.
             */
            if (values[i].CompareTo(values[i + 1]) >= 0)
            {
                return false;
            }

        }
    }

    //The condition was met for all elements.
    return true;
}

With the revised specification it would be:

public static bool InOrderEqual(int[] values, bool equalOk)
{
    //All calculations are performed in long to avoid
    //under and overflows.
    long firstDifference = 0;
    //We use i + 1 < values.Length so accessing i + 1 is always in range.
    for (int i = 0; i + 1 < values.Length; i++)
    {
        long prev = values[i];
        long next = values[i + 1];
        long difference = next - prev;

        if (equalOk && difference == 0)
        {
            continue;
        }

        if (firstDifference == 0)
        {
            firstDifference = difference;
        }

        if (firstDifference <= 0 || firstDifference != difference)
        {
            return false;
        }
    }

Now the equalOk looks out of place and makes the method much more complicated so let's remove that and make it two separate methods instead. The revised method isn't that bad but it should still be separated.

public static bool IsAlwaysIncreasing(this int[] values)
{
    //We use i + 1 < values.Length so accessing i + 1 is always in range.
    for (int i = 0; i + 1 < values.Length; i++)
    {
        /* 
         * value < nextValue implies that if
         * value >= nextValue the condition is not met.
         */
        if (values[i].CompareTo(values[i + 1]) >= 0)
        {
            return false;
        }
    }

    //The condition was met for all elements.
    return true;
}

public static bool IsNeverDecreasing(this int[] values)
{
    //We use i + 1 < values.Length so accessing i + 1 is always in range.
    for (int i = 0; i + 1 < values.Length; i++)
    {
        /* 
         * value < nextValue implies that if
         * value >= nextValue the condition is not met.
         */
        if (values[i].CompareTo(values[i + 1]) >= 0)
        {
            return false;
        }
    }

    //The condition was met for all elements.
    return true;
}

public static bool IsAlwaysIncreasingByFixedInterval(this int[] values)
{
    //All calculations are performed in long to avoid
    //under and overflows.
    long firstDifference = 0;
    //We use i + 1 < values.Length so accessing i + 1 is always in range.
    for (int i = 0; i + 1 < values.Length; i++)
    {
        long prev = values[i];
        long next = values[i + 1];
        long difference = next - prev;

        if (firstDifference == 0)
        {
            firstDifference = difference;
        }

        if (firstDifference <= 0 || firstDifference != difference)
        {
            return false;
        }
    }

    return true;
}

public static bool IsAlwaysIncreasingByFixedIntervalOrEqual(this int[] values)
{
    //All calculations are performed in long to avoid
    //under and overflows.
    long firstDifference = 0;
    //We use i + 1 < values.Length so accessing i + 1 is always in range.
    for (int i = 0; i + 1 < values.Length; i++)
    {
        long prev = values[i];
        long next = values[i + 1];
        long difference = next - prev;

        if (difference == 0)
        {
            continue;
        }

        if (firstDifference == 0)
        {
            firstDifference = difference;
        }

        if (firstDifference <= 0 || firstDifference != difference)
        {
            return false;
        }
    }

    return true;
}

I also added the this keyword to the values array so now it can be used as if it was part of the array class (if it's put in a static class):

int[] increasing = new int[] { 1, 2, 3 };
increasing.IsAlwaysIncreasing(); // true
increasing.IsNeverDecreasing(); // true
increasing.IsAlwaysIncreasingByFixedInterval(); // true
increasing.IsAlwaysIncreasingByFixedIntervalOrEqual(); // true

int[] increasingOrEqual = new int[] { 1, 2, 2, 3 };
increasingOrEqual.IsAlwaysIncreasing(); // false
increasingOrEqual.IsNeverDecreasing(); // true
increasingOrEqual.IsAlwaysIncreasingByFixedInterval(); // false
increasingOrEqual.IsAlwaysIncreasingByFixedIntervalOrEqual(); // true

Now this seems an awful lot like LINQ so let's turn up the pace and make this in to a true generic LINQ function:

public static class MyIEnumerableExtensions
{

    public static bool IsAlwaysIncreasingByFixedInterval(this IEnumerable<int> values)
    {
        if (values == null) throw new ArgumentNullException("values");

        //All calculations are performed in long to avoid
        //under and overflows.

        long firstDifference = 0;
        return values.All((prev, next) =>
            {
                long difference = (long)next - prev;

                if (firstDifference == 0)
                {
                    firstDifference = difference;
                }

                return firstDifference > 0 && firstDifference == difference;
            });
    }

    public static bool IsAlwaysIncreasingByFixedIntervalOrEqual(this IEnumerable<int> values)
    {
        if (values == null) throw new ArgumentNullException("values");

        //All calculations are performed in long to avoid
        //under and overflows.

        long firstDifference = 0;
        return values.All((prev, next) =>
        {
            long difference = (long)next - prev;

            if (difference == 0)
            {
                return true;
            }

            if (firstDifference == 0)
            {
                firstDifference = difference;
            }

            return firstDifference > 0 && firstDifference == difference;
        });
    }

    public static bool IsAlwaysIncreasing<TSource>(this IEnumerable<TSource> source)
    {
        if (source == null) throw new ArgumentNullException("source");

        var comparer = Comparer<TSource>.Default;
        return IsAlwaysIncreasing(source, comparer);
    }

    public static bool IsAlwaysIncreasing<TSource>(this IEnumerable<TSource> source, IComparer<TSource> comparer)
    {
        if (source == null) throw new ArgumentNullException("source");
        if (comparer == null) throw new ArgumentNullException("comparer");

        return source.All((prev, next) => comparer.Compare(prev, next) < 0);
    }

    public static bool IsNeverDecreasing<TSource>(this IEnumerable<TSource> source)
    {
        if (source == null) throw new ArgumentNullException("source");

        var comparer = Comparer<TSource>.Default;
        return IsNeverDecreasing(source, comparer);
    }

    public static bool IsNeverDecreasing<TSource>(this IEnumerable<TSource> source, IComparer<TSource> comparer)
    {
        if (source == null) throw new ArgumentNullException("source");
        if (comparer == null) throw new ArgumentNullException("comparer");

        return source.All((prev, next) => comparer.Compare(prev, next) <= 0);
    }

    /// <summary>
    /// Applies the <paramref name="sequencePairPredicate"/> to each pair of value and nextValue in <paramref name="source"/>.
    /// </summary>
    /// <typeparam name="TSource">The type of the elements of <paramref name="source"/></typeparam>
    /// <param name="source">An <see cref="IEnumerable&lt;T&gt;"/> that contains the elements to apply the predicate to.</param>
    /// <param name="sequencePairPredicate">A function to test each sequential pair for a condition.</param>
    /// <exception cref="ArgumentNullException"><paramref name="source"/> or <paramref name="sequencePairPredicate"/> is null.</exception>
    /// <returns>true if every element pair of the source sequence passes the test in the specified predicate, or if the sequence contains less than two elements; otherwise, false.</returns>
    public static bool All<TSource>(this IEnumerable<TSource> source, Func<TSource, TSource, bool> sequencePairPredicate)
    {
        if (source == null) throw new ArgumentNullException("source");
        if (sequencePairPredicate == null) throw new ArgumentNullException("sequencePairPredicate");

        var isFirst = true;
        var previousValue = default(TSource);
        foreach (var value in source)
        {
            if (isFirst)
            {
                previousValue = value;
                isFirst = false;
                continue;
            }

            if (!sequencePairPredicate(previousValue, value))
            {
                return false;
            }

            previousValue = value;
        }

        return true;
    }
}

The compare version can then be used with integers and strings and everything else that is IComparable and the revised version can be used on any sequence of integers:

var isIncreasing = new[] { 1, 2, 4, 8, 16 };
isIncreasing.IsAlwaysIncreasing(); // True
isIncreasing.IsNeverDecreasing(); // True
isIncreasing.IsAlwaysIncreasingByFixedInterval(); // False
isIncreasing.IsAlwaysIncreasingByFixedIntervalOrEqual(); // False

var isNeverDecreasing = new[] { 1, 1, 2, 3, 5 };
isNeverDecreasing.IsAlwaysIncreasing(); // False
isNeverDecreasing.IsNeverDecreasing(); // True
isNeverDecreasing.IsAlwaysIncreasingByFixedInterval(); // False
isNeverDecreasing.IsAlwaysIncreasingByFixedIntervalOrEqual(); // False

var isIncreasingByFixedInterval = new[] { 1, 2, 3 };
isIncreasingByFixedInterval.IsAlwaysIncreasingByFixedInterval()); // True
isIncreasingByFixedInterval.IsAlwaysIncreasingByFixedIntervalOrEqual()); // True

var isIncreasingByFixedIntervalOrEqual = new[] { 1, 2, 2, 3 };
isIncreasingByFixedIntervalOrEqual.IsAlwaysIncreasingByFixedInterval()); // False
isIncreasingByFixedIntervalOrEqual.IsAlwaysIncreasingByFixedIntervalOrEqual()); // True

var andWithStrings = new[] { "a", "b", "c", "D", "e" };
// false becayse D > e and Ordinal is case sensitive
andWithStrings.IsAlwaysIncreasing(StringComparer.Ordinal);
// true because we ignore case
andWithStrings.IsAlwaysIncreasing(StringComparer.CurrentCultureIgnoreCase);
// true, same as above, StringComparer.CurrentCultureIgnoreCase is default for strings
andWithStrings.IsAlwaysIncreasing();
\$\endgroup\$
  • \$\begingroup\$ I think you and I have different definitions of what is "simple". And this code does not return the same result as the OP's original code. It seems like 1, 4, 5 should return false \$\endgroup\$ – Simon Forsberg May 12 '15 at 0:55
  • \$\begingroup\$ It matched the specification in the question until the edit 7 hours ago. \$\endgroup\$ – Johnbot May 12 '15 at 5:43
  • \$\begingroup\$ yes, but it never matched the actual code implementation. (Yours is not the only answer that did this) \$\endgroup\$ – Simon Forsberg May 12 '15 at 10:19
  • \$\begingroup\$ I updated it to solve the updated question with the same approach. \$\endgroup\$ – Johnbot May 12 '15 at 12:29
2
\$\begingroup\$

As others have said, this is a simple function that shouldn't have embedded printouts. Given the requirement that the numbers must be an equal distance apart, I would suggest:

public static bool InOrderEqual(int a, int b, int c, bool equalOK)
{
    return b - a == c - b                // equal distances
        && b - a >= (equalOK ? 0 : 1);   // numbers are increasing
}

This doesn't require a lot of logic. The key is to boil it down to two checks: that the differences are equal, and that they are non-negative. The latter check can incorporate equalOK with a quick ternary operation, which hopefully isn't too much complexity for one line.

Using @Voo's suggestion, it might be better to rename equalOK to strict (and reverse it so strict == true means equal is not okay).

public static bool InOrderEqual(int a, int b, int c, bool strict)
{
    return b - a == c - b                // equal distances
        && b - a >= (strict ? 1 : 0);    // numbers are increasing
}
\$\endgroup\$

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