2
\$\begingroup\$

I have written a Java class that parses Shoutcast 2 stats XML. It uses JDOM to parse the XML. I have only basic knowledge of Java and I wrote this class in about 3 hours. It works, but I don't know if I've done everything right, or how efficient it is. I intend to use this class in an Android app I am working on. You can view the class below. Any feedback to make this class better and more efficient is greatly appreciated.

package com.brandsonic.shoutcastParser;

import java.io.IOException;
import org.jdom2.Document;
import org.jdom2.Element;
import org.jdom2.JDOMException;
import org.jdom2.input.SAXBuilder;

public class ShoutcastParser {

private Element root;

private int currentListeners = -1;
private int peakListeners = -1;
private int maxListeners = -1;
private int uniqueListeners = -1;
private int averageTime = -1;
private String serverGenre;
private String serverGenre2;
private String serverGenre3;
private String serverGenre4;
private String serverGenre5;
private String serverUrl;
private String serverTitle;
private String songTitle;
private String dJ;
private int streamHits = -1;
private int streamStatus = -1;
private int backupStatus = -1;
private int streamListed = -1;
private String streamPath;
private int streamUpTime = -1;
private int bitrate = -1;
private String content;
private String version;

public ShoutcastParser(String URL) {
    parseXML(URL);
}

public void parseXML(String URL) {
    SAXBuilder builder = new SAXBuilder();

    Document xml = null;
    try {
        xml = builder.build(URL);
    } catch (JDOMException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }

    root = xml.getRootElement();
    setVars();
}

public void setVars() {

    currentListeners = Integer.parseInt(root.getChildText("CURRENTLISTENERS"));
    peakListeners = Integer.parseInt(root.getChildText("PEAKLISTENERS"));
    maxListeners = Integer.parseInt(root.getChildText("MAXLISTENERS"));
    uniqueListeners = Integer.parseInt(root.getChildText("UNIQUELISTENERS"));
    averageTime = Integer.parseInt(root.getChildText("AVERAGETIME"));

    serverGenre = root.getChildText("SERVERGENRE");
    serverGenre2 = root.getChildText("SERVERGENRE2");
    serverGenre3 = root.getChildText("SERVERGENRE3");
    serverGenre4 = root.getChildText("SERVERGENRE4");
    serverGenre5 = root.getChildText("SERVERGENRE5");
    serverUrl = root.getChildText("SERVERURL");
    serverTitle = root.getChildText("SERVERTITLE");
    songTitle = root.getChildText("SONGTITLE");
    dJ = root.getChildText("DJ");
    streamHits = Integer.parseInt(root.getChildText("STREAMHITS"));
    streamStatus = Integer.parseInt(root.getChildText("STREAMSTATUS"));
    backupStatus = Integer.parseInt(root.getChildText("BACKUPSTATUS"));
    streamListed = Integer.parseInt(root.getChildText("STREAMLISTED"));
    streamPath = root.getChildText("STREAMPATH");
    streamUpTime = Integer.parseInt(root.getChildText("STREAMUPTIME"));
    bitrate = Integer.parseInt(root.getChildText("BITRATE"));
    content = root.getChildText("CONTENT");
    version = root.getChildText("VERSION");

}

public int getCurrentListeners() {
    return currentListeners;
}

public int getPeakListeners() {
    return peakListeners;
}

public int getMaxListeners() {
    return maxListeners;
}

public int getUniqueListeners() {
    return uniqueListeners;
}

public int getAverageTime() {
    return averageTime;
}

public String getServerGenre() {
    return serverGenre;
}

public String getServerGenre2() {
    return serverGenre2;
}

public String getServerGenre3() {
    return serverGenre3;
}

public String getServerGenre4() {
    return serverGenre4;
}

public String getServerGenre5() {
    return serverGenre5;
}

public String getServerURL() {
    return serverUrl;
}

public String getServerTitle() {
    return serverTitle;
}

public String getSongTitle() {
    return songTitle;
}

public String getDJ() {
    return dJ;
}

public int getStreamHits() {
    return streamHits;
}

public int getStreamStatus() {
    return streamStatus;
}

public int getBackupStatus() {
    return backupStatus;
}

public int getStreamListed() {
    return streamListed;
}

public String getStreamPath() {
    return streamPath;
}

public int getStreamUpTime() {
    return streamUpTime;
}

public int getBitrate() {
    return bitrate;
}

public String getContent() {
    return content;
}

public String getVersion() {
    return version;
}

}
\$\endgroup\$
  • \$\begingroup\$ You shouldn't be holding on to root when you don't need it anymore. Do away with private Element root; and change to protected void setVars( Element root ) { .... This will help save the memory needed to keep the root element around all the time. \$\endgroup\$ – JimmyB Mar 23 '16 at 16:50
1
\$\begingroup\$

Keep constructors lean

The job of a constructor is to create and initialize an object so that it's ready to use.

The constructor of ShoutcastParser does more than that. It opens a document and parses it. By the time this constructor is done, there's nothing left to parse.

In general it's recommended to keep constructors lean.

Single responsibility principle

ShoutcastParser has two responsibilities:

  • Parse stats from a given URL
  • Container of stats

It would be better to separate these responsibilities into two distinct classes. Together with the earlier suggestion about constructor, the usage will change to this:

ShoutcastParser parser = new ShoutcastParser(url);
ShoutcastStats stats = parser.parse();

Suppress the auto-pilot

When you have a bunch of private fields, it may feel tempting to create getters for all of them. Even if you don't need all those fields right now. Just add all the getters, because "why not", or it's " easy enough to do ", or because " probably it will be useful at some point". It's like the auto pilot has taken over, writing pointless code while the brain is asleep.

I suggest to only implement getters that you really need. But before that, only field that you really need. The more fields and public methods you have, the greater the burden of maintenance. It's good to delay implementation as much as possible. It may very well turn out that you will never need half of this stuff.

Flawed exception handling

The exception handling in parseXML is flawed. It catches exceptions and prints stack trace, but then continues running anyway. It will run into a null pointer exception on the next statement and crash the application.

You could return after printing the stack trace, but then, that's almost the same as not catching the exception at all, and letting the application crash. That will print the stack trace too. And it will be better, because the exit code will be automatically non zero, correctly indicating failure.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.