Should i make this method static?

Question

I am writing an android application that parses an xml feed about car parks. I've created a separate class that receives an xml feed as a string, this class will then parse the string, creating objects for each car park found in the feed. I will use this each time I refresh the xml feed and need to parse it, should I make this class static or not. I am still not 100% sure on when to make something static or not.

My code:

public class XMLParser2 {

private static CarPark carPark;
private static ArrayList<CarPark> carParkListings;

// Parse XML string and save each car park object created within an Arraylist collection
public static ArrayList<CarPark> parseXML(String xml) throws XmlPullParserException, IOException {

    XmlPullParserFactory factory = XmlPullParserFactory.newInstance();
    factory.setNamespaceAware(true);
    XmlPullParser xpp = factory.newPullParser();
    xpp.setInput(new StringReader(xml));
    int eventType = xpp.getEventType();
    carParkListings = new ArrayList<CarPark>();

    while (eventType != XmlPullParser.END_DOCUMENT) {

        if (eventType == XmlPullParser.START_TAG) {
            if (xpp.getName().equalsIgnoreCase("situation")) {
                // Create new CarPark object to hold car park data
                carPark = new CarPark();
            }
            // Check start tag for each element wanting to obtain and save in the 
            // carPark object using its associated set method
            if (xpp.getName().equalsIgnoreCase("carparkstatus")) {
                String status = xpp.nextText();
                if (status.matches("enoughSpacesAvailable"))
                    carPark.setCarParkActive("Open");
                else if (status.matches("carParkClosed"))
                    carPark.setCarParkActive("Closed");
                else
                    carPark.setCarParkActive("Full");
            }
            if (xpp.getName().equalsIgnoreCase("overallstarttime")) {
                carPark.setDateCreated(xpp.nextText());
            }
            if (xpp.getName().equalsIgnoreCase("latitude")) {
                Double latitude = Double.parseDouble(xpp.nextText());
                carPark.setLatitude(latitude);
            }
            if (xpp.getName().equalsIgnoreCase("longitude")) {
                Double longitude = Double.parseDouble(xpp.nextText());
                carPark.setLongtitude(longitude);
            }
            if (xpp.getName().equalsIgnoreCase("carparkidentity")) {
                String[] identity = xpp.nextText().split("\\:");
                carPark.setCarParkName(identity[0]);
                carPark.setCarParkID(identity[1]);
            }
            if (xpp.getName().equalsIgnoreCase("occupiedspaces")) {
                int occupiedSpaces = Integer.parseInt(xpp.nextText());
                carPark.setOccupiedSpaces(occupiedSpaces);
            }
            if (xpp.getName().equalsIgnoreCase("totalcapacity")) {
                int totalCapacity = Integer.parseInt(xpp.nextText());
                carPark.setTotalSpaces(totalCapacity);
            }
        }
        // Check if the tag is an end tag
        else if (eventType == XmlPullParser.END_TAG) {
            if (xpp.getName().equalsIgnoreCase("situation")) {
                // Calculate total spaces free
                carPark.setFreeSpaces();
                // Add CarPark class object to carParkListings ArrayList collection
                carParkListings.add(carPark);
            }
        }
        // Move to next event
        eventType = xpp.next();
    }
    // Return ArrayList collection
    return carParkListings;
}

}

In the title you say method, and in the question class, which one do you mean? If you mean class, this might help http://stackoverflow.com/q/7486012/612920 — Mansuro, May 13 '15 at 07:43
try to post your question here: http://codereview.stackexchange.com/ — alex, May 13 '15 at 07:50

score 2 · Answer 1 · answered May 13 '15 at 07:40

2

Better make it non static. You manipulate objects inside the class and if multiple classes call the same method in the same time you might have problems. For example one class calls this method and it start executing and in the same time another class calls the method and reaches the line carPark = new CarPark(); before the first one has finished. It will compromise the results.

Better use instances and make the fields carPark and carParkListings non static.

answered May 13 '15 at 07:40

Veselin Davidov

7,031
1
15
23

I mostly agree. But the main problem is that these fields, static or not, shouldn't be fields in the first place. They should be local variables. – JB Nizet May 13 '15 at 07:42

score 1 · Answer 2 · answered May 13 '15 at 07:47

If you don't want to create any instance of XMLParser2 class when you call the parseXML method, you can make it static.

But what you should not do is use the global static property in your parseXML method.

private static CarPark carPark;
private static ArrayList<CarPark> carParkListings;

Those two variables need to define local inside the method only.

score 1 · Answer 3 · answered May 13 '15 at 07:53

If you do not maintain any other states in XMLParser2, I think it is a good candidate for a static method since it looks like an utility method to me, but you should make the carPark and carParkListings local variables of the method itself. One should use a static method if it does not use any instance variables. You only need to specify input as parameter to the method and expect to get back some result.

Should i make this method static?

3 Answers3