Error-handling / Self-Validating mechanism

Question

The below idea seemed to look clean, to allow the object itself to validate its values for different scenarios.

For eg:

• While creating the object, the value of Object1 and Object2 in SelfValidator object should be validated for not null.
• At one scenario like inserting the SelfValidator object to Database, validation of value1 as non-zero has to be done, which is not necessary for an Update. A new validate function: validateForInsert(), which will validate and set the error data, can be included to SelfValidator class.

But I am sure, there might be some drawbacks, design flaws, which I would like to understand and improve.

ErrorInfo.java

public class ErrorInfo { String errorMessage = ""; public void setErrorMessage(String errorMessage) { this.errorMessage = new StringBuffer(this.errorMessage).append("\n+").append(errorMessage).toString(); } } 

SelfValidator.java

public class SelfValidator { String object1; String object2; int value1; ErrorInfo errorInfo; public ErrorInfo getErrorInfo() { return errorInfo; } public SelfValidator(String object1, String object2, int value1) { super(); this.object1 = (object1 == "")?null: object1; validate(this.object1, "object1"); this.object2 = (object2 == "")?null: object2; validate(this.object2, "object2"); this.value1 = value1; } private void validate(String value, String dataMember) { if(value == null){ if(errorInfo == null){ errorInfo = new ErrorInfo(); } errorInfo.setErrorMessage(dataMember +" value is invalid"); } } } 

Demonstrator.java

public class Demostrator { public static void printValidOrNot(ErrorInfo errorInfo){ if(errorInfo == null){ System.out.println("Object valid"); //Proceed with manipulating the object }else{ System.out.println(errorInfo.errorMessage); // Break and handle the error. } } public static void main(String[] args) { SelfValidator selfValidatingObject1 = new SelfValidator("Test", "Test", 1); printValidOrNot(selfValidatingObject1.getErrorInfo()); SelfValidator selfValidatingObject2 = new SelfValidator("", "", 1); printValidOrNot(selfValidatingObject2.getErrorInfo()); } } 

Answer 1

isValid Method

If you have a validator, I would expect a method to check if it is valid.

If you add an isValid method, you would get rid of the errorInfo == null check, which isn't a good idea. It exposes how the validator works internally, and relies on this never changing.

errorMessage Type

Why is your error message a String? You are creating a new StringBuffer and calling toString every time an error is appended. Just make errorMessage a StringBuffer or StringBuilder and call toString when the error message is retrieved (which should be done via a getter, not direct access).

Naming

setErrorMessage: What this actually does is appendErrorMessage

Spacing

I know it's knit-picking, but please follow the general conventions regarding spacing. Before and after ?, :, and + should be a space.

General Idea

I'm not too sure what this is good for (maybe I'm missing something?). It doesn't seem like the SelfValidator class can be reused for any other data. The three fields and their types are hardcoded, as is the validation mechanism (no empty string and no null allowed). Couldn't all your code be replaced with something like this:

 private boolean isValid = true; public MyClass(String object1, String object2, int value1) { super(); if (object1 == null || object2 == null || object1.equals("") || object2.equals("")) { this.isValid = false; } } public isValid() { return isValid; } 

It seems just as flexible as your approach, but is easier to read.

Answer 2

Several notes about ErrorInfo

this.errorMessage = new StringBuffer(this.errorMessage).append("\n+").append(errorMessage).toString(); 

why did you choose StringBuffer? Consider these options:

1

this.errorMessage += "\n+" + errorMessage; 

Here you don't create a heavy StringBuffer object (which is appropriate when you concatenate lots of Strings)

2

public class ErrorInfo { private StringBuffer errorMessage; public ErrorInfo { errorMessage = new StringBuffer; } public String getErrorMessage { return errorMessage.toString()} 

Here you create StringBuffer only once.

Another note is that your error message will start with '\n+'. Is it what you want? Or probably smth like this:

this.errorMesage += this.errorMessage.length() == 0 ? errorMessage : "\n+" + errorMessage 

About validation. One point is how the constructor will know what type of validation you need. In your example, if you add a function 'validForUpdate', you'll be able to create an instance of SelfValidator, which won't pass validation for update.

The method validate only checks for not-null. What if you need to check string length for some parameters?

If your object should be immutable, you might consider creating an object factory with methods, which allow you to create an instance, valid for your purpose.

public class SelfValidator { .... public static SelfValidator getInstanceWithNotNullValues(String object1, String object2, int value1) throws IllegalArgumentException { boolean valid = true; StringBuffer errors = new StringBuffer(); if (object1 == null || object1.equals("")) { errors.append("Object 1 is null\n"); valid = false; } if (object2 == null || object2.equals("")) { errors.append("Object 2 is null\n"); valid = false; } if (valid) return new SelfValidator(object1, object2, value1); else throw new IllegalArgumentException(errors.toString()); } } public static SelfValidator getInstanceForUpdate(String object1, String object2, int value1) throws IllegalArgumentException { boolean valid = true; StringBuffer errors = new StringBuffer(); if (your condition) { errors.append("Object 1 doesn't meet the requirements\n"); valid = false; } if (your condition) { errors.append("Object 2 doesnt' meet the requirements\n"); valid = false; } if (valid) return new SelfValidator(object1, object2, value1); else throw new IllegalArgumentException(errors.toString()); } } } 

Answer 3

A few more points:

---

String errorMessage = ""; 

You shouldn't expose this internal information. Make it a private field and expose it through a getter. Also, as pointed out by others, String might not be the best data type for it. I'd actually prefer a List (Java 8):

public class ErrorInfo { private final List<String> messages = new ArrayList<>(); public ErrorInfo addMessage( String message ) { messages.add( message ); return this; } public String getMessages() { return String.join( "\n", messages ); } } 

---

String object1; String object2; int value1; 

This is horrible. It is virtually impossible to understand what these variables stand for. Always use meaningful names for everything you write – from variables to method names to class names.

Also, make these fields private and, if possible, final. Always use the most restrictive level of visibility and only expose your public API. Everything else makes maintaining as well as debugging harder.

---

public SelfValidator(String object1, String object2, int value1) { super(); this.object1 = (object1 == "")?null: object1; validate(this.object1, "object1"); this.object2 = (object2 == "")?null: object2; validate(this.object2, "object2"); this.value1 = value1; } 

First off, the same thing about names goes for parameters. You also don't need to explicitly call the empty super constructor, the compiler will do this for you.

The absolute no-go in here is comparing Strings with ==. To compare strings, you always need to use equals() as == will only compare references which fails as soon as the strings aren't interned the right way.

However, no need to compare at all here. Strings have an isEmpty() method you can call. However, you might want to watch out for null values.

---

private void validate(String value, String dataMember) { if(value == null){ if(errorInfo == null){ errorInfo = new ErrorInfo(); } errorInfo.setErrorMessage(dataMember +" value is invalid"); } } 

Why do you only create errorInfo at this point? You might as well create the instance right where you declare it. This also allows you to make it final.

I also don't get the point of this method at all. You never seem to use it. It also seems odd that it only does something if the first parameter is null.

---

Overall, to be honest, I don't really get the point of what you're trying to do anyway, so I'll restrict myself to comment on coding style and best practices here.