7
\$\begingroup\$

A little background
I'm an intern at a large engineering company, and I'm also the only CS major in the entire building. The people on my team don't have technical backgrounds, but hired me to start developing an internal application.

About my program
I'm trying to read two part numbers as input, search through a CSV file to find them, and then extract all of the associated parts into two separate lists that can then be compared.

Currently I can enter part numbers and then read the associated parts into an ArrayList. However

  • things feel kind of hacked together at this point
  • I don't exactly have a solid grasp on OOP
  • The two classes that "smell" to me are my FileInput.java and PartList.java classes (although for all I know the whole thing may stink, it's difficult without a developer to mentor me).

Ideally, I want to have two PartList objects that can then be compared in another class.

My main issue is how I'm instantiating the PartList within the FileInput. I was essentially fiddling around with Eclipse and its built in refactoring features to get it to work, and that's what I got.

I just don't think things are organized logically, and I'm unsure how to continue. I omitted the PartNumber class because it's twice as long as the others and I don't think it's necessary to understand my problem. Any advice is appreciated!

FileInput.java

public class FileInput {

    // Just a test file
    public final static String strFile = "C:\\Users\\hamlib1\\Projects\\HVC_Project\\TestData\\TT524S-FinalV1.csv";

    public void readCSV(String input) throws IOException {
        CSVReader reader = new CSVReader(new FileReader(strFile));
        String[] row;

        while ((row = reader.readNext()) != null) {

            int limit = row.length;

            for (int i = 0; i < limit; i++) {

                if (row[i].contains(input)) {
                    // The way I'm instantiating and calling from PartList here seems off to me
                    PartList partList = new PartList();
                    partList.setList(reader, row);
                }
            }
        }
        reader.close();
        PartList.printList();
        System.out.println("Reader Closed");
    }
}

PartList.java

public class PartList {

    private static ArrayList<String[]> partList = new ArrayList<String[]>();

    // Not sure what to even put here
    public PartList() {

    }

    // Algorithm for filling the list works, but it smells to me
    // Mainly passing the CSVreader and row, but when I start reading I want
    // to be at the row that matches the input
    public void setList(CSVReader reader, String[] row) throws IOException {
        String levelString = row[0];
        int levelInt = Integer.parseInt(levelString);
        if (levelInt == 2) {
            partList.add(row); // Add the first row to the list
            row = reader.readNext();
            String nextLevelString = row[0];
            int nextLevelInt = Integer.parseInt(nextLevelString);
            while (nextLevelInt > 2) { // Add subsequent rows
                partList.add(row);
                row = reader.readNext();
                nextLevelString = row[0];
                nextLevelInt = Integer.parseInt(nextLevelString);
            }
        }
    }

    // Just so I can check if I'm filling the list correctly
    public static void printList() {
        for (int i = 0; i < partList.size(); i++) {
            String[] strings = partList.get(i);
            for (int j = 0; j < strings.length; j++) {
                System.out.print(strings[j] + " | ");
            }
            System.out.println();
        }
    }

}

RunProgram.java

public class RunProgram {

    public static void main(String[] args) {

        PartNumber part1 = new PartNumber();
        PartNumber part2 = new PartNumber();

        FileInput file = new FileInput();

        part1.readInput();
        part2.readInput();

        String partString1 = part1.getPartNumber();
        String partString2 = part2.getPartNumber();

        try {
            System.out.println(part1.toString());
            file.readCSV(partString1);
            System.out.println(part2.toString());
            file.readCSV(partString2);
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

    }
}

Example of CSV file

Here's an example of the CSV. The one I pulled this from is about 35,000 lines, but they all follow the same format. I deleted some irrelevant columns for simplicity, but the important information are the levels and part numbers. They won't change position, and searching through an entire CSV only takes a few seconds. To pull a group of parts I first check if its level is 2 (denoting a "top" level) then I pull all subsequent parts until the next 2.

Edit

I decided to create create a PartList object and pass it to my readCSV method. This appeared to work when I printed the list. Then I created a second PartList, and tried to print both lists. What should have happened is each list printed separately. Instead, it printed both lists merged together, twice. Somehow I'm adding the parts for two separate lists into a single list. I think this comes down to my PartList class (and lack of appropriate constructors, getters & setters?). I think this problem is now more suited for Stack Overflow, but if anyone has general refactoring help I'll still greatly appreciate it.

\$\endgroup\$
  • \$\begingroup\$ How big is the CSV file (# of lines)? How often does the CSV change? Can the part number show up in any "column" in the file? It also looks like there is some sort of hierarchy to the csv file based on how you are searching. Perhaps post a few lines as an example. You can change names, codes, etc. if needed as long as the structure / columns are intact. \$\endgroup\$ – nickles80 Jul 30 '13 at 21:51
  • \$\begingroup\$ I can't seem to find an email listed on your profile. Mine is brianhamlin14@gmail.com if you have any advice. Thanks. \$\endgroup\$ – bhamlin Jul 31 '13 at 12:48
  • \$\begingroup\$ If it were me, I would load the files into hsqldb.org and then do a SQL statement. This way, if they ask for a new report I could re-use the code and deliver quickly. \$\endgroup\$ – konijn Jul 31 '13 at 13:24
  • \$\begingroup\$ @RobertSnyder: It would be great if that advice were to be shared for everyone. :-) \$\endgroup\$ – Dave Jarvis Jul 31 '13 at 20:50
  • \$\begingroup\$ @DaveJarvis yeah I have a soft spot for that type of situation. :) \$\endgroup\$ – Robert Snyder Jul 31 '13 at 20:58
10
\$\begingroup\$

Some beginner mistakes...

The functionality of the classes should make sense logically. Instead of bringing out the terminologies, I prefer to think from a layman's interpretation of a class. Why should your PartList know what is a CSVReader? And why should your PartNumber know how to read an input?

Other pointers

You also need to learn how to accurate translate the rules and requirements of your work into programming logic. For example, the identification of non-"top level" parts. Are they identified by a model level number greater than two, as written in your code? Or just when they are not equal to two, as you have written in your question? This is also where validation of values and "business requirements" come in to play, to ensure your code functions as per expectations.

Gotcha

Looking at your code again, there is a glaring bug between the for loop below and the setList method of PartList, which is called in the same loop too:

for (int i = 0; i < limit; i++) {
        if (row[i].contains(input)) {
            // The way I'm instantiating and calling from PartList here seems off to me
            PartList partList = new PartList();
            partList.setList(reader, row);
        }
    }

setList is actually modifying the state of your reader object, i.e. when this method returns and there is the slim chance that the next column matches the input (I know it's not intentional, so probably bug #2?), you wouldn't be looping through the same lines of the reader object. Instead, you will be starting from the next line which the first invocation of setList left off. In fact, your code will also be reading in one extra line when setList exits upon encountering the next top-level part, and the flow goes into the next iteration of the outer while loop which calls reader.readNext() again. Last but not least, PartList is instantiated within the for loop, meaning it can't be used outside at all.

Suggested solution

package com.myproject.se;

public class Part {

    enum Serviceability {
        SERVICEABLE("SVCBLE"), NOT_SERVICEABLE("NOT SVC"), UNDEFINED("");

        private String value;

        Serviceability(String value) {
            this.value = value;
        }

        public String toString() {
            return value;
        }

        static Serviceability fromValue(String value) {
            if (value != null) {
                String valueToCompare = value.toUpperCase();
                for (Serviceability current : Serviceability.values()) {
                    if (current.value.equals(valueToCompare)) {
                        return current;
                    }
                }
            }
            return UNDEFINED;
        }
    }

    enum PartType {
        PART("PART"), ASSEMBLY("ASSEMBLY"), UNDEFINED("");

        private String value;

        PartType(String value) {
            this.value = value;
        }

        public String toString() {
            return value;
        }

        static PartType fromValue(String value) {
            if (value != null) {
                String valueToCompare = value.toUpperCase();
                for (PartType current : PartType.values()) {
                    if (current.value.equals(valueToCompare)) {
                        return current;
                    }
                }
            }
            return UNDEFINED;
        }
    }

    private int modelLevel;
    private String partNumber;
    private String name;
    private Serviceability serviceability;
    private int quantity;
    private PartType partType;

    public Part(String[] values) {
        // TODO: sanity checking for desired number of columns
        setModelLevel(Integer.parseInt(values[0]));
        setPartNumber(values[1]);
        setName(values[2]);
        setServiceability(values[3]);
        setQuantity(Integer.parseInt(values[4]));
        setPartType(values[5]);
    }

    public int getModelLevel() {
        return modelLevel;
    }

    public void setModelLevel(int modelLevel) {
        this.modelLevel = modelLevel;
    }

    public String getPartNumber() {
        return partNumber;
    }

    public void setPartNumber(String partNumber) {
        // normalize to upper case
        this.partNumber = partNumber.toUpperCase();
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        // normalize to upper case
        this.name = name.toUpperCase();
    }

    public Serviceability getServiceability() {
        return serviceability;
    }

    public void setServiceability(Serviceability serviceability) {
        this.serviceability = serviceability;
    }

    public void setServiceability(String serviceability) {
        this.serviceability = Serviceability.fromValue(serviceability);
    }

    public int getQuantity() {
        return quantity;
    }

    public void setQuantity(int quantity) {
        this.quantity = quantity;
    }

    public PartType getPartType() {
        return partType;
    }

    public void setPartType(PartType partType) {
        this.partType = partType;
    }

    public void setPartType(String partType) {
        this.partType = PartType.fromValue(partType);
    }

    public boolean isTopLevelPart() {
        return modelLevel == 2;
    }

    public boolean hasPartNumberContains(String value) {
        return partNumber.contains(value);
    }

    public boolean isServiceable() {
        return serviceability == Serviceability.SERVICEABLE;
    }

    public String toString() {
        return modelLevel + " | " + partNumber + " | " + name + " | " + serviceability + " | " + quantity + " | "
                + partType;
    }
}

I used two Enums to represent valid values for serviceability and part type, in case I need to compare these values often and I want to avoid any typos in doing String comparisons. I also normalize String inputs to upper case for this exercise. By right, I can also have a PartFactory class to take in the six String inputs and construct a Part for me, but I have decided to keep it simple and just implemented a constructor for this purpose. Feel free to add useful methods that is related to a Part and will aid in your work here, for example I have the isTopLevelPart method that will return true if the model number is 2.

package com.myproject.se;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

public class PartList implements Iterable<Part> {
    private List<Part> partList = new ArrayList<Part>();

    public PartList(Part part) {
        addPart(part);
    }

    public void addPart(Part part) {
        // simple constraint
        if (partList.isEmpty() && !part.isTopLevelPart()) {
            throw new RuntimeException("This is not a top-level part.");
        }
        partList.add(part);
    }

    @Override
    public Iterator<Part> iterator() {
        return partList.iterator();
    }

    public Part getTopLevelPart() {
        return partList.get(0);
    }

    public int size() {
        return partList.size();
    }

    public List<Part> getServiceableParts() {
        List<Part> result = new ArrayList<Part>();
        for (Part part : partList) {
            if (part.isServiceable()) {
                result.add(part);
            }
        }
        return result;
    }
}

PartList is a simple wrapper class that stores Part objects in an ArrayList. The only enhancement I have added is a constraint on the first element of the List, which must be a top-level part (see above). Other than that, feel free to once again introduce methods related to what a PartList should know, in my case here I added a simple getServiceableParts method for illustration.

package com.myproject.se;

import java.io.Closeable;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

public class RecordSupplier implements Iterable<Part>, Closeable {
    private static final List<String[]> testValues = new ArrayList<String[]>();

    static {
        testValues.add(new String[] { "2", "ABC", "NAME 1", "", "1", "" });
        testValues.add(new String[] { "3", "DEF", "NAME 2", "SVCBLE", "2", "PART" });
        testValues.add(new String[] { "4", "GHI", "NAME 3", "NOT SVC", "3", "ASSEMBLY" });
        testValues.add(new String[] { "2", "JKL", "NAME 4", "", "1", "" });
        testValues.add(new String[] { "3", "MNO", "NAME 5", "SVCBLE", "5", "PART" });
    }

    @Override
    public void close() throws IOException {
        // do nothing
    }

    @Override
    public Iterator<Part> iterator() {
        return new Iterator<Part>() {
            private Iterator<String[]> innerIterator = testValues.iterator();

            @Override
            public boolean hasNext() {
                return innerIterator.hasNext();
            }
            @Override
            public Part next() {
                return new Part(innerIterator.next());
            }
            @Override
            public void remove() {
                throw new UnsupportedOperationException();
            }
        };
    }

}

I needed a mock class here to simulate a CSVReader, so here's a RecordSupplier using the iteration pattern of providing input. What is crucial to note here is that RecordSupplier, or more specifically its iterator, supplies Part objects, instead of a String array. Generally, you will want a well-behaved input provider that passes only types that can appropriately encapsulate the raw underlying values to caller methods that need the data. The other minor benefit of wrapping your CSVReader in such a class is that you can easily replace the actual input in the future by a database, or XML file reader etc, and all the caller methods see are still Part objects.

package com.myproject.se;

import java.io.IOException;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Scanner;

public class Main {

    public static void main(String[] args) {
        @SuppressWarnings("unused")
        Scanner scanner = new Scanner(System.in);
        String partNumber1;
        String partNumber2;
        System.out.println("Enter first part number:");
        // partNumber1 = scanner.next();
        partNumber1 = "JKL";
        System.out.println("Enter second part number:");
        // partNumber2 = scanner.next();
        partNumber2 = "ABC";
        Map<String, PartList> result = getPartLists(partNumber1, partNumber2);
        for (Entry<String, PartList> entry : result.entrySet()) {
            System.out.println("Part number search term: " + entry.getKey());
            Iterator<Part> iterator = entry.getValue().iterator();
            while (iterator.hasNext()) {
                System.out.println(iterator.next());
            }
            System.out.println("====================");
        }
    }

    public static Map<String, PartList> getPartLists(String... partNumbers) {
        // only searching for two parts now, easily extensible
        if (partNumbers.length != 2) {
            throw new RuntimeException("Only two part numbers needed.");
        }
        Map<String, PartList> result = new HashMap<String, PartList>();
        RecordSupplier supplier = new RecordSupplier();
        Iterator<Part> iterator = supplier.iterator();
        PartList partList = null;
        while (iterator.hasNext()) {
            Part part = iterator.next();
            if (part.isTopLevelPart()) {
                String match = partNumberMatches(part, partNumbers);
                if (match.isEmpty()) {
                    partList = null;
                } else {
                    // match belongs to a new PartList
                    partList = new PartList(part);
                    // add the new PartList to result
                    result.put(match, partList);
                }
            } else if (partList != null) {
                // keep adding current Part to existing PartList
                partList.addPart(part);
            }
        }
        try {
            supplier.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
        // returning just a List of PartList should be good enough actually
        return result;
    }

    public static String partNumberMatches(Part part, String... partNumbers) {
        for (String partNumber : partNumbers) {
            if (part.hasPartNumberContains(partNumber)) {
                return partNumber;
            }
        }
        return "";
    }
}

Finally, the main class. I have commented out the parts that read in user input (see the use of the Scanner class) and hard-coded two test values. The bulk of the work is done in getPartLists.

Map<String, PartList> result = new HashMap<String, PartList>();
RecordSupplier supplier = new RecordSupplier();
Iterator<Part> iterator = supplier.iterator();
PartList partList = null;

Create our result object and the RecordSupplier.

while (iterator.hasNext()) {
    Part part = iterator.next();

Get a Part object representing the current row.

    if (part.isTopLevelPart()) {
        String match = partNumberMatches(part, partNumbers);

If this is a top-level part, check if it matches our inputs or not (I think partNumberMatches is pretty self-explanatory and will not go there).

        if (match.isEmpty()) {
            partList = null;

If there is no match, we set our partList to null, using this as some sort of a flag afterwards to know we are ignoring subsequent lines until the next top-level part.

        } else {
            // match belongs to a new PartList
            partList = new PartList(part);
            // add the new PartList to result
            result.put(match, partList);
        }

We have a match, lets set partList as a new PartList with this top-level part and add it to our result.

    } else if (partList != null) {
        // keep adding current Part to existing PartList
        partList.addPart(part);
    }
}

For other non-top-level parts, we will just check if partList is null or not. If it isn't, it means we have just matched a top-level part to our inputs and we need to add the current part to partList.

Implicit assumptions:

  1. We are only matching part number names with the second column, Part Number Breakdown (see partNumberMatches and Part.hasPartNumberContains)
  2. One top-level part will only match one input.

I hope this is informative enough...

\$\endgroup\$
  • \$\begingroup\$ I am impressed. +1 \$\endgroup\$ – konijn Aug 1 '13 at 21:17
  • 1
    \$\begingroup\$ Probably one of the most useful suggestions I have seen in a long time. Very well written and orgainized. Very thorough. Good job. +1 \$\endgroup\$ – Robert Snyder Aug 2 '13 at 3:21

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.