3
\$\begingroup\$

Problem Statement:

Write a program that cleans up user-entered phone numbers so that they can be sent SMS messages.

The rules are as follows:

  • If the phone number is less than 10 digits assume that it is bad number
  • If the phone number is 10 digits assume that it is good
  • If the phone number is 11 digits and the first number is 1, trim the 1 and use the last 10 digits
  • If the phone number is 11 digits and the first number is not 1, then it is a bad number
  • If the phone number is more than 11 digits assume that it is a bad number

We've provided tests, now make them pass.

Hint: Only make one test pass at a time. Disable the others, then flip each on in turn after you get the current failing one to pass.

Code:

public class PhoneNumber {
  private String number;

  public PhoneNumber(String number) {
    this.number = cleanNumber(number);
    setNumber();
  }

  public String getNumber() {
    return number;
  }

  public String getAreaCode() {
    return number.substring(0, 3);
  }

  public String pretty() {
    return String.format(
        "(%s) %s-%s",
        getAreaCode(), number.substring(3, 6), number.substring(6));
  }

  private void setNumber() {
    if (hasInvalidInput(number)) {
      number = "0000000000";
    } else if (number.length() == 11) {
      number = number.substring(1);
    }
  }

  private boolean hasInvalidInput(String number) {
    return hasInvalidLength(number) || hasInvalidPrefix(number);
  }

  private boolean hasInvalidLength(String number) {
    return (number.length() < 10 || number.length() > 11);
  }

  private boolean hasInvalidPrefix(String number) {
    return number.length() == 11 && !number.startsWith("1");
  }

  private static final String cleanNumber(String number) {
    return number.replaceAll("[\\s\\.()-]", "");
  }
}

Test Suite:

import org.junit.Test;

import static org.junit.Assert.*;

public class PhoneNumberTest {

    @Test
    public void cleansNumber() {
        final String expectedNumber = "1234567890";
        final String actualNumber = new PhoneNumber("(123) 456-7890").getNumber();

        assertEquals(
            expectedNumber, actualNumber
        );
    }

    @Test
    public void cleansNumberWithDots() {
        final String expectedNumber = "1234567890";
        final String actualNumber = new PhoneNumber("123.456.7890").getNumber();

        assertEquals(
            expectedNumber, actualNumber
        );
    }

    @Test
    public void validWhen11DigitsAndFirstIs1() {
        final String expectedNumber = "1234567890";
        final String actualNumber = new PhoneNumber("11234567890").getNumber();

        assertEquals(
            expectedNumber, actualNumber
        );
    }

    @Test
    public void invalidWhenOnly11Digits() {
        final String expectedNumber = "0000000000";
        final String actualNumber = new PhoneNumber("21234567890").getNumber();

        assertEquals(
            expectedNumber, actualNumber
        );
    }

    @Test
    public void invalidWhen9Digits() {
        final String expectedNumber = "0000000000";
        final String actualNumber = new PhoneNumber("123456789").getNumber();

        assertEquals(
            expectedNumber, actualNumber
        );
    }

    @Test
    public void areaCode() {
        final String expectedAreaCode = "123";
        final String actualAreaCode = new PhoneNumber("1234567890").getAreaCode();

        assertEquals(
            expectedAreaCode, actualAreaCode
        );
    }

    @Test
    public void prettyPrint() {
        final String expectedPrettyNumber = "(123) 456-7890";
        final String actualPrettyNumber = new PhoneNumber("1234567890").pretty();

        assertEquals(
            expectedPrettyNumber, actualPrettyNumber
        );
    }

    @Test
    public void prettyPrintWithFullUSPhoneNumber() {
        final String expectedPrettyNumber = "(123) 456-7890";
        final String actualPrettyNumber = new PhoneNumber("11234567890").pretty();

        assertEquals(
            expectedPrettyNumber, actualPrettyNumber
        );
    }

}

Again its not state of the art code, few points that's bugging me:

  • AFAIK the computation should be avoided in the constructor but I didn't want to do the same computation again and again while getting the number so, I had to do the sin.
  • The getAreaCode and pretty still uses and throws away strings don't know if its a good idea but by seeing that string isn't going to be too long I kind of traded of here.

Reference: Exercism

