Generic data converter for one type of data to another

Question

I wrote a generic data converter that takes one kind of data and converts to JSON and XML types of file. Now, it reads data from the given CSV file hotels.csv. The first line is a header which describes all field names and, follows the rules given below:

a. A hotel name may only contain UTF-8 characters.
b. The hotel URL must be valid 
c. Hotel ratings are given as a number from 0 to 5 stars. There may be no negative numbers
d. the tool needs to be extensible to new data output formats
e. Write proper Unit tests where necessary
f. Options to sort/group the data before writing it

AS the project is little larger to put here entirely, I keep it in the Github and please have a look from there. I provided the structure of the project below -

Here are some classes with importance:

App.java

public class App {

    public static void main(String[] args) throws Exception {

        DataConverter dataConverter = new DataConverter(Constant.HOTEL_DATA);

        System.out.println(new GetMessage().welcomeTrivagoDevTeam());
        System.out.println();

        Scanner scanner = new Scanner(System.in);

        while (scanner.hasNext()) {

            String input = scanner.nextLine();
            System.out.println("ENTER Q OR QUIT TO QUIT \n");

            if (input.toLowerCase().equals("q") || input.toLowerCase().equals("quit")) {
                System.out.println("SEE YOU");
                System.exit(0);
            }

            try {

                int caseValue = Constant.DESIRED_OUTPUT_FILE = Integer.parseInt(input);
                switch (caseValue) {

                    case 1:
                        System.out.println("INPUT : " + input + " " + " :XML ");
                        System.out.println();
                        break;

                    case 2:
                        System.out.println("INPUT : " + input + " " + " :JSON ");
                        System.out.println();
                        break;

                    default:
                        System.out.println("OUTPUT TYPE NOT SUPPORTED ");
                }
            } catch (Exception e) {

                e.printStackTrace();
                System.exit(0);
            }

            System.out.println("IF WOULD YOU LIKE TO SORT/GROUP THE RESULT, \nENTER YES OR Y");
            System.out.println("OTHERWISE HIT ENTER TO CONTINUE");

            input = scanner.nextLine();

            if (input.isEmpty()) {
                System.out.println("LETS GET THE OUTPUT FILE \n");
                break;
            } else if (input.toLowerCase().equals("yes") || input.toLowerCase().equals("y")) {

                System.out.println("HOW WOULD YOU LIKE TO SORT THE RESULT ?");
                System.out.println("ENTER 1 FOR BASED ON NAME ");
                System.out.println("ENTER 2 FOR BASED ON THE HOTEL RATINGS");

                input = scanner.nextLine();

                if (input.toLowerCase().equals("1")) {
                    Constant.DESIRED_SORTING = 1;
                    break;
                } else if (input.toLowerCase().equals("2")) {
                    Constant.DESIRED_SORTING = 2;
                    break;
                } else
                    return;
            } else
                System.out.println("FOLLOW THE INSTRUCTIONS");
            return;
        }

        dataConverter.changeDataFormat(Constant.DESIRED_OUTPUT_FILE, Constant.DESIRED_SORTING);
    }
}

FileReader.java

