1

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?

| improve this question | |
  • 1
    Does this answer your question? Usage of magic strings/numbers – gnat Feb 3 at 16:24
  • Constant values (public static final) are still hardcoded. – user253751 Feb 3 at 17:11
  • Pain Drive Development - hard code everything you not sure about, then when you see that you need to change all occurrences for the same reason - then you will introduce some centralised place to keep values. – Fabio Feb 3 at 22:56
3

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).

| improve this answer | |
  • 3
    "Declaring it separately means you can reference it in unit (or integration) tests." Such a value should be private and thus unaccessible in the tests. Remember that your tests are not testing implementations, but behaviour! – Vincent Savard Feb 3 at 16:31
  • That's definitely a valid approach, but in my view this depends on context and how the team approaches testing. I've known teams that treat the string itself as an implementation detail that shouldn't be considered a part of the class itself. (From an SRP standpoint, the idea would be "it's not the validator class's responsibility to spell the error text correctly, it should only be either adding the error or not"). – autophage Feb 3 at 16:51
  • 1
    @VincentSavard Maybe you want to test that it returns a particular error message, but without duplicating the error message. That is behaviour. – user253751 Feb 3 at 17:11
  • 1
    @user253751 Yes and no. You are correct that it might be behaviour you want to test, but I disagree that sharing the constant is testing the behaviour. You're exposing internal details of the class for the sole reason of testing, which is generally a code smell. As a client of your code under test, your test class is asserting that its observable actions are behaving as expected, without knowing how it is actually implemented. By sharing a constant, you couple your client with the how instead of the what. You want your test to fail if you change the constant, because that is a breaking change! – Vincent Savard Feb 3 at 17:30
  • @VincentSavard Observable behaviour: when the user enters a filename that doesn't exist, they get a FILE_NOT_FOUND error. When the user deletes a read-only item, they get a ITEM_READONLY error. I suppose you could put the error messages into another class entirely (which is no different), or you could return an error code, test for that, and translate it to a string later (more complexity). – user253751 Feb 3 at 17:48

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.