\$\endgroup\$
  • \$\begingroup\$ Your program does a lot more than the rules given in the problem statement describe. In fact, the given rules don't seem to describe the stated task at all -- they're validity rules, not details of what it means to "clean up" a phone number. Have you omitted part of the problem? \$\endgroup\$ – PellMel May 6 '16 at 18:26
  • \$\begingroup\$ You might want to look at the builder pattern which involves using work in constructors. You could build phone numbers using dependency injection \$\endgroup\$ – Niklas Rosencrantz May 6 '16 at 18:28
  • \$\begingroup\$ Umm… the question title doesn't make sense grammatically. \$\endgroup\$ – 200_success May 6 '16 at 22:50
3
\$\begingroup\$

Make it immutable

In cases when you're forced to work in the constructor, you might want to do all the work there. On the other hand, make the result of the calculations final, and the object created immutable.

Your implementation was effectively immutable. The value of number never change after construction time. But this was not really guaranteed. One could still accidentally add a statement to change number. You can improve this, by making the number field final.

And if number is final, then there's no point calculating area code either, or the pretty format, all these could be pre-calculated at construction time.

Pointless static final

The static and final modifiers don't make much sense together. You can drop the final safely, as a static method cannot be overridden anyway.

More static methods

Many of the helper methods don't need access to the fields, so could have been static.

The invalid number

The invalid number is not exactly trivial. Instead of having it buried deep in the implementation, it would be better to move it to a constant.

Suggested implementation

Putting the above suggestions together:

public class PhoneNumber {
    public static final String INVALID_NUMBER = "0000000000";

    private final String number;
    private final String areaCode;
    private final String prettyFormat;

    public PhoneNumber(String number) {
        this.number = cleanNumber(number);
        this.areaCode = this.number.substring(0, 3);
        this.prettyFormat = prettyFormat(this.number, areaCode);
    }

    public String getNumber() {
        return number;
    }

    public String getAreaCode() {
        return areaCode;
    }

    public String pretty() {
        return prettyFormat;
    }

    private static String prettyFormat(String number, String areaCode) {
        return String.format("(%s) %s-%s", areaCode, number.substring(3, 6), number.substring(6));
    }

    private static boolean hasInvalidLength(String number) {
        return (number.length() < 10 || number.length() > 11);
    }

    private static String cleanNumber(String number) {
        number = number.replaceAll("[\\s\\.()-]", "");

        if (hasInvalidLength(number)) {
            return INVALID_NUMBER;
        }
        if (number.length() == 11) {
            if (number.charAt(0) != '1') {
                return INVALID_NUMBER;
            }
            return number.substring(1);
        }
        return number;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thinking in OOP way I think that class PhoneNumber is doing too many things like its sanitizing the number AND its formatting it in proper, wdyt? \$\endgroup\$ – CodeYogi May 7 '16 at 2:42
  • \$\begingroup\$ Sanitizing and formatting are closely related to the phone number, so it's perfectly fine to have these logic in this class \$\endgroup\$ – janos May 7 '16 at 5:21
  • \$\begingroup\$ Sanitizing is fine, but formatting isn't. Formatting should be done by the "rendering" class. \$\endgroup\$ – Dave May 8 '16 at 18:49
2
\$\begingroup\$

I would replace PhoneNumber constructor with the following:

public PhoneNumber(String number) {
    if (number.matches( Pattern.compile("(1\\d{10}|\\d{10})") )) {
        if (number.length == 11) {
            this.number = cleanNumber(number.substring(2, number.length));
        } else {
            this.number = cleanNumber(number);
        }
    }
}

Other comments:

  • Setters (like setNumber()) should be simple Setters (you shouldn't be changing the number there, unless you change the method name). They should also take a parameter (the value you want to set).
  • pretty() isn't a descriptive method name. It also looks like it's been formatted differently to the rest of your code for some reason. Copy & Paste job? Discuss the method name with the author of the tests - since the tests were provided and they reference the method name.
  • There is absolutely no reason not to set internal variables appropriately in the Constructor. You're setting internal state ready for use, based on input provided. Calling setNumber() (especially with no argument) is more wrong than using the Constructor to establish internal state.
  • The pretty() current implementation causes a bug if I call new PhoneNumber("(123) 456-7890").getNumber()

This problem looks like it's on a course that you're taking, and I would genuinely change the method names in PhoneNumber to provide getNumber() and getFormattedNumber() - and I'd delete pretty() completely. This would make the whole project fail to compile, and I'd hand the work in with JavaDoc comments explaining my choice of actions.

Then I'd probably get an F. :-)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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