2
\$\begingroup\$

I'm writing a small almost one method Java class for a job application and the advice given was to write it as though it was a piece of commercial software.

The processResult method needs two string arguments of equal length. I thought the best thing to do is check that the args has at least 2 items, then inside the processResult method check that the strings have the same length.

I am unsure about throwing a generic exception in my main method and then later using the try/catch statement. Is this a good practice to do? Would it be better practice to handle these as part of one try catch? Are custom Exceptions a good way to go for this type of problem?

Here is what I have at the moment.

public class Task {
    public static void main(String[] args) throws Exception {
        if (args.length < 2) {
            throw new Exception("Need 2 string arguments");
        }

        try {
            int result = processResult(args[0], args[1]);
            System.out.println(result);
        } catch (IllegalArgumentException e) {
            System.err.println(e.getMessage());
        }
    }

    private static boolean lengthsEqual(String a, String b) {
        return Integer.compare(a.length(), b.length()) == 0;
    }

    private static int processResult(String inputOne, String inputTwo) {
        if (!lengthsEqual(inputOne, inputTwo) {
            throw new IllegalArgumentException("Strings must be the same length");
        }

        // method logic

        return 0;
    }

}
\$\endgroup\$
  • \$\begingroup\$ Ah yes.. edit on the way \$\endgroup\$ – Joel Biffin Jun 11 at 20:33
  • \$\begingroup\$ Uh, one thing: a.length == b.length is easier compared to Integer.compare(..) == 0. \$\endgroup\$ – slowy Jun 12 at 14:42
0
\$\begingroup\$

Usually, we throw an exception if some pre-condition or assumption we've made about the code is violated. So unless processResult assumes/requires that the two string arguments will be equal to do its job, I wouldn't throw an exception.

When the program is as small as this, it could be okay to change the initial check to:

if (args.length != 2 || lengthsEqual(args[0], args[1])) {
    throw new IllegalArgumentException("Need 2 string arguments of the same length");
}

Though, if the program will ever have to be altered to accommodate for strings of unequal length, then this will have to be changed. Generally, commercial code tries to be as reusable and extensible as reasonably possible to allow for quick extension and minimal introduction of bugs upon extending or altering code behaviour. Since we can't always predict the future, this is usually done by the programmer attempting to make a some assumption about where/how the code might have to be expanded in the future. If this program was under active development and intended to be developed upon for the foreseeable future, it might be a reasonable assumption that at some point in the future the program may have to handle two strings of differing length due to a new business requirement and, thus, we wouldn't put this exception here.

This is all highly contextual, and it's hard to make any meaningful decision with a single-purpose, one-time program like this. That being said, it's generally useful to keep an exception to as small a scope as you can so that you can more quickly pinpoint the potentially problematic code throwing the exception in the event of a bug, so I think it's okay to have two exceptions here, though maybe the first exception is better to be an IllegalArgumentException as well.

Two potential issues:

  • I don't think there's any reason to use Integer.compare() instead of the == operator, since the return type of length() is a primitive int according to the Oracle JavaSE 7 documentation.

  • Check if args.length != 2, instead of checking it's less than 2 since somebody could supply too many strings as opposed to not enough.

\$\endgroup\$
  • \$\begingroup\$ Thanks for this, does that mean there is a time penalty for the casting in the Integer.compare() method? Just due to the description of the task I felt that the single if statement might give the impression that I'm not thinking on the larger scale in terms of production-level code. Maybe I'm over thinking it. \$\endgroup\$ – Joel Biffin Jun 11 at 21:20
  • \$\begingroup\$ Also, I feel like I need to show Exception handling in the method itself, since doing all the sanity checking in the main method would mean external methods wouldn't have the string length checking when called elsewhere. \$\endgroup\$ – Joel Biffin Jun 11 at 21:21
  • \$\begingroup\$ There would be a performance hit in using the Integer.compare() method due to autoboxing/unboxing in Java (docs.oracle.com/javase/tutorial/java/data/autoboxing.html). I'm not sure how significant the performance hit would be, you'd probably want to be making a lot of calls that result in autoboxing/unboxing in performance-sensitive section of code for it to be significant. \$\endgroup\$ – screeb Jun 11 at 21:53
  • \$\begingroup\$ The above only really applies if Integer.compare() actually does result in the autoboxing of the primitives passed to it -- maybe it doesn't, I'm not too familiar with the implementation. That's another good argument for curtailing the exception to the method so that the method can be called elsewhere. \$\endgroup\$ – screeb Jun 11 at 22:27
  • \$\begingroup\$ Unfortunately the requirement of making your code "production-ready" is a bit vague and difficult to apply to a small-scale problem. Often times, applying techniques to abstract and make code more re-usable that are often seen in large software projects don't make all that much practical sense to apply to a small project like this as you might end up over-engineering the code and hampering readability. I'd try to focus on simplicity, good variable naming and making sure the code is easy to understand and read. \$\endgroup\$ – screeb Jun 12 at 9:18
2
\$\begingroup\$

I took your code and rewrote it a bit, so that it satisfies my personal requirements for production-ready code.

if (args.length < 2) {
    System.err.println("usage: Task <arg1> <arg2>");
    System.exit(1);
}

I removed the throw new Exception since passing the wrong number of arguments is not a programming error but a wrong invocation of the program. Only programming errors should print a stack trace. Instead of saying "Need 2 string arguments" I am following the "usage:" pattern that has been successfully established by Unix programs. Since you didn't provide any context in your question, the best variable names I could come up with are <arg1> <arg2>. If your task were to copy a file, the usage line should better be usage: Copy <source> <target>, of course.

} catch (IllegalArgumentException e) {
    System.err.println(e.getMessage());
    System.exit(1);
}

I added the System.exit since it was missing. If the program fails, it must report this via System.exit or by throwing an exception from the main function.

private static int processResult(String inputOne, String inputTwo) {
    if (inputOne.length() != inputTwo.length()) {
        throw new IllegalArgumentException("Strings must be the same length");
    }

There's no need to have a separate function for checking the lengths of the strings. Using IntelliJ it was quite simple to inline the method call to lengthsEqual (I just pressed Ctrl+Alt+N) and to remove the redundant call to Integer.compare (which was already marked in dark gray, so I just had to press Alt+Enter there).

It's unfortunate that the exception message says "String must be the same length". This message does not tell which strings are meant, and since that message is printed to System.err later, it should be worded with the same care as the usage message.

I disagree with bag's answer, especially the part that commercial code needs to be extensible. It doesn't. It needs to be easy to read, and it needs to clearly tell its intention. If the business requires that the two strings be equal (for whatever reason), the code should say exactly this, and the code should match the wording of the original requirement as closely as possible, so that later changes to the requirements can be programmed as easily as possible.

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