0
\$\begingroup\$

The exercise is from a Java-course in which I'm enrolled.

The classes Book and APIResponseParser were provided by the instructor. For Book I had to add just a few getter- and setter-methods.

For APIResponseParser the instance-version of parse() had to be written. The static version of parse() had to be completed. Please see the comments within the code.

These values are then used for to instantiate an object of the class Book. For some values is additional type conversion necessary.

Here's my code:

public class Book {     
    private String title;
    private String author;
    private int publicationYear;
    private double averageRating;
    private int ratingsCount;
    private String imageUrl;

    // Add getters & setters for author, averageRating, and ratingsCount

    public String getTitle() {
        return title;
    }
    public void setTitle(String title) {
        this.title = title;
    }

    public String getAuthor() {
        return author;
    }
    public void setAuthor(String author) {
        this.author = author;
    }

    public int getPublicationYear() {
        return publicationYear;
    }
    public void setPublicationYear(int publicationYear) {
        this.publicationYear = publicationYear;
    }

    public String getImageUrl() {
        return imageUrl;
    }
    public void setImageUrl(String imageUrl) {
        this.imageUrl = imageUrl;
    }

    public void setAverageRating(double rating) {
        this.averageRating = rating;
    }

    public double getAverageRating() {
        return averageRating;
    }

    public void setRatingsCount(int ratingsCount) {
        this.ratingsCount = ratingsCount;
    }

    public int getRatingsCount() {
        return ratingsCount;
    }
}

// ----- Main Class with the String-parser ---------
public class APIResponseParser {

    /**
     * Parses the input text and returns a Book instance containing
     * the parsed data. 
     * @param response text to be parsed
     * @return Book instance containing parsed data
     */
    public static Book parse(String response) {
        // -- Start: Provided by instructor --
        Book book = new Book();
        String endRule = "<";

        String startRule = "<title>";

        APIResponseParser arp = new APIResponseParser();
        String title = arp.parse(response, startRule, endRule); 
        book.setTitle(title);
        // -- End: Provided by instructor --

        startRule = "<name>";
        String name = arp.parse(response, startRule, endRule);
        book.setAuthor(name);

        startRule = "<original_publication_year type=\"integer\">";
        String publicationYear = arp.parse(response, startRule, endRule);
        book.setPublicationYear(new Integer(publicationYear));

        startRule = "<average_rating>";
        String averageRating = arp.parse(response, startRule, endRule);
        book.setAverageRating(new Double(averageRating));

        startRule = "<ratings_count type=\"integer\">";
        String ratingsCount = arp.parse(response, startRule, endRule);
        book.setRatingsCount(new Integer(ratingsCount.replaceAll(",", "")));

        startRule = "<image_url>";
        String imageUrl = arp.parse(response, startRule, endRule);
        book.setImageUrl(imageUrl); 

        return book;
    }

    private String parse(String response, String startRule, String endRule) {
        int pos = response.indexOf(startRule);
        int start = pos + startRule.length();
        int end = response.indexOf(endRule, start);

        return response.substring(start, end);
    }

    // write overloaded parse method with the 3 parameters response, startRule, endRule 
    public static void main(String[] args) {
        String response = "<work>" + 
                                "<id type=\"integer\">2361393</id>" +
                                "<books_count type=\"integer\">813</books_count>" +
                                "<ratings_count type=\"integer\">1,16,315</ratings_count>" + 
                                "<text_reviews_count type=\"integer\">3439</text_reviews_count>" +
                                "<original_publication_year type=\"integer\">1854</original_publication_year>" +
                                "<original_publication_month type=\"integer\" nil=\"true\"/>" +
                                "<original_publication_day type=\"integer\" nil=\"true\"/>" +
                                "<average_rating>3.79</average_rating>" +
                                "<best_book type=\"Book\">" +
                                    "<id type=\"integer\">16902</id>" +
                                    "<title>Walden</title>" + 
                                    "<author>" +
                                        "<id type=\"integer\">10264</id>" + 
                                        "<name>Henry David Thoreau</name>" + 
                                    "</author>" +
                                    "<image_url>" + 
                                        "http://images.gr-assets.com/books/1465675526m/16902.jpg" +
                                    "</image_url>" +
                                    "<small_image_url>" + 
                                        "http://images.gr-assets.com/books/1465675526s/16902.jpg" +
                                    "</small_image_url>" +
                                "</best_book>" +
                            "</work>";

        Book book = APIResponseParser.parse(response);

        out.println(book.getAuthor());
        out.println(book.getTitle());
        out.println(book.getAverageRating());
        out.println(book.getImageUrl());
    }
}

The code has passed the automated tests.

But I would appreciate some real feedback.

What do you think about my string-processing and the way I've done the type conversion? What would you have done different and why?

All comments, hints and answers appreciated.

\$\endgroup\$
  • \$\begingroup\$ What version of Java are you using? \$\endgroup\$ – candied_orange May 27 '18 at 13:19
  • \$\begingroup\$ It would help if you more clearly specified which part of the code was provided by instructor and which one by you (maybe before and after code snippets?). \$\endgroup\$ – Łukasz Ciosek May 27 '18 at 13:35
  • \$\begingroup\$ @candied_orange It's version Java 8. \$\endgroup\$ – michael.zech May 28 '18 at 5:57
  • \$\begingroup\$ @ŁukaszCiosek Thanks for the hint. I have tried to improve my question accordingly. \$\endgroup\$ – michael.zech May 28 '18 at 6:05
2
\$\begingroup\$

"Main work was the instance-version of parse()" I am guessing that this means the non static parse() method. it contains 4 lines of code. out of 140 lines posted, that is very little "Main work"...

like it was said in the comments, it is not clear what code is yours and what was given. So I restricted my comments to the non static parse() method in APIResponseParser class:

  1. the APIResponseParser class seems (to me) to fit the "utility" pattern of class that has only static methods (like Java.lang.Objects or Apache's StringUtils). Therefore, I recommend that the non static parse() method be changed into static. That would eliminate the calling (static) method having to instantiate an instance of APIResponseParser.

  2. so there is already a parse method in APIResponseParser. For clarity sake, choose a different name for your method (unless it is dictated by the exercise). how about getXmlElementText()

  3. I am not sure what was the exercise but you need to understand that this is not the proper way to parse an XML document into a Java bean. In the 'real world' there are 3rd party parsers that can build a Book out of XML document.

  4. Now, if we stick to string processing, the method does not really parse an XML element. all it does is cut a substring of a given string. I mean it is fed the starting and ending tokens. If it is to be an XML element parser, it should be given just the element name and it should figure out how to find the text part of the element. in psuedo code:

String getXmlElementText(String elementName) {
search for "<" + elementName;
search for next ">"
start capturing text value;
search for next "</" + elementName -> stop capturing text value.
return text value
}

\$\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.