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.

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 :)

share|improve this question
7  
The first thing I noticed was that you had a class whose name ended with Manager. –  icktoofay Jul 4 at 20:32
    
Yeah, I just completely blanked out when naming. I was tempted to change everything when posting here, to save face, but honestly- this is what it looks like when I code under those circumstances. It ain't pretty that's for sure :) –  Iris Classon Jul 4 at 20:38
1  
Do you do any code katas? –  Hammerstein Jul 4 at 20:40
2  
Yes, I do on almost daily basis. I alternate between kata, koan and just reading code. I have 1h each morning in my calendar set aside for that, and an additional h for reading code in the evening. I just really like code :) –  Iris Classon Jul 4 at 20:51
2  
What exactly do you mean by "user stories"? Welcome to Code Review by the way, congratulations in getting a popular question on your first try! –  Simon André Forsberg Jul 4 at 22:56
show 4 more comments

9 Answers 9

up vote 11 down vote accepted

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)
    {
    .....
    }
}
share|improve this answer
    
I don't think an extension method is the way to go (although I don't frequently use C#), but I agree that the manager class is overkill. –  Simon André Forsberg Jul 4 at 21:15
2  
The class had more logic to it (there were 10 user stories in total, all related- as I wrote in the question). I've only posted one method and test as I need to confirm with the company that I can post the whole thing. –  Iris Classon Jul 4 at 21:16
    
@irisclasson what other fields did you have then? If it was just the string then my comments stand. –  Graeme Bradbury Jul 4 at 21:23
1  
An extension method on string for something that's only useful for one and only one use case of string which should probably be encapsuled in its own class to begin with? Urgh, no I'd consider that exceedingly bad design (does it make sense to have RemoveDialingCode for every string? hardly) Admittedly I'd just call the class Phonenumber, take a string as input in the constructor and correctly handle it there. –  Voo Jul 5 at 17:07
    
+1 because: "The single sign of over thinking was your use of a business primitive (PhoneNumberManager)." This is the idea of the PhoneNumberManager, not about the name of the class. Given this idea and then seeing the class keep state is enough for me. And this is why I also liked the extension methods approach suggested. Direct, stateless. Not introducing a new design concept. Whatever the larger design is like, extensions tend play nice. I posit that any discussion of implementation details is moot. –  radarbob Jul 13 at 1:19
add comment

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.

share|improve this answer
    
Yeah, major facepalm in retrospect. I got stressed out about my Regex inabilities and did a .Replace to move on to the next part. –  Iris Classon Jul 4 at 20:40
    
This site should help you visualizing your Regex : regexper.com/#%5B%5E0-9.%5D –  Xiaoy312 Jul 4 at 20:49
    
[^0-9]is only equal to \D if ECMAScript-compliant behavior is specified, see the "ECMAScript Matching Behavior" section in Regular Expression Options. –  Johnbot Jul 7 at 11:33
add comment

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.

share|improve this answer
4  
Had I been doing the interview, I would probably have said, "Great that you're writing tests! But for this task let's just skip ahead to the code if you're comfortable with that." –  David Harkness Jul 4 at 21:28
    
I asked if I should do a TDD approach since the hiring manager had said 'you'll pair up with X and we can see you code and test skills'. When I asked the other dev if I should do TDD he said:'do as you usually do'. So I went for TDD. –  Iris Classon Jul 4 at 22:31
1  
"Do what you normally do" can often be a way of saying "It's not what we normally do, but feel free to try and tell us we should be doing it differently" –  Rowland Shaw Jul 5 at 7:58
    
Good answer! Welcome to CodeReview as well, look forward to reading more of your answers! –  Malachi Jul 13 at 13:21
add comment

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.

share|improve this answer
    
Im sorry, I wrote forgot that two of the user stories were combined. The question was two part, replace country code with 0 and remove space and non digits. Ive updated the question –  Iris Classon Jul 4 at 20:27
add comment

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();
    }
share|improve this answer
6  
Welcome to Code Review! Your answer was in the "First Post" queue. Answers here tend to be better when they also explain a bit about what changed and not only "This code is maybe better" –  Simon André Forsberg Jul 4 at 21:06
6  
Meta review: Whenever you loop over a set to filter out something and maybe transform elements to result a new set then Linq is typically a tool of choice. Your code can be shortened into one line: return new string(input.Where(Char.IsDigit).ToArray()); –  ChrisWue Jul 5 at 7:16
    
Welcome to CodeReview! Nice simple answer to the question. I think Chris said it best. And I don't know what is more efficient, I assume that all the solution s from op and this answer use a loop in one way or another –  Malachi Jul 13 at 13:28
add comment

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.

share|improve this answer
    
That's not simpler. C# conventions are for the code to execute like you read (left to right). By inlining those method calls the code instead executes right to left. Also imagine what the code would look if all ten methods where chained in that way, you'd end with ten close parentheses. –  Graeme Bradbury Jul 4 at 22:10
    
@GraemeBradbury That's why I said "simpler" I agree that it's not a clean solution. I find the question lacks some context though. –  Simon André Forsberg Jul 4 at 22:14
    
@GraemeBradbury I updated my answer with another approach, do you like that one better? –  Simon André Forsberg Jul 4 at 22:41
add comment

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?

share|improve this answer
add comment

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
share|improve this answer
add comment
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.

share|improve this answer
1  
Welcome to Code Review! I fear that your answer does not quite fit with the format here. We are suppose to review the question code, not suggest one without explaining much. –  Marc-Andre Jul 8 at 13:21
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.