Using "magic" strings for single-use error reasons?

Question

I'm writing a fairly large piece of logic, during which there are 6 points where things could go wrong and execution should stop after logging the error. The error is also stored in an object.

However, up to now I have been using a public static final String for this error string. As it is only used in a single place (and almost certainly won't be used anywhere else), does it make more sense to remove this global variable declaration from the class and instead hardcode the error reason?

Answer 1

Declaring it separately means you can reference it in unit (or integration) tests. If you end up needing to change the contents of the string, you then only have to change it once (where it's defined), rather than making the same change in the other instances of the string.

There can be other benefits to centralization - for example, if you end up needing to add localization (other-language translated versions of the user-facing content), it's significantly easier to hand off a few files with all of the strings to a translator, rather than an ad-hoc bunch of spots throughout the code.

If you're not writing test code, you probably want to look into doing that - it's generally considered a best practice. If it's a quick personal project, it's OK to take the shortcut of hardcoding - but it's definitely not something that's generally recommended.

Edit: On rereading this, I realize that I'd missed a specific piece of your description: that the strings are currently defined publicly in the class. I had had the impression that they were defined externally, in a different file. My recommendation would be to move them out to a separate "Error Messages" class that contains all of them (which is what I'd thought you were describing from the start).

Answer 2

A little late response here, but I'll summarise some of the other comments on autophage's answer and combine them with my own experience. Personally, I would not usually put these in a static constant field, although it largely depends on context. The main reasons for this are outlined as follows:

Testing

If you use the static final field in both your class and your tests, you're never actually testing the content of the field. The following test would pass despite containing multiple typing errors:

public class Greeter {
  public static final String WELCOME_MESSAGE = "Welcmoe to my websiet";

  public String getWelcomeMessage() {
    return WELCOME_MESSAGE;
  }
}

class GreeterTest {

  @Test
  void welcomeMessage_Always_AsExpected() {
    // Passes and doesn't really test much
    assertEquals(Greeter.WELCOME_MESSAGE, new Greeter().getWelcomeMessage());
  }
}

Instead, it might be advisory to check the contents of the string in our test, rather than relying on internal implementation details:

// This highlights the typing mistakes
assertEquals("Welcome to my website", new Foo().getWelcomeMessage());

Readability

Sometimes I'm reading through some code and I get to something like this:

if (complexConditionIsMet()) {
  throw new IllegalArgumentException(ERROR_MESSAGE);
}

What does that mean? I then have to scroll to find where ERROR_MESSAGE is defined to decipher what this means. (Granted, a lot of tooling will do this for you on hover, but there are still plenty of tools like GitHub where this is still an issue.) If this read as:

if (complexConditionIsMet()) {
  throw new IllegalArgumentException("User email is invalid");
}

that's suddenly a lot easier to digest. While this example could be resolved somewhat by choosing better named variables, bear in mind that this is a trivial example, and differentiating similar exception messages stored in constants might be difficult while trying to be concise.

Where I really hate to see this is in template strings:

private static final String MESSAGE_TEMPLATE =
    "User %s does not have access to %s. Requires permission %s";

// ...

String.format(MESSAGE_TEMPLATE, username, resource, permission);

If I'm reviewing this code, for example, how can I tell if the correct number of arguments are being supplied? Are they in the correct order? Again, this breaks the flow of reading as I try to find the template and compare it with the provided arguments.

That being said, there are certain examples that do make sense to be stored in static final fields. Take the following example:

public class User {
  private String username;

  //...

  public Optional<String> getUsername() {
    return Optional.ofNullable(username);
  }
}

public class Greeter {

  public String getWelcomeMessage(User user) {
    return "Welcome to my website, " + user.getUsername().orElse("anon");
  }
}

It's not clear here why "anon" is used. You can probably guess that it's a default value, but this could be cleared up like this:

public class Greeter {

  private static final String DEFAULT_NAME = "anon";

  public String getWelcomeMessage(User user) {
    return "Welcome to my website, "
        + user.getUsername().orElse(DEFAULT_NAME);
  }
}

Note that here I'm using a mix of both a "magic" string, and a static constant, but I think this is a good compromise. Instead, this could have been written as:

return WELCOME_MESSAGE
    + user.getUsername().orElse(DEFAULT_NAME);

Doing something like this would be pretty horrible to read as I can't tell if the username is being concatenated in a way that makes sense. Is the welcome message "Welcome!"? If so, this won't read properly, but I can't tell without checking the value of the constant.

Conclusion

User your own judgement. I'd recommend not relying on the fields in the test class, but whether you store the value in a static field in the class itself should depend purely on what you think is the most readable solution.

Other thoughts

In your question, you use a public static final String field. If it's not being used elsewhere, you may as well change the access modifier to private to further hide the implementation details. If I tried to auto-complete using your class, I'd see that string as something I can retrieve and interact with. Do I need this? What's it for? I probably shouldn't see it to save myself this confusion.