4
\$\begingroup\$

I've had a go at the following challenge (from codewars):

Interesting car mileages are 3-or-more digit numbers that meet one or more of the following criteria:

  • Any digit followed by all zeros: 100, 90000
  • Every digit is the same number: 1111
  • The digits are sequential, incementing†: 1234
  • The digits are sequential, decrementing‡: 4321
  • The digits are a palindrome: 1221 or 73837
  • The digits match one of the values in the awesomePhrases array

† For incrementing sequences, 0 should come after 9, and not before 1, as in 7890. ‡ For decrementing sequences, 0 should come after 1, and not before 9, as in 3210.

Complete the method IsInteresting, so that it returns:

  • 2 if a mileage is interesting
  • 1 if an interesting mileage occurs within the next 2 miles
  • 0 for any other number

Mileages will be in the range 0 to 1,000,000,000. awesomePhrases will always be supplied, but may be empty.

In order to support development, I wrote the following tests (apart from ShouldWorkTest, which was supplied with the challenge).

using NUnit.Framework;
using System.Collections.Generic;

[TestFixture, Category("MileageChallenge")]
public class CarMileageTests
{
    public const int NotInteresting = 0;
    public const int AlmostInteresting = 1;
    public const int Interesting = 2;

    [TestCase(0)]
    [TestCase(97)]
    //[TestCase(98)]  // Almost Interesting
    //[TestCase(99)]  // Almost Interesting
    public void NumbersBelow100AreNotInteresting(int mileage)
    {
        Assert.AreEqual(NotInteresting, CarMileage.IsInteresting(mileage, new List<int> { }));
    }

    [TestCase(100)]
    [TestCase(7000)]
    [TestCase(900000000)]
    public void NumbersFollowedByOnlyZeroAreInteresting(int mileage)
    {
        Assert.AreEqual(Interesting, CarMileage.IsInteresting(mileage, new List<int> { }));
    }

    [TestCase(102)]
    [TestCase(7700)]
    [TestCase(900700000)]
    public void NumbersIntermingledWithZerosAreNotInteresting(int mileage)
    {
        Assert.AreEqual(NotInteresting, CarMileage.IsInteresting(mileage, new List<int> { }));
    }

    [TestCase(101)]
    [TestCase(7700)]
    [TestCase(900700000)]
    public void NumbersMatchingAwesomeListAreIntersting(int mileage)
    {
        Assert.AreEqual(Interesting, CarMileage.IsInteresting(mileage, new List<int> {600, 101, 900700000, 5555, 7700, 7777 }));
    }


    [TestCase(67576)]
    [TestCase(7117)]
    [TestCase(900000009)]
    public void PalendromesAreInteresting(int mileage)
    {
        Assert.AreEqual(Interesting, CarMileage.IsInteresting(mileage, new List<int> { }));
    }

    [TestCase(34567890)]
    [TestCase(7890)]
    [TestCase(12345)]
    public void IncrementingSequencesAreInteresting(int mileage)
    {
        Assert.AreEqual(Interesting, CarMileage.IsInteresting(mileage, new List<int> { }));
    }

    [TestCase(543210)]
    [TestCase(9876)]
    [TestCase(54321)]
    public void DecrementingSequencesAreInteresting(int mileage)
    {
        Assert.AreEqual(Interesting, CarMileage.IsInteresting(mileage, new List<int> { }));
    }

    // Strictly speaking, special case of palendrome
    [TestCase(4444)]
    [TestCase(99999999)]
    public void AllSameDigitsShouldBeInteresting(int mileage)
    {
        Assert.AreEqual(Interesting, CarMileage.IsInteresting(mileage, new List<int> { }));
    }

    [TestCase(100)]
    [TestCase(900700000)]
    [TestCase(67576)]
    [TestCase(987654)]
    [TestCase(123456)]
    public void CloseMatchesAreAlmostInteresting(int interestingMileage)
    {
        Assert.AreEqual(AlmostInteresting, CarMileage.IsInteresting(interestingMileage - 1, new List<int> { 900700000 }));
        Assert.AreEqual(AlmostInteresting, CarMileage.IsInteresting(interestingMileage - 2, new List<int> { 900700000 }));
    }

    // Supplied by challenge
    [Test]
    public void ShouldWorkTest()
    {
        Assert.AreEqual(0, CarMileage.IsInteresting(3, new List<int>() { 1337, 256 }));
        Assert.AreEqual(1, CarMileage.IsInteresting(1336, new List<int>() { 1337, 256 }));
        Assert.AreEqual(2, CarMileage.IsInteresting(1337, new List<int>() { 1337, 256 }));
        Assert.AreEqual(0, CarMileage.IsInteresting(11208, new List<int>() { 1337, 256 }));
        Assert.AreEqual(1, CarMileage.IsInteresting(11209, new List<int>() { 1337, 256 }));
        Assert.AreEqual(2, CarMileage.IsInteresting(11211, new List<int>() { 1337, 256 }));
    }
}

My solution to the challenge:

using System.Collections.Generic;

public class CarMileage
{

