HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Intern with no mentor struggling with refactoring and OOP

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
refactoringwithinternmentorstrugglingandoop

Problem

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

Solution

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;
}

pu

Code Snippets

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);
        }
    }
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;
    }
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;
    }
}
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();
            }
        };
    }

}
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 "";
    }
}

Context

StackExchange Code Review Q#29191, answer score: 10

Revisions (0)

No revisions yet.