6
\$\begingroup\$

Going through a book, I encountered the following assignment:

Write an application that asks for the user’s first, middle, and last names and replies with the user’s initials.

Are there any flaws you notice immediately? Is there any way this code can be improved? I'm not really interested in coding style (I guess someone is going to say that I should put those braces on the next line, but I like it this way).

import javax.swing.JOptionPane;

public class Program
{

private static String fullName;
private static String firstName;
private static String middleName;
private static String lastName;

private static char firstInitial;
private static char middleInitial;
private static char lastInitial;

public static void main(String[] args)
{
    start();
}

public static void start()
{
    greet();
    askInput();
    seperateFullName(fullName);
    getInitials(firstName, middleName, lastName);
    showResult();
}

// @formatter:off
private static void greet()
{
    int dialogResult = JOptionPane.showConfirmDialog(null,  "This program asks you for you first, middle and last name"
                            + " and gives back your initials.\nDo you want to continue?");

    if (dialogResult == JOptionPane.NO_OPTION)
    {
        JOptionPane.showMessageDialog(null, "Shutting down...", null, JOptionPane.WARNING_MESSAGE);
        System.exit(0);
    }

}

private static void askInput()
{
    fullName = JOptionPane.showInputDialog("Please enter your first, middle and last name.");
}

private static void seperateFullName(String fullName)
{
    try
    {
        int firstSpace = fullName.indexOf(" ");
        firstName = fullName.substring(0, firstSpace);
        int lastSpace = fullName.lastIndexOf(" ");
        middleName = fullName.substring(firstSpace + 1 , lastSpace);
        lastName = fullName.substring(lastSpace + 1, fullName.length());
    }
    catch (StringIndexOutOfBoundsException e)
    {
        JOptionPane.showMessageDialog(null, "Invalid input!", null,
                JOptionPane.WARNING_MESSAGE, null);
        start();
    }
}

private static void getInitials(String firstName, String middleName, String lastName)
{
    firstInitial = firstName.charAt(0);
    middleInitial = middleName.charAt(0);
    lastInitial = lastName.charAt(0);
}
// @formatter:on

private static void showResult()
{
    JOptionPane.showMessageDialog(null, "Your initials are: "
            + firstInitial + "." + middleInitial + "." + lastInitial + ".");
}
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Is the current indentation level intentional, or an artefact of posting here? \$\endgroup\$ Commented Aug 6, 2015 at 22:24
  • 1
    \$\begingroup\$ Because ew. At least indent the class body. \$\endgroup\$ Commented Aug 6, 2015 at 22:27
  • \$\begingroup\$ As @jacwah's answer points out, this code doesn't even compile. \$\endgroup\$ Commented Aug 7, 2015 at 4:02
  • \$\begingroup\$ Sorry, I first didn't use the charAt method, instead I used substring, but when I saw that the code could be improved, I edited the question without changing the type to from String to Char, my bet. The indentation is just because I posted it here, it looks good in eclipse. \$\endgroup\$
    – Maxim
    Commented Aug 7, 2015 at 12:11

3 Answers 3

10
\$\begingroup\$

Other than the style (yeah, you mentioned it) I think that you can:

First a small change: replace this

    int firstSpace = fullName.indexOf(" ");
    firstName = fullName.substring(0, firstSpace);
    int lastSpace = fullName.lastIndexOf(" ");
    middleName = fullName.substring(firstSpace + 1 , lastSpace);
    lastName = fullName.substring(lastSpace + 1, fullName.length());

with this:

String[] parts = fullName.split(" ");

But the main change I would suggest is a design one: remove the need for instance variables, by making each function return the input for the next.

public static void start()
{
    greet();
    String fullName = askInput();
    String[] nameParts = seperateFullName(fullName);
    String[] initials = getInitials(nameParts);
    showResult(initials);
}

(and of course modifying each function to return the value they compute)

Why?

  • It decouples each different part of the program, keeping them independent, specific, and therefore less prone to errors introduced somewhere else in the code.
  • It helps also with testing, as you can easily test each function separately.
  • It also helps with reusability as you could re-use each different part in other places.

Other than that, check the spelling in functions' names (For example: in seperateFullName separate is misspelt)

\$\endgroup\$
1
  • \$\begingroup\$ Should modify to work with double barreled names :-) \$\endgroup\$ Commented May 3, 2016 at 6:31
5
\$\begingroup\$

Classes aren't objects

private static String firstInitial;
private static String middleInitial;
private static String lastInitial;