    public static int IsInteresting(int mileage, List<int> awesomePhrases)
    {
        const int NotInteresting = 0;
        const int AlmostInteresting = 1;
        const int Interesting = 2;

        if (IsExactMileageInteresting(mileage, awesomePhrases))
        {
            return Interesting;
        }

        if (   IsExactMileageInteresting(mileage + 1, awesomePhrases) 
            || IsExactMileageInteresting(mileage + 2, awesomePhrases)) 
        {
            return AlmostInteresting;
        }

        return NotInteresting;
    }

    private static bool IsExactMileageInteresting(int mileage, List<int> awesomePhrases)
    {
        if (mileage < 100) return false;

        if (IsSingleDigitAndZeros(mileage)) return true;
        if (IsAwesomePhrase(mileage, awesomePhrases)) return true;

        var mileageString = mileage.ToString();

        if (IsPalendrome(mileageString)) return true;
        if (IsSequence(mileageString, 1)) return true;
        if (IsSequence(mileageString, -1)) return true;

        return false;
    }

    private static bool IsSingleDigitAndZeros(int mileage)
    {
        while(mileage >= 10)
        {
            if (mileage % 10 != 0) return false;
            mileage /= 10;
        }
        return true;
    }

    private static bool IsAwesomePhrase(int mileage, List<int> awesomePhrases)
    {
        return awesomePhrases.Contains(mileage);
    }

    private static bool IsPalendrome(string mileageString)
    {
        int start = 0, end = mileageString.Length-1;

        while(start < end)
        {
            if(mileageString[start] != mileageString[end])
            {
                return false;
            }
            start++;
            end--;
        }

        return true;
    }

    private static bool IsSequence(string mileageString, int increment)
    {
        var previousNumeral = mileageString[0];

        for(var index = 1; index < mileageString.Length; index++)
        {
            if (previousNumeral + increment == mileageString[index]) {
                previousNumeral = mileageString[index];
                continue;
            }
            if(mileageString[index] == '0' &&
              (previousNumeral == '9' || previousNumeral == '1'))
            {
                previousNumeral = mileageString[index];
                continue;
            }
            return false;
        }
        return true;
    }
}

Feedback on any aspect of the tests/solution is welcome.

\$\endgroup\$
5
  • \$\begingroup\$ Tests should have error messages. Otherwise test fail error output is too general (e.g. "was false, expected true"). Messages make tests more readable (quickly comprehensible) and go to self documentation. \$\endgroup\$
    – radarbob
    Oct 3, 2016 at 15:22
  • \$\begingroup\$ "Trust but verify" - President Ronald Regan. Check for for null List<int> in IsInteresting() \$\endgroup\$
    – radarbob
    Oct 3, 2016 at 15:28
  • \$\begingroup\$ I don't see a test for an empty awesomePhrases \$\endgroup\$
    – radarbob
    Oct 3, 2016 at 15:30
  • 1
    \$\begingroup\$ @radarbob, selfexplaining test name is better then error message. With good test method name you have no needs to look at output at all \$\endgroup\$
    – Fabio
    Nov 17, 2016 at 9:23
  • \$\begingroup\$ "selfexplaining test name is better then [sic] error message" is a spurious argument. They are not the same thing, They have different purposes. A good test name does not identify the failed data. My point is brought home in this context: many thousands of tests that I did not write testing code I did not write. Pages and pages of output. Tests that get asserted in a general method (ShouldWorkTest is its inverse, with the same ambiguity) yet has multiple test inputs and at times specific data's context is built up through more-than-trivial test setup. \$\endgroup\$
    – radarbob
    Nov 17, 2016 at 16:57

3 Answers 3

2
\$\begingroup\$

One thing I'd change would be to have tests for each of the individual methods. Notice how the test has to provide an empty list to test IsPalindrome() or IsSequence()? Palindromes have nothing to do with awesome phrase arrays, yet here they are, adding confusion to the tests.

Isolating the tests will also allow you to test uninteresting hits. Is 101 a hit because it's Awesome, or because it's a palindrome (of course does it really matter, when palindromes are clearly awesome anyway?) But what about 102?

IsSequence() might benefit more from isolated tests, because the intent of the ==9 or ==1 check isn't clear. 7890 is a sequence, but what about 7899? 7891? 78901? What about 890123, or 210987?

I see no error handling. My car has no minus sign, so I doubt -1 is a valid odometer reading. What about 4294967295? 4294967296? I might find it interesting if I could cause an integer overflow or underflow.

IsSequence is kind of confusing because it's handling both increments and decrements. Due to this, it's more difficult to understand how it handles the 0 threshold. Perhaps consider two separate, clearly-named methods that converge on a common logic core.

Consider renaming the class. CarMileage sounds like how much fuel a car uses per mile. Is this more of an AnalyzeOdometerReading or AnalyzeOdometerValue class?

\$\endgroup\$
2
\$\begingroup\$

The first thing that strikes me is the repetition of the three constants in the code and in the tests:

    const int NotInteresting = 0;
    const int AlmostInteresting = 1;
    const int Interesting = 2;

I'd make them internal and set the InternalsVisibleTo to the test.


IsSequence

For this one I'd use the OrderBy[Descending] + SequenceEqual. This would make it virtually a one-liner.

\$\endgroup\$
2
\$\begingroup\$

make the class static

I would return an enum instead of an int

Its Palindrome not Palendrome

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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