What are some indicators that I was over-thinking my solution to this problem?

Question

This question is a cross between career question and a code review. I was uncertain where to ask, but since there is code involved I went with CodeReview.

I’m going through the process of technical interviews, and it hasn’t been a positive experience but I’m trying to make sense of the feedback I’ve gotten to improve the experience.

While some feedback is conflicting with other feedback, I’ve been told more than once that I’m overthinking, and that my solutions are too complex (not talking about complexity as in Big O). I’m trying to understand what that means so I know how and what to change.

My theory is the following: I’m a junior developer, but the developers I hang out with are senior- and probably beyond that. All the conversations are around architecture and complex problems- and it probably rubs off on me as I find myself thinking a lot about ‘what is the best design etc.’

The questions I have (this where I need your help): What is meant by overthink/overly complex? Any examples? What would be a good approach to avoid doing that?

I’ll give you an example, and please keep in mind that the code was written under pressure and I was a nerve wreck. One of the things I had to implement was replacing a country code from a phone number with 0 and remove space and non didgit (this was one out of approx. 10 user stories). I did it TDD style, and here is the test:

        [TestMethod]
    public void StripNumber_WhenGivenNumber_ReturnsNumberWithoutSpaceAndNonNumeric()
    {
        var number = "046 55.5";
        var expectedString = "046555";

        var numberManager = new PhoneNumberManager(number);
        var cleanedString = numberManager.StripWhiteSpaceAndNonDigit().Result();

        Assert.AreEqual(expectedString, cleanedString);
    }

And the code after some quick refactoring.

    internal class PhoneNumberManager
{
    private string _nonNumeric = "[^0-9.]";
    private string _result;

    public PhoneNumberManager(string number)
    {
        _result = number;
    }

    public PhoneNumberManager StripWhiteSpaceAndNonDigit()
    {
        _result = Regex.Replace(_result, _nonNumeric, "").Replace(".", "");
        return this;
    }
    //bunch of other methods, and a Result() method
}

What/how I did:

• Started on test
• Wrote enough code to make it pass – some red test in-between, did some debugging
• Refactored to a class
• Verified test still runs green
• Quick minor refactoring
• Moved on to next story

Took me 20 minutes with some discussions around it and other questions that came up (I would think it would take way less if I was at work and coding and not an interview)

Keep in mind this was live coding via screen share, and this was just one of of 10 stories to implement. It pains me to share this, but this is how my code looks like under pressure.

EDIT: Since some seem to have missed the part about several user stories: There were 10 of them, all of them dealing with the phone number somehow.

EDIT RE. TDD: The interviewer talked a lot about TDD before the interview (and how this was important to them), and before I started coding I asked if I should do TDD since they had said I would be evaluated on code and tests. Reply was : 'do what you usually do' - so I did TDD.

As I was coding I said things such as:

• I verified the amount of data we would be working with so I could estimate how much I should worry about complexity
• Naming isn’t terrible good here, but I’ll leave it at that for now since the focus is the code (I blanked out on naming basically)
• I don’t like the Regex and Replace combo, I should use a Regex only
• The other dev (interviewing) suggested a Regex match but I said no since that meant we would get a split at each non digit, yielding a collection which I thought would add overhead.
• I didn’t like the lonely “0” concat
• I regretted not using a string builder instead- but data amount wouldn’t be big (first thing I asked)

At the end of the coding session I refactored the class to a fluent approach with method chaining, explaining that I considered this to be a pipeline approach where we might want to use different pipes and still have one defined place to go for the output.

Where did I go wrong? What are the signs of overthinking in the code?- and also, always happy to hear what is messed up in the code or can be improved :)

Answer 1

The single sign of over thinking was your use of a business primitive (PhoneNumberManager).

The problem called for you to start with a string and end with a string so no need for an extra type in the middle.

It looks like it also caused you to miss the obvious string extension approach.

Getting the Regex wrong is normally ok in interviews since almost nobody writes Regex perfectly of the top of the head. An improvement in that area would be to not recreate the regex on each call, but use a static regex.

so code wise:

public static class PhoneNumberExtensions
{
    private static readonly Regex NonDigit = new Regex(@"\D", RegexOptions.Compiled);