public abstract class FileReader implements FileReadable, 
                                            XmlFileConvertable, 
                                            JsonFileConvertable {

    protected List<HotelData> rows;
    protected HotelData hotelData;
    protected Headers headers;
    private String fileName = null;

    public FileReader(String fileName) {

        this.fileName = fileName;
        this.rows = new ArrayList<>();
        this.hotelData = null;
        this.headers = null;

        fileReader();
    }

    public void fileReader() {

        try {
            readCsvFile();
        } catch (FileNotFoundException ex) {
            ex.printStackTrace();
        }
    }

    @Override
    public void readCsvFile() throws FileNotFoundException {

        File file = new FileFinder().getTheFile(fileName);

        try (Scanner scanner = new Scanner(file)) {

            List<String> line = parseLine(scanner.nextLine());

            if (line != null) {

                // name,address,stars,contact,phone,uri
                headers = new Headers(line.get(0),
                        line.get(1),
                        line.get(2),
                        line.get(3),
                        line.get(4),
                        line.get(5));
            }

            while (scanner.hasNext()) {

                line = parseLine(scanner.nextLine());

                if (line != null) {

                    // name,address,stars,contact,phone,uri
                    String name = line.get(0);
                    String address = line.get(1);
                    String stars = line.get(2);
                    String contact = line.get(3);
                    String phone = line.get(4);
                    String uri = line.get(5);

                    boolean nameValidated = isNameIsUTF8(name);
                    boolean urlIsValidated = isUrlValidated(uri);
                    boolean hotelRatingValidated = isValidHotelRating(stars);

                    // name, uri and the hotel rating validated
                    if (nameValidated && urlIsValidated && hotelRatingValidated) {
                        hotelData = new HotelData(name, address, stars, contact, phone, uri);
                        rows.add(hotelData);
                    }
                }
            }
            scanner.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

DataConverter.java

public class DataConverter extends FileReader {

//    private StringBuilder stringBuilder;

    public DataConverter(String csvFile) {
        super(csvFile);
    }

    public void changeDataFormat(int value, int sort) {

        // sorting is required by the client
        if (sort != -1)
            sortDataList(sort, rows);

        // get the output in the desired format
        switch (value) {

            case 1:
                dataToXmlConverter();
                break;

            case 2:
                dataToJsonConverter();
                break;

            default:
                System.out.println("FILE OUTPUT TYPE IS NOT SUPPORTED");
                break;
        }
    }

    @Override
    public void dataToXmlConverter() {

        if (rows == null || rows.isEmpty())
            return;

        DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
        DocumentBuilder docBuilder = null;

        try {
            docBuilder = docFactory.newDocumentBuilder();
        } catch (ParserConfigurationException e) {
            e.printStackTrace();
        }

        Document doc = docBuilder.newDocument();
        Element rootElement = doc.createElement("info");
        doc.appendChild(rootElement);

        for (int i = 0; i < rows.size(); i++) {

            HotelData hotelData = rows.get(i);

            // name,address,stars,contact,phone,uri
            Element content = doc.createElement("row");
            rootElement.appendChild(content);

            Attr attr = doc.createAttribute("id");
            attr.setValue(String.valueOf(i + 1));
            content.setAttributeNode(attr);

            Element name = doc.createElement(headers.getName());
            name.appendChild(doc.createTextNode(hotelData.getName()));
            content.appendChild(name);

            Element address = doc.createElement(headers.getAddress());
            address.appendChild(doc.createTextNode(hotelData.getAddress()));
            content.appendChild(address);

            Element stars = doc.createElement(headers.getStars());
            stars.appendChild(doc.createTextNode(hotelData.getStars()));
            content.appendChild(stars);

            Element contact = doc.createElement(headers.getContact());
            contact.appendChild(doc.createTextNode(hotelData.getContact()));
            content.appendChild(contact);

            Element phone = doc.createElement(headers.getPhone());
            phone.appendChild(doc.createTextNode(hotelData.getPhone()));
            content.appendChild(phone);

            Element uri = doc.createElement(headers.getUri());
            uri.appendChild(doc.createTextNode(hotelData.getUri()));
            content.appendChild(uri);
        }

        try {

            TransformerFactory transformerFactory = TransformerFactory.newInstance();
            Transformer transformer = transformerFactory.newTransformer();
            DOMSource source = new DOMSource(doc);

            StreamResult result = new StreamResult(new File(Constant.OUTPUT_LOC + "/result.xml"));
            transformer.transform(source, result);
        } catch (Exception ex) {
            ex.printStackTrace();
        }

        System.out.println("CONVERTED TO XML");
    }

    @Override
    public void dataToJsonConverter() {

        //  long startTime = System.currentTimeMillis();
        //  JSON CONVERSION TIME : 144 (FOR NAME BASED GROUPING)

        File file = new File(Constant.OUTPUT_LOC + "/result.json");

        String jsonValue = dataToJsonConverterHelper(rows);

        try (FileOutputStream fop = new FileOutputStream(file)) {

            // if file doesn't exists, then create it
            if (!file.exists()) {
                file.createNewFile();
            }

            // get the content in bytes
            byte[] contentInBytes = jsonValue.getBytes();

            fop.write(contentInBytes);
            fop.flush();
            fop.close();

            System.out.println("CONVERTED TO JSON");
        } 

        catch (IOException e) {
            e.printStackTrace();
        }

        // long estimatedTime = System.currentTimeMillis() - startTime;
        // System.out.println("JSON CONVERSION TIME : " + estimatedTime);
    }

    public String dataToJsonConverterHelper(List<HotelData> rows) {

        String jsonValue = "";

        if (rows == null || rows.isEmpty())
            return jsonValue;

        jsonValue = hotelsToJSON(rows);
        return jsonValue;
    }


//    @Override
//    public void dataToJsonConverter() {
//
//        ObjectMapper mapper = new ObjectMapper();


//        long startTime = System.currentTimeMillis();
//        JSON CONVERSION TIME : 8114 (FOR NAME BASED GROUPING)

//        try {
//            String jsonInString = "";
//
//            for (HotelData hotels : rows) {
//                jsonInString += mapper.writeValueAsString(hotels);
//            }
//            mapper.writeValue(new File(Constant.OUTPUT_LOC + "/result.json"), jsonInString);
//        }



//        long startTime = System.currentTimeMillis();
    //        JSON CONVERSION TIME : 332 (FOR NAME BASED GROUPING)

    // better performance using the StringBuilder
//        try {
//            stringBuilder = new StringBuilder();
//
//            for (HotelData hotels : rows) {
//                stringBuilder.append(mapper.writeValueAsString(hotels).trim());
//            }
//            mapper.writeValue(new File(Constant.OUTPUT_LOC + "/result.json"), stringBuilder.toString());
//        }



//        catch (JsonGenerationException e) {
//            e.printStackTrace();
//        } catch (JsonMappingException e) {
//            e.printStackTrace();
//        } catch (IOException e) {
//            e.printStackTrace();
//        }

//        long estimatedTime = System.currentTimeMillis() - startTime;
//        System.out.println("JSON CONVERSION TIME : " + estimatedTime);

//        System.out.println("CONVERTED TO JSON");
//    }

}

How can I improve the architecture, elegance and performance (lesser priority in this context) of code ?

\$\begingroup\$ No suggestion at all ? \$\endgroup\$

Chak
– Chak

2016-12-02 12:21:00 +00:00
Commented Dec 2, 2016 at 12:21 — Chak
– Chak, Commented Dec 2, 2016 at 12:21

h.j.k. · Accepted Answer · 2016-12-02 16:57:28Z

SHOUTY PROMPTS

NOT SURE WHY YOU PREFER TO PROMPT THE USER BY 'SHOUTING' AT THEM...

You're not programming an 80s-era (70s?) console terminal program, so from a UX perspective it's better to display the prompts using the appropriate casing.

Vertical whitespace

You have used a non-insignificant amount of vertical whitespace, i.e. empty lines, which tends to make scrolling through code a little harder to comprehend.

Comments

You have a huge chunk of commented code. This may be fine for a small, personal, non-versioned project, but once you factor in a code repository, it's usually suggested not to leave commented code inside the codebase. This is because old/unused code can be easily reverted through the history, or using the appropriate branching features.

Constants/Flags

if (input.toLowerCase().equals("1")) {
    Constant.DESIRED_SORTING = 1;
    break;
} else if (input.toLowerCase().equals("2")) {
    Constant.DESIRED_SORTING = 2;
    break;
} else
    return;

You didn't show what your Constant class is, but you shouldn't be assigning values to it for two reasons:

In any other codebase, a Constant class sounds like what it should be doing: providing some constant values. These values should not be mutable.

You should not be using int values as flags to tell your program what to do. Since Java 5, there's support for Enum types which lets you do something like:

public enum Sorting {
    BY_NAME,
    BY_RATING;

    // pro-tip: Have each value carry a Comparator<HotelData>
}

// usage
private List<HotelData> sort(List<HotelData> input, Sorting sortingBy) {
    final Comparator<HotelData> comparator;
    switch (sortingBy) {
        case BY_NAME:
            comparator = getComparatorByName();
            break;
        case BY_RATING:
            comparator = getComparatorByRating();
            break;
        default:
            throw new RuntimeException("No sorting flag provided");
    }
    List<HotelData> result = new ArrayList<>(input);
    Collections.sort(result, comparator);
    return result;
}

Exception handling

Your current approach of exception handling looks like it's just the default option that any good ol' IDE provides. You should be more careful though, for example when the program fails to read a file.

The current implementation simply prints the exception stack trace at the constructor of FileReader, leaving all its variables as the default null values, which makes it easy for NullPointerException errors to be thrown to users of the object.

A FileReader object that fails to read a file should instead propagate the error to the user.

Thanks a lot for your review. Any suggestion of the project architecture ? — Chak
– Chak, Commented Dec 3, 2016 at 2:22
I'll probably update my answer later, but for starters why not have two classes to do the conversion based on the input, instead of one class that knows how to switch between? — h.j.k.
– h.j.k., Commented Dec 3, 2016 at 9:46
Okay, I apparently thought this is the better way of doing. I'm programmer seriously for more than 2 years now. Is that I'm fallen behind than the other programmers you interact ? Just curious! I recently discovered the Code Review platform and trying to make the best of it. — Chak
– Chak, Commented Dec 3, 2016 at 9:50
Welcome to Code Review, we all learn from one another here. :) — h.j.k.
– h.j.k., Commented Dec 3, 2016 at 16:56

Community · Accepted Answer · 2017-05-23 12:40:53Z

@h.j.k. has provided a great answer on programming style.

Disclaimer: I use strong words. Don't be discouraged, we're here to get better! I can be yelled at if I learn something in the process.

On the matter of style, I can just add that you're doing a lot of work on your constructors, which is considered bad practice. Additionally, you're doing work on the constructor that is performed in an overridable method. This is quite bad, as the method may be overrided in a way that breaks the object, or repurpose it, etc.

Now I've said this, I will focus on code structure.

Code Structure

Your FileReader class looks like this:

public abstract class FileReader implements FileReadable, XmlFileConvertable, JsonFileConvertable {
    (...field declaration...)

    public FileReader(String fileName) {
        (...field init...)
        fileReader();
    }

    public void fileReader() {
        try {
            readCsvFile();
        } catch (FileNotFoundException ex) {
            ex.printStackTrace();
        }
    }

    @Override
    public void readCsvFile() throws FileNotFoundException {
        (...some work...)
    }
}

Let's talk class name:

FileReader is a taken name. It is taken by the JDK, you usually don't want to clash with these, or people will find it annoying to interact with your custom class and a widely used JDK API class, in the same scope.
FileReader is a misleading name. It does not tell me that the class only reads CSV files. Why not name it CsvFileReader ?
XXXXFileReader is a bad name. It does not tell me that it only reads Hotel CSV files. Why not name it HotelCsvReader ?

Let's talk Interfaces:

FileReadable is a bad name. It is backwards, and its only method is readCsvFile, so the interface declares a Reader, not a Readable. Why not name it CsvReader ?
JSONNable is a bad name. Its only method is toJSON so it is more of a JsonConverter
JsonFileConvertable and XmlFileConvertableare probably bad names... But I won't figure out what they are supposed to be, because frankly I'm starting to distrust your naming.

Run away from side-effect methods

All your interfaces force their implementation to perform actions with side-effects. Their single methods all look like : void doSomethingInternally(/*No Args!*/) throws SomeException. I see 4 mistakes in this kind of declaration for an object of those types:

Since these methods take no arguments, you're supposed to load the object with the data beforehand. So you're already asking the object to do some Read / Store operation. This is probably already boyond the scope of e.g. a Converter object. Since not method to load data is provided by the interface, there will be no standard way of doing this. This means the user has to know each specific implementation's way of doing things!
These methods expect the object to perform an action. This is normal. However because of all the surronding points, the object's internal representation will have to be mutated. This is not a problem if the Object is supposed to have a lifecycle. But a Converter, a Reader, Writer, etc. aren't supposed to.
The method does not return anything. Not even a success flag, even less the resulting object. That means the way to retrieve action status and data are, again, non-standard and will be a pain to use, similar to 1).
The methods throw nothing. Many things can happen during a conversion, at least provide a checked exception. In your case of mutable Converter, you'd need a IllegalStateException. This is badly needed because of your poorly handled exception in constructor, which is only printed but lets the program run using crippled object. If you had a stateless converter that expected some arguments like I propose, then it may throw an InvalidArgumentsException. In any case, a conversion may still fail for business reasons, so I would add FailedConversionException. A reader can have a FileNotFoundException as you did, but also a IncorrectFormatException, etc.

Recommendation

Generally, I would declare the following interfaces:

CsvHotelReader :

public interface CsvHotelReader {
    List<HotelData> readHotelFromCsv(String fileLocation) throws FileNotFoundException;
}

JsonHotelReader :

public interface JsonHotelReader {
    List<HotelData> readHotelFromJson(String fileLocation) throws FileNotFoundException;
}

CsvHotelConverter :

public interface CsvHotelConverter {
    String convertToCsv(HotelData... hotel);
    String convertToCsv(List<HotelData> hotels);
}

JsonHotelConverter:

JsonHotelConverter with a two methods:
    String convertToJson(HotelData... hotel);
    String convertToJson(List<HotelData> hotels);
}

