3

I often use objects to store properties of entities I get from a database. I use private variables in the object and then use getters and setters to set the values. So to initialize the object I call all the setters of the object. But it is impossible to keep track of all the setters and I often forget to set some variable. Is there a way to make compulsory the calling of the setters. I can definitely initialize the variables making use of a constructor instead, but usage of a constructor to set 10-12 properties makes the code look shabby. I wish to use interfaces and sub classes in such a way that they work the way it is necessary to implement all the methods of an interface but here not implement but call them instead.

com.mysql.jdbc.PreparedStatement getInternships = (PreparedStatement) Connection.con.prepareCall("CALL getInternships()");

        rs=getInternships.executeQuery();
        Internship current;
        while(rs.next()){
            current=new Internship();
            current.setId(Integer.parseInt(rs.getString("id")));
            current.setTitle(rs.getString("title"));

            current.setCategory(rs.getString("category"));
            current.setOpening(rs.getDate("opening"));
            current.setClosing(rs.getDate("closing"));
            current.setDuration(Integer.parseInt(rs.getString("duration")));
            current.setStatus(rs.getString("status"));
            current.setApplicants(Integer.parseInt(rs.getString("applicants")));
            current.setSeats(Integer.parseInt(rs.getString("seats")));
            current.setHired(Integer.parseInt(rs.getString("hired")));
            list.add(current);
        }

The internship class