    public static string StripWhiteSpaceAndNonDigit(this string phoneNumber)
    {
        return NonDigit.Replace(phoneNumber, string.Empty);
    }

    public static string RemoveDialingCode(this string phoneNumber)
    {
    .....
    }
}

Answer 2

The Regex expression [^0-9.] matches anything other than 0 to 9 and .. If you remvoe the . from the expression, It should only match everything you want. But, this can be furthermore simplify to \D which matches anything but numbers.

So the whole thing could be written in one line

Regex.Replace(value, @"\D", "")

---

I don't think it is worth to create a PhoneNumberManager class that holds the direct (and maybe? the temporary) result to perform this kind of task. It could be refactored down to a helper class like this :

internal class PhoneNumberHelper
{
    public static string StripNonDigit(string number)
    {
        const string NonDigit = @"\D";
        return Regex.Replace(number, NonDigit, string.Empty);
    }

    // and your bunch of other methods below
}

---

Pre optimization is the root of all evil

While your code may not be perfect or optimized, but it works and that is your primary focus/target. Optimization and refactoring only come after because, while working on the code, your view is narrowed on the problem. And, thus unable to view the big picture.

Don't push yourself too much when you just started as junior developer. A lot of tools and techniques are refined from experiences that you will eventually pick up along the way.

Answer 3

When you're in an interview, few interviewers expect you to come up with the perfect solution. Mainly, they're looking for an idea of how you think and how you approach a problem. Especially as a junior programmer.

Approaching the problem from a test driven perspective first might seem the logical way, but what about just writing the code to solve the problem, holding back on the test driven approach until you're asked how you could make it better, let the conversation take it. Some places still don't understand what test driven really means, so you might come across as too advanced. If you want to find out about whether they value this skill set, aim your questions to them around finding out about their development process.

I love the "when you have a hammer, everything looks like a nail" phrase. You have a whole toolbox you've built up in relatively short amount of time and it's coming naturally to you to apply some of that stuff.

As for indicators you are overthinking it;

• Trying to start by building up infrastructure around the problem
• When you have to think about what nuget packages you need for the problem
• Basically, whenever your mind wanders away from the problem you've been tasked

Employers are looking for developers to solve problems. So, they're keen to know if you can take a requirement, run with it and turn it in to a solution.

Answer 4

This would have been maybe easier to read:

    public static string GetFormattedPhoneNumber(string input)
    {
        StringBuilder result = new StringBuilder();

        foreach (char character in input)
        {
            if (Char.IsDigit(character))
            {
                result.Append(character);
            }
        }

        return result.ToString();
    }

Answer 5

Your regex removes anything that's not 0-9 or a '.', then you replace the '.'. You could have removed the '.' from the regex to do it all in one shot:

"[^0-9]" instead of "[^0-9.]"

Remember to make sure to answer the question that was asked. You've simplified the number, but the requirement is to get the country code.

