5

In Java, I've to set a POJO class with values. However to decide which setter function to be used, I've to depend on if condition. My current code looks as follow:

// Code written in a function which is called within a loop, while parsing xml file.
if (name.equals("dim1")) {
    line.setDim1Code(Integer.parseInt(value));
} else if (name.equals("dim2")) {
    line.setDim2Code(Integer.parseInt(value));
} else if (name.equals("debitcredit")) {
    line.setDebitOrCredit(value);
} else if (name.equals("basevalue")) {
    line.setBasevalue(Integer.parseInt(value));
} else if (name.equals("rate")) {
    line.setRate(Integer.parseInt(value));
} else if (name.equals("value")) {
    line.setValue(Integer.parseInt(value));
} else if (name.equals("description")) {
    line.setDescription(value);
} else if (name.equals("vatbasetotal")) {
    line.setVatBaseTotal(value);
} else if (name.equals("vattotal")) {
    line.setVatTotal(value);
}

This is just an example but I've 70+ such properties to be set. My code is working but I wonder if it is right way of doing things?

AFAIK, such code is against coding best practices. How can we optimise this code in Java? What is Java best practice to deal with such code?

resueman
  • 10,572
  • 10
  • 31
  • 45
Kapil Sharma
  • 10,135
  • 8
  • 37
  • 66
  • 3
    You can use the `switch case`! – Rahul Tripathi Aug 27 '15 at 13:38
  • 8
    If I understand correctly, your code is essentially de-serializing XML data into a POJO manually, line by line, with a condition for each node/property. There is a lot more wrong with that than just the huge if/else. I'd say you're better off finding some high-level serialization framework such as Jackson, and rewrite the lot of it. – Mena Aug 27 '15 at 13:39
  • If is solving my problem. Switch will have same problem like if. Code will still look ugly. – Kapil Sharma Aug 27 '15 at 13:39
  • Can you expand on the difference between, for example, `setVatTotal()` and `setVatBaseTotal()`? I think it would be helpful if you posted two examples for comparison. – Rainbolt Aug 27 '15 at 13:42
  • For this issue switch case is better than if else. But that will not solve your problem. Anyway better to put this block of code to a separate method. That might make your code readable. Better to read Clean Code by Robert Martin for solving these kind of issues. Thanks – isurujay Aug 27 '15 at 13:44
  • What do you do with the POJO once its 70+ properties are set? – WilQu Aug 27 '15 at 13:46
  • Ahh lot of unrelated questions. 'vatTotal' and 'vatBaseTotal' are two of many vat fields. Its functional requirement of project. POJO is just used to hold data got from webservices as it is passed to few other classes/methods. Can't pass 70+ parameters, right? – Kapil Sharma Aug 27 '15 at 13:50
  • So your `name` and `value` come from the values of an HTTP request? Generally, if you have an object with 70 fields, it's probably not designed correctly, and you should be breaking it down into smaller objects, each representing some logical entity. – RealSkeptic Aug 27 '15 at 13:54
  • Yes, unfortunately web service is returning lot of data. Making smaller classes seems good. Also @NickLamp, sorry for my wrong word selection which caused negative vote fro you. I at least removed my downvote. Seems reflection or Jackson will help me here. Let me check Jackson which seems pretty from libik's answer. Thanks everyone for answers and comments. – Kapil Sharma Aug 27 '15 at 14:00
  • 1
    This may be a dumb question, but can POJO objects have a list of properties instead? I am having trouble wrapping my head around what a POJO is, but it seems like a 70+ item list of key-value pairs would solve the problem. – Rainbolt Aug 27 '15 at 14:01
  • A POJO is a Plain Old Java Object, it could have a list of 70 properties if you fancied. With getters and setters for each one, but that's what he has as far as I can see. When you say list do you mean a List, or are meaning a Map? – Andrew Aitken Aug 27 '15 at 14:04
  • @AndrewAitken I meant "list" as in *1 collection* with *1 getter* and *1 setter* (as opposed to *70 properties* with *70 getters* and *70 setters*... wow). I think that Map is an ideal choice, because indexing into a Map is an O(1) operation. But apparently someone disagrees, because my answer already has a downvote. – Rainbolt Aug 27 '15 at 14:16

4 Answers4

5

It is actually something, which should be done automatically based on annotations by some library like Jackson 2.0+ or something similar (I am parsing only JSON so far)

Then the object looks like this :

@XmlAccessorType(XmlAccessType.FIELD)
    public class Employee
    {
        @XmlAttribute
        @XmlID
        protected String id;

        @XmlAttribute
        protected String name;

        @XmlIDREF
        protected Employee manager;

        @XmlElement(name="report")
        @XmlIDREF
        protected List<Employee> reports;

        public Employee() {
            reports = new ArrayList<Employee>();
        }
    }
libik
  • 22,239
  • 9
  • 44
  • 87
1

You can try Java Architecture for XML Binding (JAXB), here you have a tutorial. i.e:

    File file = new File("C:\\file.xml");
    JAXBContext jaxbContext = JAXBContext.newInstance(Pojo.class);

    Unmarshaller jaxbUnmarshaller = jaxbContext.createUnmarshaller();
    Pojo pojo= (Pojo) jaxbUnmarshaller.unmarshal(file);
Daniel Boncioaga
  • 336
  • 1
  • 13
0

What you are probably looking to do is write a dynamic Getter/Setter. A quick Google search will give you a large number of options and approaches, but here is a couple I found quickly.

How to define dynamic setter and getter using reflection?

Invoking setter method using java reflection

I personally like the look of BeanUtils. Simple and easy to understand.

Community
  • 1
  • 1
Sh4d0wsPlyr
  • 948
  • 12
  • 28
0

Rather than having a separate field and setter for each property, create a dictionary to hold all of your properties. Borrowing code from How do you create a dictionary?:

private Map<String, String> properties;

// Constructor
public MyClass() {
    properties = new HashMap<String, String>();
}

// Setter
public string setProperty(String name, String value) {
    properties.put(name, value);
}

// Getter
public string getProperty(String name) {
    return properties.get(name); // May return null. You may want to handle that.
}

Now all 70+ of your properties have only one getter and setter. No giant if-else or switch block.

With a dictionary, you lose some of the contracts that properties provide. For example, I could add a property to the dictionary that shouldn't exist. You can enforce these restrictions yourself (for example, by creating a list of allowed property names and refusing to set any property that is not in the list).

Community
  • 1
  • 1
Rainbolt
  • 3,542
  • 1
  • 20
  • 44