figuring out how to display special error message

[Jack Tauson]

Right now I've a class- MessageReturn like this and this class is used for an error message like this:

String proc = abc.getCodesDisplay();
proc = proc.replace("P", ""); 
if (optionQH.equals("EDC")) {
	if (proc.indexOf("E") == -1 || proc.length() > 1) {
			return new MessageReturn("This is a general error message - " + proc);
}

I want to have something like private String specialErrorMsg; in the MessageReturn class and then use it like this from some other class.

return new MessageReturn("This is a special error message - " + proc);

But the constructor will always take errorMsg as that's what the constructor is doing. I can't define another constructor like this

public MessageReturn(String specialErrorMsg) {
		this.specialErrorMsg = specialErrorMsg;
}

So I'm wondering how can I use specialErrorMsg variable for the message?

public class MessageReturn {

	private String errorMsg;
	private String successMsg;
	private String warningMsg;
	

	public MessageReturn() {

	}

	public String toString() {
		return ToStringBuilder.reflectionToString(this);
	}

	public MessageReturn(String errorMsg) {
		this.errorMsg = errorMsg;
	}

	public String getErrorMsg() {
		return errorMsg;
	}

	public void setErrorMsg(String errorMsg) {
		this.errorMsg = errorMsg;
	}

	public String getSuccessMsg() {
		return successMsg;
	}

	public void setSuccessMsg(String successMsg) {
		this.successMsg = successMsg;
	}

	public String getWarningMsg() {
		return warningMsg;
	}

	public void setWarningMsg(String warningMsg) {
		this.warningMsg = warningMsg;
	}

}

[Paul Clapham]

So now you're going to have "private String specialErrorMessage" in there as well as the other three types of message?

I'd suggest stop making "errorMessage" be handled differently than the other message types. Have only one constructor which takes no parameters and have all of the data fields set by "setXXXMessage" methods.

You could also have the "setXXXMessage" methods return "this" instead of void, so you could chain them together:

public MessageReturn() {
}

public MessageReturn setErrorMessage(String errorMessage) {
  this.errorMessage = errorMessage;
  return this;
}
// and so on

Then you could write

return new MessageReturn().setSpecialErrorMessage("This is a special error message - " + proc));

[Jack Tauson]

Paul Clapham wrote:So now you're going to have "private String specialErrorMessage" in there as well as the other three types of message?

I'd suggest stop making "errorMessage" be handled differently than the other message types. Have only one constructor which takes no parameters and have all of the data fields set by "setXXXMessage" methods.

You could also have the "setXXXMessage" methods return "this" instead of void, so you could chain them together:

public MessageReturn() {
}

public MessageReturn setErrorMessage(String errorMessage) {
  this.errorMessage = errorMessage;
  return this;
}
// and so on

Then you could write

return new MessageReturn().setSpecialErrorMessage("This is a special error message - " + proc));

Just to clarify, you are suggesting to replace the following method:

public MessageReturn(String errorMsg) {
        this.errorMsg = errorMsg;
    }
 

with the following, right?

public MessageReturn setErrorMessage(String errorMessage) {
  this.errorMessage = errorMessage;
  return this;
}

And I would still define private String specialErrorMessage and using it's setter method, I could do the following:

return new MessageReturn().setSpecialErrorMessage("This is a special error message - " + proc));

Please let me know if I misunderstood anything here. Thanks

[Paul Clapham]

Yes, that sounds right. Give it a try and see what goes wrong.

[Lou Hamers]

Side comment.. if you can only have one message be returned at a time, you could be more concise with a single message field and a flag indicating what type it is.

private String message;
private MessageReturn.Type type;

public MessageReturn.Type getType() {
  return type;
}

public void setType(MessageReturn.Type type) {
  this.type = type;
}

public enum Type {
  SUCCESS, WARNING, ERROR, SPECIAL_ERROR
}

[Junilu Lacar]

I concur with Lou's suggested approach, but without the setter for message type. Do it all in the constructor and make the class immutable.

Also, consider renaming the class to simply Message: I don't see any reason to name it "MessageReturn" - you're probably thinking "It's a massage that I return, so 'MessageReturn'". No. It's a message plain and simple. The fact that you return it from a method is irrelevant to the name choice.

[Campbell Ritchie]

Junilu Lacar wrote:. . . Do it all in the constructor and make the class immutable. . . .

Junilu is right; make the class and both its fields final, so it will be immutable. That way there will be no risk of users forgetting to set the message. You don't want people to use your Message object and have a method return null.

[Jack Tauson]

Junilu Lacar wrote:I concur with Lou's suggested approach, but without the setter for message type. Do it all in the constructor and make the class immutable.

Also, consider renaming the class to simply Message: I don't see any reason to name it "MessageReturn" - you're probably thinking "It's a massage that I return, so 'MessageReturn'". No. It's a message plain and simple. The fact that you return it from a method is irrelevant to the name choice.

Could you tell me your constructor approach by pasting a few lines of code? Thanks!

[Junilu Lacar]

public class Message {
   private final String text;
   private final MessageType type;

   public Message(MessageType type, String text) {
      this.type = type;
      this.text = text;
   }

   // no setters, just getters
   ...
}

[Paul Clapham]

That is much nicer than the original code, but it assumes that all of the peculiar complexity of the original code is unnecessary. Which may in fact be true, but I didn't want to assume that.

[Campbell Ritchie]

Surely you would want the class to be final, Junilu?

[Lou Hamers]

Yeah going immutable is a good suggestion. In fact if we're going for immutable on a recent version of Java:

public record Message(MessageType messageType, String message) {
}

Get rid of the getters too!!!

How about that for "concise"?    Even the millennials should like that one (I think).

[Campbell Ritchie]

I'd forgotten about records.

Lou Hamers wrote:. . . Get rid of the getters too!!!  . . .

But a record always has getters; they are simply named differently.

[Lou Hamers]

True, I just meant the code itself, not the implicit getters.