This isn't how static variables are usually used. You're essentially limiting yourself to do this only once in the entire program. More common would would be to make these instance fields in a Name object. Then adding more names is as simple as adding more object instances.

class Name {
    private char firstInitial;
    private char middleInitial;
    private char lastInitial;

Or you can do without them entirely. See the section on getters below.

Separation of concerns

public static void start()
{
    greet();
    askInput();
    seperateFullName(fullName);
    getInitials(firstName, middleName, lastName);
    showResult();
}

This is a very step-based, procedural approach. It doesn't take advantage of Java's object-orientated nature.

You pass class fields into each method, but you don't need to do so. They're already there. You already use the fields rather than returning the values. It would be better to separate on some other basis.

Also, getInitials requires seperateFullName (sic) which requires askInput, but if you switch them, the compiler won't notice. You'll eventually get runtime errors.

Try to avoid generic names like askInput and showResult. They don't read as well as more specific names, and they can clash with future times when you need to ask for input or show results.

    Name name;
    do {
        greet();
        name = new Name(askName());
    } while (name.hasError());
    showInitials(name);

This way, greet and showInitials know how to display; askName knows how to get input; and the Name object knows how to store and process data. And if you change the order, you'll get a compile time error.

    private static void showResult()
    {
        JOptionPane.showMessageDialog(null, "Your initials are: "
                + firstInitial + "." + middleInitial + "." + lastInitial + ".");
    }

If you pass a Name object into the method, you can use getters to fetch the initials.

    private static void showInitials(Name name)
    {
        JOptionPane.showMessageDialog(null, "Your initials are: "
                + name.getFirstInitial() + "."
                + name.getMiddleInitial() + "."
                + name.getLastInitial() + ".");
    }

You can easily implement the getters like

    public char getFirstInitial() {
        return firstName.charAt(0);
    }

Or you could generate the initials in the constructor if you use them frequently. That doesn't seem necessary in this case though.

Or you can change to a paradigm that doesn't require initials to come in threes:

    private static void showInitials(Name name)
    {
        StringBuilder message = new StringBuilder("Your initials are:  ");
        for (char initial : getInitials()) {
            message.append(initial);
            message.append('.');
        }

        JOptionPane.showMessageDialog(null, message.toString());
    }

Careful when handling errors

    }
    catch (StringIndexOutOfBoundsException e)
    {
        JOptionPane.showMessageDialog(null, "Invalid input!", null,
                JOptionPane.WARNING_MESSAGE, null);
        start();
    }

Sneaky. You start all over again. However, you'll still have the rest of the previous run to finish. So you will call showResult twice.

Note that my revised version doesn't require this. You'd instead mark an error in that case, which will be handled in the caller. That way Name doesn't need to know how it was called. As it is, this is a separation of concerns violation. You could only call this code from start.

\$\endgroup\$
4
\$\begingroup\$

getInitials is broken. String.charAt returns a Character, which you try to assign to a variable of type String. I'm not sure if you run a different version of Java where this is legal or if you simply didn't test your code. Since the three intial components can only be one character long, I suggest you declare them as Character instead.

The program should be able to handle more than one space between the first, middle and last name. It should also handle leading spaces. Typing in " Jacob Michael Wahlgren" (notice the leading space) causes the program to terminate with a StringIndexOutOfBoundsException. Use String.trim to remove leading and trailing whitespace in a string.

@Sebastian already mentioned String.split, but his version can be improved. Instead of split(" "), use split("\\s+"). The first splits the string at any single space, while the latter splits the string at one or more whitespace characters (space, tab etc). A string like "Jacob Michael Wahlgren" (oops, double space) will then be correctly parsed. See the String.split and Java regex documentation.

It's a bit annoying that new dialog boxes open underneath other windows. I'm not very familiar with Swing, but I'm sure there's a way to grab focus or open a new dialog on top.

Initials are alway upper case - use the String.toUpper method to enforce this even if the user writes input in lower case.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Not all initials are upper case - for example, "John von Doe" (although whether or not "v" should be considered an initial is also debatable) \$\endgroup\$ Commented Aug 6, 2015 at 22:47
  • \$\begingroup\$ @Dannnno The "von" is not a middle name though, I guess the initial would be just J.D. \$\endgroup\$
    – jacwah
    Commented Aug 6, 2015 at 22:49
  • \$\begingroup\$ That's true, but it is an example for which this implementation would produce a bad result. Furthermore, while I don't have any evidence to prove this, I suppose that upper case initials could be locale-based (and I think, at least in some places, you could legally change your name to something lower case). \$\endgroup\$ Commented Aug 6, 2015 at 22:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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