package internships;
import java.util.Date;
public class Internship {
private String title,category,status,about,eligibility,information;
private int id,duration,applicants,seats,hired;
private Date opening,closing;
/**
 * @return the opening
 */
public Date getOpening() {
    return opening;
}
/**
 * @param opening the opening to set
 */
public void setOpening(Date opening) {
    this.opening = opening;
}
/**
 * @return the hired
 */
public int getHired() {
    return hired;
}
/**
 * @param hired the hired to set
 */
public void setHired(int hired) {
    this.hired = hired;
}
/**
 * @return the seats
 */
public int getSeats() {
    return seats;
}
/**
 * @param seats the seats to set
 */
public void setSeats(int seats) {
    this.seats = seats;
}
/**
 * @return the applicants
 */
public int getApplicants() {
    return applicants;
}
/**
 * @param applicants the applicants to set
 */
public void setApplicants(int applicants) {
    this.applicants = applicants;
}
/**
 * @return the closing
 */
public Date getClosing() {
    return closing;
}
/**
 * @param closing the closing to set
 */
public void setClosing(Date closing) {
    this.closing = closing;
}
/**
 * @return the duration
 */
public int getDuration() {
    return duration;
}
/**
 * @param duration the duration to set
 */
public void setDuration(int duration) {
    this.duration = duration;
}
/**
 * @return the category
 */
public String getCategory() {
    return category;
}
/**
 * @param category the category to set
 */
public void setCategory(String category) {
    this.category = category;
}
/**
 * @return the status
 */
public String getStatus() {
    return status;
}
/**
 * @param status the status to set
 */
public void setStatus(String status) {
    this.status = status;
}
/**
 * @return the title
 */
public String getTitle() {
    return title;
}
/**
 * @param title the title to set
 */
public void setTitle(String title) {
    this.title = title;
}
/**
 * @return the id
 */
public int getId() {
    return id;
}
/**
 * @param id the id to set
 */
public void setId(int id) {
    this.id = id;
}
/**
 * @return the about
 */
public String getAbout() {
    return about;
}
/**
 * @param about the about to set
 */
public void setAbout(String about) {
    this.about = about;
}
/**
 * @return the eligibility
 */
public String getEligibility() {
    return eligibility;
}
/**
 * @param eligibility the eligibility to set
 */
public void setEligibility(String eligibility) {
    this.eligibility = eligibility;
}
/**
 * @return the information
 */
public String getInformation() {
    return information;
}
/**
 * @param information the information to set
 */
public void setInformation(String information) {
    this.information = information;
}

}
Joey Pinto
  • 1,735
  • 1
  • 18
  • 34
  • Possible duplicate of [How do I initialize classes with lots of fields in an elegant way?](http://stackoverflow.com/questions/33106041/how-do-i-initialize-classes-with-lots-of-fields-in-an-elegant-way) – Andreas Fester Jan 15 '16 at 08:48
  • Check out [Builder Patterns with a Twist](http://stackoverflow.com/a/33210006/2775450), while it can make calling all methods mandatory, I am not sure if it can actually make your code cleaner... – Codebender Jan 15 '16 at 08:53
  • That post did not help me much as for me I do not intend to use default values but instead want to necessitate the entry i.e. use of all the setters when I instantiate the object – Joey Pinto Jan 15 '16 at 08:54
  • I suggest not having to call the setters at all - and simply iterating the ResultSet's columns and calling setters automagically. See below answer. Since calling all those setters is boring and error prone, *automate it* instead of checking whether you did it right or not. – tucuxi Jan 15 '16 at 17:08

4 Answers4

2

Here is a suggestion:

  • Define a "data" class containing all fields (category, opening, closing, duration, etc.), e.g. InternshipData
  • In the Internship class, define a setter method: setData(InternshipData data). Use this setter whenever you need to configure an Internship.
  • Inside the setter, check that all the fields of the supplied InternshipData object are properly initialised. If they are not, you can throw an exception, or just return false from the setter and check the returned value externally.

(Note, a similar approach can be used if you want to pass the data to the Internship constructor)

This lets you check for all the fields in a single place (the setData) method, which is otherwise difficult to do with your current approach because you have a bunch of setters and you don't know the order in which they are going to be called from client code.

Grodriguez
  • 21,501
  • 10
  • 63
  • 107
  • This is a runtime error check and I guess he is looking for a compile-time option... – Codebender Jan 15 '16 at 08:58
  • 1
    @Codebender He does not explicitly say that he's looking for a compile-time option. He just says that he "often forgets to set some variable". With his current approach this is not checked anywhere. With my suggested approach this would be at least detected at runtime. – Grodriguez Jan 15 '16 at 09:24
  • How would you distinguish, inside your `setData` method, between an integer field set to 0 in the `InternshipData` object and an uninitialized field (which would start with a default value of 0)? – tucuxi Jan 15 '16 at 10:46
  • @tucuxi Two options. If there are "invalid" values (e.g. -1), init the field to that invalid value in the field declaration itself, so that you can easily check later if a valid value has been assigned. If all values are valid then you'll need to use a different type. For example a non-primitive `Integer`, which will be `null` initially. – Grodriguez Jan 15 '16 at 11:31
  • Null values could also be legal in the DB - the question still stands. – tucuxi Jan 15 '16 at 13:33
  • The answer still stands as well: "Use a datatype which allows values that are not legal in the DB". Your original question referred to primitive ints (default value of 0) so I suggested a non-primitive `Integer` field. If null is also legal, then use some other datatype; for example, a `Long`. – Grodriguez Jan 15 '16 at 13:49
  • I am looking for a compile time option. – Joey Pinto Jan 15 '16 at 13:54
  • 1
    @JoeyPinto (You should probably specify that in the question then). If you need a compile time option you need to either resort to external tools to analyze/validate your code or just stick with the "10-12 arg constructor" approach. – Grodriguez Jan 15 '16 at 14:48
1

This is another run-time idea -- the only compile-time answer I can think of is to introduce new PMD / FindBugs rules, and have your IDE run those on each save.

You would need to declare, for each class where you want to avoid "uninitialized" values, a BitSet to track which fields have been set, and an enum called Fields that would count fields for you and associate field-names to their corresponding BitSet index.

// keeps track of initialized fields
private BitSet initialized = new BitSet(Fields.values().length);

// field-names not included here will not be considered included in BitSet
enum Fields {
   title,category,status,about,eligibility,information,
   id,duration,applicants,seats,hired,
   opening,closingopening
}

This would only return true if all fields in Fields have been set at least once:

public void isFullyInitialized() {
   return initialized.cardinality() == initialized.size();
}

And this would be a sample setter:

/**
 * @param opening the opening to set
 */
public void setOpening(Date opening) {
   this.opening = opening;
   initialized.set(Fields.opening.ordinal()); // <- do this in all setters
}

You could then assert(isFullyInitialized()) at the end of an initialization run, and it would throw an ugly assertion error if you left any field un-initialized. It does have a run-time cost though: a BitSet for each object-instance, and slightly slower setXyz() method calls.

You can avoid the per-object overhead of having all those bitsets if you combine this answer with Grodriguez's answer: have an intermediate object do all the accounting (with the bitsets and so on), and verify its integrity using the bitset during the setData() operation. If the operation succeeds, the intermediate object can be safely discarded. However, I cringe when thinking of all the boilerplate code that would be required.

Community
  • 1
  • 1
tucuxi
  • 17,561
  • 2
  • 43
  • 74
1

How about this simple reflection fun?

We will be using a lighter variant of your internship class. Mark fields that should not be null in the end with @NotNull. Yes, this won't work with primitive fields, but unless forced by a constructor, your fields can have the state undefined, which is expressed by the value null. Using primitives would be hiding that fact.

class Internship {
    @NotNull
    private String name;

    private String status;

    public Internship() {

    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public String getStatus() {
        return status;
    }

    public void setStatus(String status) {
        this.status = status;
    }
}

Here is a complete test of the annotation and it's processor, the beautifully named method validateNotNullFields. It gets every field of the object, even private fields and fields defined in super classes. If the annotation is found on a field that holds null as value it throws an IllegalArgumentException. If you like my solution you will probably want to adapt this and properly output the fields name or even chose a solution without the exception.

public class Test {
    public static void main(String... args) {
        Internship a = new Internship();
        a.setName("Karl");
        a.setStatus(null);
        validateNotNullFields(a);

        System.out.println("ok");

        try {
            Internship b = new Internship();
            b.setName(null);
            b.setStatus(null);
            validateNotNullFields(b); // throws IllegalStateException!
            System.out.println("not ok");
        } catch (IllegalStateException ise) {
            System.out.println("ok");
        }

    }

    static void validateNotNullFields(Object candidate) throws IllegalStateException {
        if (candidate == null) {
            throw new IllegalArgumentException("argument candidate must not be null!");
        }
        Class<?> clazz = candidate.getClass();

        while (!Object.class.equals(clazz)) {
            Field[] fields = clazz.getDeclaredFields();
            for (Field field : fields) {
                Annotation annotation = field.getAnnotation(NotNull.class);
                if (annotation == null) {
                    continue;
                }

                field.setAccessible(true);

                try {
                    if (field.get(candidate) == null) {
                        throw new IllegalStateException(
                                "Field " + field.getName() + " must not be null at this point!");
                    }
                } catch (IllegalArgumentException | IllegalAccessException ex) {
                    throw new RuntimeException(ex);
                }
            }
            clazz = clazz.getSuperclass();
        }
    }
}
ASA
  • 1,911
  • 3
  • 20
  • 37
1

Instead of checking, after the fact, that you have set every single field... you could use a method that reads all fields for you every time it is called. No more memory lapses:

static void load(Object target, ResultSet rs) throws Exception {
    Class<?> clazz = target.getClass();

    ResultSetMetaData rsmd = rs.getMetaData();
    for (int i=0; i<rsmd.getColumnCount(); i++) {
        Field field = clazz.getDeclaredField(rsmd.getColumnName(i+1));
        field.setAccessible(true);
        int type = rsmd.getColumnType(i+1);
        switch (type) {
            case Types.DATE: {
                field.set(target, rs.getDate(i+1));
            }
            case Types.VARCHAR: {
                field.set(target, rs.getString(i+1));
            }
            case Types.BIGINT: {
                field.set(target, rs.getInt(i+1));
            }
            default: {
                throw new IllegalArgumentException(
                   "Unhandled field type: " + type);
            }
        }
    }
}

There may be typos in the above code (I haven't tried to run it) - but it amounts to a poor man's JPA: for each column in a ResultSet of a few key types (add your own!), it attempts to set the corresponding field of the same name in the passed-in object. Assuming all names and types match, there will be no fields left unset.

Performance may not be great (you could cache those fields and avoid setting them accessible over and over again) - but if you wanted cleaner code, you would be using JPA anyway.

tucuxi
  • 17,561
  • 2
  • 43
  • 74