6
\$\begingroup\$

The following question was taken from Absolute Java 5th ed. by Walter Savitch:

Write a program that starts with the string variable first set to your first name and the string variable last set to your last name. Both names should be all lower- case. Your program should then create a new string that contains your full name in pig latin with the first letter capitalized for the first and last name. Use only the pig latin rule of moving the first letter to the end of the word and adding “ay.” Output the pig latin name to the screen. Use the substring and toUpperCase methods to construct the new name.
For example, given
first = "walt";
last = "savitch";
the program should create a new string with the text “Altway Avitchsay” and print it.

This is the code that I have written:

    public class Question3 {

    private static String first;
    private static String last;
    private static String newName;

    public static void main(String[] args) {

        Question3 name = new Question3("walt", "savitch");

        System.out.println(name.convertName(first) + " "
                + name.convertName(last));

    }

    public Question3(String lowerCaseFirstName, String lowerCaseLastName) {
        first = lowerCaseFirstName;
        last = lowerCaseLastName;
    }

    private String convertName(String originalName) {
        String firstLetter = originalName.substring(0, 1);
        newName = nameWithoutFirstLetter(originalName) + firstLetter; // move first letter to the end
        newName = capitalizeFirstLetter() + nameWithoutFirstLetter(newName) // capitalize first letter and add "ay" to the end
                + "ay";
        return newName;
    }

    private String capitalizeFirstLetter() {
        String capitalFirstLetter = newName.substring(0, 1).toUpperCase();
        return capitalFirstLetter;
    }

    private String nameWithoutFirstLetter(String name) {
        String restOfName = name.substring(1);
        return restOfName;
    }
}
\$\endgroup\$
7
\$\begingroup\$

Structure

Try not to use static variables. static is for when the value of a field is the same across all instances of your object. But this isn't the case with peoples names.

What if you have two people? You would create your people:

    Question3 name = new Question3("walt", "savitch");
    Question3 anotherName = new Question3("john", "smith");

But then, you cannot use them the way you want.

If instead you would rewrite your code like this:

public class Question3 {

    private String first;
    private String last;
    private String newName;

    public static void main(String[] args) {

        Question3 name = new Question3("walt", "savitch");
        Question3 anotherName = new Question3("john", "smith");
        System.out.println(name.convertFullName());
        System.out.println(anotherName.convertFullName());

    }

    public Question3(String lowerCaseFirstName, String lowerCaseLastName) {
        first = lowerCaseFirstName;
        last = lowerCaseLastName;
    }

    public String convertFullName() {
        newName = convertName(first) + " " + convertName(last);
        return newName;
    }

    [...]
}

You can see that you can create however many people you want.

Temporary variables

If you don't need variables, you shouldn't create them. For example:

    String restOfName = name.substring(1);
    return restOfName;

could just be

    return name.substring(1);

Functions

You have a function which removes the first letter from a string, but you do not have one for getting the first letter of a string, but instead write that functionality directly in your convertName function. This seems inconsistent. I would either have a function for both or neither.

I also wouldn't operate on newName directly in capitalizeFirstLetter. None of your other functions (for example nameWithoutFirstLetter) do this, so it is inconsistent and can be confusing.

Also, right now, newName is just a temporary variable, which isn't ideal. You can never be sure what state it has. I would either change the code so it always holds the complete new name (for example Altway Avitchsay), or just get rid of newName completely.

\$\endgroup\$
  • \$\begingroup\$ I disagree to a certain extend with the temporary variables. Sometimes it is just better for readability to have four, well named temporary variables, instead of four chained method calls. In this case, yes, the variable can be scraped (especially as the function is only one line and well named). \$\endgroup\$ – Bobby Sep 14 '14 at 9:45
6
\$\begingroup\$

These variables don't need to be static: first, last. It would be better to define these inside the main method as local variables.

newName is a static variable, set in convertName and later used by capitalizeFirstLetter. This is not good design.

Also, setting static variables in the constructor is not a good practice: the variable will have the value set by the last instance created, which makes the variable unpredictable and prone to errors.

It would be better to rewrite this program without static variables, and without the constructor, for example:

public static void main(String[] args) {
    String first = "walt";
    String last = "savitch";

    System.out.println(convertName(first) + " " + convertName(last));
}

private static String convertName(String name) {
    String firstLetter = getFirstLetter(name);
    String newName = nameWithoutFirstLetter(name) + firstLetter;
    return capitalizeFirstLetter(newName) + nameWithoutFirstLetter(newName) + "ay";
}

private static String getFirstLetter(String name) {
    return name.substring(0, 1);
}

private static String capitalizeFirstLetter(String name) {
    return getFirstLetter(name).toUpperCase();
}

I also added a getFirstLetter method, because you can reuse it in 2 places.

\$\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.