Failed Tech Interview, Can you review my code and tell me what I've done wrong?

Other people have already made several good critiques, but to repeat the big ones:

  • Your constants don't really add much
  • Rather then using regex, you should instead just let the code fail and catch the exception.
  • Your code is using globals to communicate between methods, which is a huge issue to the point of being almost taboo with many programmers
  • Your code is apparently not perfectly externally correct. I haven't run and tested your code myself, so I'll take the other commenters at their word, but submitting a program which does not work perfectly is never good.

Some additional critiques:

  1. Your methods are too long -- as a general rule of thumb, you should try and keep each method under at least 20 lines (ignoring things like comments and blank lines). If possible, you should go even further and shoot for about 5-10 lines per method.

    The idea is that if your methods are any longer then 10-20 lines, then it either contains redundancy that should be refactored, or contains distinct and separate subtasks that should be moved out.

    For example, lines 29-35 and 38-44 are practically identical so should be pulled into a separate method. Lines 136-170 are highly redundant -- each if statement should have been refactored. Better yet, you should have stored the constant values in an array, and looped over that array to find the change needed -- that way, your program should in theory be able to handle arbitrary monetary values.

  2. In fact, the fact that you're not using an array or a loop of some sort, and are attempting to handle each coin value manually/using separate variables is a bit of a red flag for me. This sort of scenario is exactly the sort of thing loops and lists excel at handling, and you should be able to recognize that.

  3. Your if statements also frequently contain redundant statements. For example, take lines 31-35. Those lines could be shortened to:

    String productPrice = keyboardInput.nextLine();
    if(!productPrice.matches(REGEX_PRICEPATTERN)) {
        productPrice = getCorrectValuePattern(productPrice);
    }
    product = Double.parseDouble(productPrice);
    

    Another example -- lines 92-102 could be shortened to:

    if (!moneyGiven.matches(REGEX_PRICEPATTERN)) {
        moneyGiven = getCorrectValuePattern(moneyGiven);
    }
    double hasPaid = Double.parseDouble(moneyGiven);
    needsToPay -= hasPaid;
    paid += hasPaid;
    
  4. Lines 114-117 should be simplified to:

    return paid > product;
    
  5. Several of your lines are too horizontally long -- in particular, lines 172-184. This makes your code much less readable -- you should try and keep every line of code under 100 characters long at max, including indents. If possible, it would be good to go even further and keep your lines about 80 characters long at max.

    As a general rule of thumb, you should almost never have to horizontally scroll when reading code. You should also assume that most coders will not have their editor be fully expanded, especially if they're working on medium to larger screens. For example, I keep my window half-expanded, taking up the left side of my screen, and my browser open to the right, so I can look up documentation.

    I'm willing to bet many other coders do the same, so you should be able to accomodate such a setup + not force me to horizontally scroll.

  6. You should probably include a file or class header comment summarizing your program.

/r/learnprogramming Thread