In tech questions, I would tend to focus on the smallest amount of code that can provide the answer that is being requested. Smaller code is easier for the reviewer to understand (if you're doing something tricky).

I wouldn't take a TDD approach. I would have a single function that can be called to produce the output (maybe with supporting functions or classes).

The testing approach can test your function, but the most important thing is to actually answer the question, and demonstrate the result that they are looking for.

Showing that you know things beyond the scope of the question sounds like a good idea, but it can obscure your solution, and possibly make you lose out when a simpler solution might have done the trick.

Answer 6

One major "overthinking" to me is the return this;

I personally use that mostly for the Builder pattern. As your class is named PhoneNumberManager, it doesn't sound like a PhoneNumberBuilder (possible rename?). You mentioned that you have other methods as well, so the main question is: Does it makes much sense to chain them? Or do you only have to call one or two of them and then get the result?

If it does not make sense to chain them, which it doesn't by the look of your code in your test:

    var numberManager = new PhoneNumberManager(number);
    var cleanedString = numberManager.StripWhiteSpaceAndNonDigit().Result();

Then var cleanedString = PhoneNumber.StripWhiteSpaceAndNonDigit(number); would be much better, readable, and less complex.

---

Another approach, assuming that all the methods return string and requires string could be to use Func<string, string> delegates. Then I believe you could have a whole bunch of static methods in your PhoneNumber class and use one utility method:

public static string Process(IEnumerable<Func<string, string>> functions, string str) {
     string value = str;
     foreach (Func<string, string> func in functions) {
          value = func.apply(value);
     }
     return value;
}

And then call it as PhoneNumber.Process(PhoneNumber::StripWhiteSpaceAndNonDigits, PhoneNumber::StripStuff)

I might have mixed C# with Java syntax here. (Old Good habits die hard)

---

I have to say that you seem to know your C# though, you're adhering to all the C# conventions I know of, except of course that your private string _nonNumeric = "[^0-9.]"; should be readonly or const.

Answer 7

Not sure if this is the issue, because I wasn't there (and missed all the communication), but:

If the requirement is:

One of the things I had to implement was replacing a country code from a phone number with 0 and remove space and non digit (this was one out of approx. 10 user stories).

The test case you have to explain the requirement:

[TestMethod]
public void StripNumber_WhenGivenNumber_ReturnsNumberWithoutSpaceAndNonNumeric()
{
    var number = "046 55.5";
    var expectedString = "046555";

    var numberManager = new PhoneNumberManager(number);
    var cleanedString = numberManager.StripWhiteSpaceAndNonDigit().Result();

    Assert.AreEqual(expectedString, cleanedString);
}

(I assume you have some other test case that explains the replacement of country code by 0?, which is done by another method?)

If I read the requirement and write it as one test I end up with:

public class PhoneNumberTests
{
    public class Parse
    {
        [Fact]
        public void Must_parse_phone_number_to_a_correct_local_number()
        {
            // Arrange
            var phoneNumber = "+31 20.1234.567";

            // Act
            var result = PhoneNumber.Parse(phoneNumber);

            // Assert
            Assert.Equal("0201234567", result.LocalNumber);
        }
    }
}

The way the test code is written doesn't matter, but look at the difference in talking to the object.

(We can argue all day if static methods are good or bad, but that's not what I want to point out.)

You don't have an object to describe a phone number, but some Swiss army knife class that can do whatever you ask it to a string/phone number (I don't know if that was the requirement).

You are unable to ask it: Can you give me the local number for this string?

You need to ask it: Can you replace the spaces for me and remove the non digit characters, and also replace the area code by 0 for this string?

I don't know what was important in the given problem domain, but it might be that just getting the local number is enough. Then everything else might be seen as over thinking?

Answer 8

Here are a few points that I wouldn't like as an interviewer:

• The name of the test method is extremely abstract, just call it Test
• The name PhoneNumberManager is confusing for this purpose, PhoneNumberHelper would be more approperiate
• You are initializing a class to do a simple task while all you need is one static method
• You should return the reformatted phone number immediately
• There is no reason to return this because obviously it is already known to the caller
• You're using a regex to modify data while you aren't even sure if the data is valid in the first place, but I just assume that the interviewer didn't ask for this explicitly.
• You are doing an additional String.Replace operation, you could have done that immediately with Regex.Replace
• _nonNumeric should have been a literal

Answer 9

You've already got a lot of good answers about your class design. (This should be a static method taking a string as an argument and returning a string, nothing else, but you've already been told that).

So let me focus on the regexp design. This isn't a regexp problem, you've a very simple text processing task. Use a regexp for complex ones, when you need to get strings with different lengths, replace strings and not characters, etc.

Here, all you need it to filter a string. Since strings are immutable in C# (which in this case is sad, because otherwise you could alter the string in place since the resulting string always will be equally long or shorter) you need to create a new string with only some of the characters in the old one.

static string removeNonValid(string nbr)
{   
    StringBuilder sb = new StringBuilder();
    for (int i = 0; i < nbr.Length; i++)
    {   
        if (Char.IsDigit(nbr[i]))
        {   
            sb.Append(nbr[i]);
        }   
    }   
    return sb.ToString();
}   

Yes, regexp will work here, since regexp can do it all, but it's way overkill (and a lot slower).

On a side note, the task was also to remove the country-code, which you didn't. I suppose that you did, but didn't provided the code here. Otherwise, that would of course be a huge miss.

Answer 10

string Digits = (new PhoneNumberManagerFactory() as IPhoneNumberManagerFactory).Create<PhoneNumberManager>().StripNonDigits("123.456");

This might be an example of a little 'over-thinking' for this kind of problem. However i like the abstract-factory pattern since you can easily add functionality to it.