The FileWriter can then be the standard JDK one, and will write whatever String you throw at it.

To come up with this is simple: a class only does what it advertises. A Reader reads, but does not store. A Converter converts, but does not read, nor store. A Writer does not convert. One class is used to store data (HotelData, you don't need Headers btw), this is used as rich pivot format for data handling.

Then your classes are all reusable. The power comes from combining these classes in higher-order methods, not inheriting to make God-objects that implement everything.

Utils are usually badly done

A package called 'utils' makes me frown.

If it helps something, it must be in something's package. Java has String helper methods directly in String class, Collections class is in Collection package.
I'll pass on Constant class because I read h.j.k. 's answer, and I don't want to have my eyes actually read this. EDIT. Too late.
Your Formatter is actually a Parser... and only parses CSV. Name it CsvParser, put it in csv package (possibly even in CsvFileReader class) because it won't be used to parse JSON!
FileFinder is a FileLoader
GetMessage is a verb. Should be a noun for class name. Additionally I have no idea what message. For once I won't suggest another naming. I believe it is quite useless, and should not be an Object at all. Scrap the object and make it a method of the app class.
Sorter is well named! However, I wouldn't make it a standalone object, either. I'm not trying to be an ass here, but generally, I detected you had trouble handling bundles of hotels. I suggest having a HotelRepository class (essentially a List inside) which can be browsed (with several orderings), and passed to a converter entirely. So it goes out of utils, and into models package. Then you can have it handled by the interfaces directly. Oh, and it solves the bizarre Headers.java class, because a HotelRepository class could contain global metadata.
Validator could be good. The rating validation is business and so should be in HotelData. I'd be wary of trying to regex an URL, as there are many ways of doing it wrong. In any case you don't need isUrlValidated and isUrlValidated2: either they do different things (and have different names) or they do the same thing, but one is wrong and has to go. isNameIsUTF8 is a bad name: bad english (isNameUTF8 is better) and it mixes with your business jargon (id does not only check names, but any string) so should be isUTF8.

The Models

It is good to have a package for grouping business classes! Well thought.

HotelData

It is a good class, like a Bean. It may benefit from a Builder class.

The method hotelsToJSON is a helper for Conversion. It should be in the JsonConverter class.

Making a Hotel JSONable is a design decision. Be careful, if the hotel implements too many XXXable, it might become to complex. Since you have a JSONConverter (which is entirely reasonable since HotelData exposes getters for everything - which is good), then maybe you don't need this at all. Suppose you suddently need to serialize this class to store it in DB, to disc or pass it over JMS. If its JSON description changes (because the JSON API / format changed) then you need to change the class, and suddently deserializing is becoming a pain. But it shouldn't because a hotel is still the same business thing! So I would remove these methods.

The root classes

DataConverter is rightly named. It converts everything. The problem is it does too much... What good is it splitting functions in interfaces if only one object implements all? Make a JsonConverter, etc.

Haven't looked at App much, but try to take out everything from main, and extract as many methods as possible.

If you only retain one thing from this wall of text:

There are only two hard things in Computer Science: cache invalidation and naming things.

-- Phil Karlton

You have a big problem with naming. This is the one thing to work on! Once naming is correct, everything becomes easy...

And please, follow all of h.j.k.'s advices!

Stack Exchange Network

Generic data converter for one type of data to another

2 Answers 2

SHOUTY PROMPTS

Vertical whitespace

Comments

Constants/Flags

Exception handling

Code Structure

Let's talk class name:

Let's talk Interfaces:

Run away from side-effect methods

Recommendation

Utils are usually badly done

The Models

HotelData

The root classes

You must log in to answer this question.

Hot Network Questions

Generic data converter for one type of data to another

2 Answers 2

SHOUTY PROMPTS

Vertical whitespace

Comments

Constants/Flags

Exception handling

Code Structure

Let's talk class name:

Let's talk Interfaces:

Run away from side-effect methods

Recommendation

Utils are usually badly done

The Models

HotelData

The root classes

You must log in to answer this question.

Related

Hot Network Questions