6
\$\begingroup\$

The following method is used to add objects to a polymorphic ArrayList from a file.

It reads lines from the file, and based on a specific line which denotes the category of the product object, it uses the loadFromFile() method of the specific class, before moving onto the next. We are taught to build programs in such a way to allow for future functionality, so I would like to find a way to modify this so if a new child class was created, it would be able to detect that and call the method of that class. instead of the developer needing to add to the switch case every time.

Each child class has slightly different attributes hence why this is needed. I am looking for a more sophisticated and future proof method.

public void loadFromFile(String fileName) throws FileNotFoundException, IOException, ParseException{
    Filename = fileName;
    String record;

    FileReader reader;

    reader = new FileReader(Filename);
    BufferedReader bin = new BufferedReader(reader);
    record = new String();

    while ((record = bin.readLine())!=null)
    {     
      switch(bin.readLine()){

        case "Electronic":
          ElectronicProduct eProduct = new ElectronicProduct();          
          Products.add(eProduct.loadFromFile(eProduct,bin)); 
          break;

        case "Kitchen":
          KitchenProduct kProduct = new KitchenProduct();          
          Products.add(kProduct.loadFromFile(kProduct,bin)); 
          break;

        case "Food":
          FoodProduct fProduct = new FoodProduct();          
          Products.add(fProduct.loadFromFile(fProduct,bin)); 
          break;

        case "Book":
          BookProduct bProduct = new BookProduct();          
          Products.add(bProduct.loadFromFile(bProduct,bin)); 
          break;

      }                  
    bin.close();
    bin =null;    
    }
}  

EDIT

Below shows entire class, some methods not fully functional as currently upgrading from iteration 1 to iteration 2.

package mysupermarket;
import java.awt.Image;
import java.awt.image.BufferedImage;
import java.io.BufferedReader;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.Date;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.imageio.ImageIO;
import javax.swing.ImageIcon;
import javax.swing.JLabel;
/**
 *
 * @author x
 */
public class ProductList {

    private ArrayList<Product>Products;
    private String Filename;    

    public ProductList(){


    Filename = "ProductList.txt";
    }


    public void add(Product src) throws IOException{

        Products.add (src);          

    }

    public void remove(Product src){
        Products.remove (src);
    }

    public Optional<Product> find(String nameInput){


        Optional<Product> aProduct= this.Products.stream()
                 .filter(p->p.getName().equalsIgnoreCase(nameInput))
                 .findFirst();


        return aProduct;

    }

    public void display (javax.swing.JTextArea src){

        this.Products.forEach((product) -> {
            product.display(true, src);
        });  
    }                                  

    public void displayReorders(javax.swing.JTextArea src){

        src.setText(null);   

        for(Product product: this.Products){
           if(scanForLowStock(product)){
                    src.append("Product: " + product.getName()+"\n");
                    src.append("Quantity: " + product.getQuantity()+"\n");
                    src.append("Min Restock amount: " + (product.getMinimumStockLevel()-product.getQuantity())+"\n");
                }

        }
    }

    public void displayImage(JLabel imageArea, String image) throws IOException{

        String noImageFound = "no-image-available.jpg";

        BufferedImage img = null;
        try {    
            img = (BufferedImage)ImageIO.read(new File(image));
            Image actualimage = img.getScaledInstance(imageArea.getWidth(), imageArea.getHeight(), 0);
            imageArea.setIcon(new ImageIcon(actualimage));
            } 
        catch (IOException e) {
            img = (BufferedImage)ImageIO.read(new File(noImageFound));
            Image actualimage = img.getScaledInstance(imageArea.getWidth(), imageArea.getHeight(), 0);
            imageArea.setIcon(new ImageIcon(actualimage));
            System.out.println(e.getMessage());
            }          
        }

    public void loadFromFile(String fileName) throws FileNotFoundException, IOException, ParseException{
        Filename = fileName;
        String record;

        FileReader reader;

        reader = new FileReader(Filename);
        BufferedReader bin = new BufferedReader(reader);
        record = new String();

        while ((record = bin.readLine())!=null)
        {     
             switch(bin.readLine()){

                case "Electronic":
                  ElectronicProduct eProduct = new ElectronicProduct();          
                  Products.add(eProduct.loadFromFile(eProduct,bin)); 
                  break;

                case "Kitchen":
                  KitchenProduct kProduct = new KitchenProduct();          
                  Products.add(kProduct.loadFromFile(kProduct,bin)); 
                  break;

                case "Food":
                  FoodProduct fProduct = new FoodProduct();          
                  Products.add(fProduct.loadFromFile(fProduct,bin)); 
                  break;

                case "Book":
                  BookProduct bProduct = new BookProduct();          
                  Products.add(bProduct.loadFromFile(bProduct,bin)); 
                  break;

            }                  
        bin.close();
        bin =null;    
        }

    }

    public void saveToFile(boolean append, String fileName) throws IOException, ParseException{

        Filename = fileName+".txt";

        try{//This simply overwrites the file with a blank file
            FileWriter aWriter = new FileWriter(Filename,false);
        }
        catch(IOException ioe){
            System.out.println("Failed to wipe file");
        }
        //Now file writing begins
        FileWriter aWriter = new FileWriter(Filename,true);
        aWriter.write(System.getProperty("line.separator"));

        this.Products.forEach((product) -> {

            try {

                product.saveToFile(aWriter);

            } catch (IOException ex) {
                Logger.getLogger(ProductList.class.getName()).log(Level.SEVERE, null, ex);
            } catch (ParseException ex) {
                Logger.getLogger(ProductList.class.getName()).log(Level.SEVERE, null, ex);
            }
        });    

        try{

            aWriter.close();
        }

        catch(IOException ioe){
            System.out.println("Failed to save product list");
        }
    }


    public int generateProductID(){
        //This is superior to using ArrayList index as it, as that is dynamic and this is static once set. 
        Boolean NewIDFound = false;
        Boolean inUse = false;
        int potentialID;


        for(potentialID=0;NewIDFound==false;potentialID++){
            inUse=false;
            for(Product productToCompare: this.Products){

                if (potentialID==productToCompare.getID()){
                    inUse=true;
                    }

            }
            if(inUse!=true){
                NewIDFound=true;
                return potentialID;
            }
        }
        return potentialID;
    }

    public int calculateSuggestedQuantity(Product chosenProduct){   

        int suggestedQuantity = chosenProduct.getMaximumStockLevel()-chosenProduct.getQuantity();      

        return suggestedQuantity;
    }

    public String calculateAdvisedOrderDate(Product chosenProduct,int deliveryTime) throws ParseException{
            String date = "";
            date = chosenProduct.getExpiryDate();
            SimpleDateFormat formatter = new SimpleDateFormat("dd/MM/yy"); 
            Calendar cal = Calendar.getInstance();
            cal.setTime(formatter.parse(date));
            cal.add(Calendar.DATE,-deliveryTime);
            date = formatter.format(cal.getTime());        
            return date;

    }

    public double calculateOrderCost(Product chosenProduct, int orderQuantity){
        Double orderCost = 0.0;
        orderCost= orderQuantity * chosenProduct.getUnitCost();
        return orderCost;
    }



    public boolean checkIfNameInUse(String input){
        boolean alreadyInStock =false;
        for(Product product : this.Products){

            if(input.equals(product.getName())){
                alreadyInStock =true;
                break;
            }
        }
    return alreadyInStock;
    }

    public void updateLastOrderDate(Product input){

        String date = new SimpleDateFormat("dd/MM/yy").format(new Date());

                input.setDateOfLastOrderPlacement(date);      

            }

    public int setStockDefaultValue(String userInput){      
            int output = 0;       
                if(!userInput.equalsIgnoreCase("")){            
                    output=Integer.valueOf(userInput);                                   
                }                                                          
            return output;
        }

    public boolean confirmStockLevelsSet(Product chosenProduct){
        boolean reOrderPossible =true;  
            if(chosenProduct.getMaximumStockLevel()==0||chosenProduct.getMinimumStockLevel()==0){
            reOrderPossible=false;    
            }
        return reOrderPossible;
    }

    public boolean scanForLowStock(Product chosenProduct){
        boolean warningRequired = false;   
            if(chosenProduct.getQuantity()<chosenProduct.getMinimumStockLevel()){
            warningRequired=true;   
            } 
        return warningRequired;
    }

    public String getFilename() {
        return Filename;
    }

    public void setFilename(String Filename) {
        this.Filename = Filename;
    }


    public boolean checkIfFileExists(String fileName){
      String filename = fileName;

    File tempFile = new File(filename);
    boolean exists = tempFile.exists();

    return exists;
    }

}

EDIT 2

An extract of a child class is as follows, all child classes are similar with one or two unique attributes

public KitchenProduct loadFromFile(KitchenProduct kProduct, BufferedReader bin) throws IOException, ParseException{


    super.loadFromFile(kProduct,bin);

    kProduct.setFragile(super.convertTextToBoolean(bin.readLine()));

    return kProduct;

    }

below is an extract of the parent class

public Product loadFromFile(Product aProduct, BufferedReader bin) throws IOException, ParseException{


    aProduct.edit(
                    /*int ID*/Integer.valueOf(bin.readLine()),
                    /*String name*/bin.readLine(), 
                    /*String Supplier*/bin.readLine(), 
                    /*String deliverycomapyn*/bin.readLine(),
                    /*String caTEGORY*/bin.readLine(), 
                    /*double price*/Double.valueOf(bin.readLine()),
                    /*double unit cosre*/Double.valueOf(bin.readLine()),
                    /*int quantity*/Integer.valueOf(bin.readLine()),
                    /*int wiehgt*/Integer.valueOf(bin.readLine()),
                    /*int shelf life*/Integer.valueOf(bin.readLine()),
                    /*string date of last order*/bin.readLine(),
                    /*string image name*/bin.readLine(),
                    /*String min stock*/Integer.valueOf(bin.readLine()),
                    /*string max stock */Integer.valueOf(bin.readLine()));

    return aProduct;

}

product list is as follows, different child classes have different amount of attributes which is reflected in file and detected when reading.

6
Potatoes
Farm Wholesale
UPS
veg
5.0
0.5
100
50
14
18/01/20
food
30
300

1
spatula
amazon
UPS
Farm
2.0
0.3
10
1
4
20/01/20
kitchen
4
15
true
\$\endgroup\$
4
  • 4
    \$\begingroup\$ It would be better if the whole class was provided. \$\endgroup\$
    – pacmaninbw
    Feb 17, 2020 at 14:45
  • 1
    \$\begingroup\$ Thank you for your suggestion. \$\endgroup\$ Feb 17, 2020 at 16:21
  • \$\begingroup\$ It would be useful to have the Product class, and at least 1 derived product class. (Is eProduct.loadFromFile a virtual method or a static method? We can’t tell yet.). Also, a sample ProductList.txt file. After pasting your properly formatted code from your editor into the post, select the entire code and press Ctrl-K or Cmd-K to properly format it here, with the correct indentation. \$\endgroup\$
    – AJNeufeld
    Feb 17, 2020 at 16:27
  • \$\begingroup\$ please check edit \$\endgroup\$ Feb 17, 2020 at 16:33

3 Answers 3

13
\$\begingroup\$

First, a Code Review

Anti-pattern

Please don't do this:

record = new String();

This is (barring compiler heroics), creating a brand new empty string literal, when a perfectly good interned string literal is available:

record = "";

Using this interned literal "" prevents the creation of a number of identical tiny objects, and will improve the performance of the JVM. This is the same reason for preferring Integer.valueOf(some_string) over new Integer(some_string).

Primitives -vs- Objects

In Java, a primitive value is a light-weight - uh - value thing. It can stored in a register, or in a couple of bytes of memory. It has no "identity"; there is no difference between 5678 and 5678 ... they are simply equal values. In contrast, an Object is a heavy weight thing. It is always stored in the heap, requires dozens of bytes of memory, access via indirection, and has identity. Identity means new Integer(5678) and new Integer(5678) are different objects which contain the same numerical value; they exist in different areas of memory.

boolean is a value; Boolean is an object which holds a boolean value.

Boolean inUse = false;

Here, you are declaring a Boolean object, and attempting to storing a boolean value in that variable. The compiler autoboxes the false value into the singleton Boolean.FALSE for you, and stores a reference to that object in inUse. Additionally, it may increases a reference count of Boolean.FALSE, in order to properly manage the object heap, find unreferenced objects, and perform garbage collection.

If instead, you used:

boolean inUse = false;

the compiler would move a false value into a temporary register.

Compare the amount of behind the scenes work described in those paragraphs. Your code will run much faster when you use values instead of objects.

Reading further into your code, I see lines like these:

           /*int ID*/Integer.valueOf(bin.readLine())
           /*double price*/Double.valueOf(bin.readLine()),

The comments suggested these values are being passed to an int and a double parameter. However Integer.valueOf(...) returns an Integer object and Double.valueOf(...) returns a Double object. This means you've again asked the JVM to do more work than necessary. After reading the line, Integer.valueOf(...) converts the string into a int. (It internally uses int in the conversion process for efficiency.) After the conversion is complete, it boxes the int value into an Integer (allocated on the heap, if it cannot intern the value), and returns this object. Then, since the returned value is being passed to an int parameter, the Integer object is unboxed back into an int.

If instead you used:

           /*int ID*/Integer.parseInt(bin.readLine())
           /*double price*/Double.parseDouble(bin.readLine()),

these function would return an int and a double respectively. No boxing and subsequent unboxing is required. Again, less work means faster, more efficient code.

AutoCloseable

Most reader and writer objects are AutoCloseable, which means they can be used with Java's try-with-resources statement. This ensures the operating system resources are properly closed at the end of normal and exceptional execution paths. It also means you don't have to call bin.close(), if you write your code correctly.

Instead of:

FileReader reader;

reader = new FileReader(Filename);
BufferedReader bin = new BufferedReader(reader);

/* ... read code omitted for brevity ... */

bin.close();
bin =null;

You should write:

try (FileReader reader = new FileReader(Filename);
        BufferedReader bin = new BufferedReader(reader))
    {
    /* ... read code omitted for brevity ... */
    }

Notice the absence of bin.close(). Resources declared and opened inside try ( ... ) are closed for you at the end of the try block. If an exception is thrown inside the try, the resources are still properly closed for you; something which your code currently does not handle.

Coding Conventions

Variable and member names should not start with an upper case letter; class names start with upper case letters. In reader = new FileReader(Filename);, it looks like a Filename is a class.

Use spaces liberally, including after commas, around operators, after if, for, while, and before {. Compare:

if(chosenProduct.getMaximumStockLevel()==0||chosenProduct.getMinimumStockLevel()==0){
    reOrderPossible=false;    
}

and

if (chosenProduct.getMaximumStockLevel() == 0  ||  chosenProduct.getMinimumStockLevel() == 0) {
    reOrderPossible = false;    
}

Notice the double space around the || which helps suggest the order of precedence of the operations.

@Override

You have:

class Product {
    public Product loadFromFile(Product aProduct, BufferedReader bin) throws IOException, ParseException { ... }
    ...
}

and

class KitchenProduct : public Product {
    public Product loadFromFile(KitchenProduct kProduct, BufferedReader bin) throws IOException, ParseException { ... }
    ...
}

First thing to note is neither of these is a static method. This means they must be called with a receiver, the this object.

In your code, you call:

  KitchenProduct kProduct = new KitchenProduct();          
  ...( kProduct.loadFromFile(kProduct, bin) ); 

So when kProduct.loadFromFile(...) is invoked, this will be the value of kProduct from the caller, and the first argument will be the value of kProduct from the caller.

In KitchenProduct::loadFromFile(KitchenProduct kProduct, ...), you call kProduct.setFragile(...). You could have just as easily written this.setFragile(...) because this has the same value as the first argument to the function. Or less verbosely, you could simply write setFragile(...) because this. is implicit.

Improved code (step 1):

public KitchenProduct loadFromFile(KitchenProduct kProduct, BufferedReader bin) throws IOException, ParseException{
    super.loadFromFile(this, bin);

    setFragile(super.convertTextToBoolean(bin.readLine()));

    return this;
}

Similarly, Produce::loadFromFile(Product aProduct, ...) is being called with the receiver (this) the same as the first argument (aProduct), so it can be rewritten:

public Product loadFromFile(Product aProduct, BufferedReader bin) throws IOException, ParseException{

    edit(
         /* ... omitted for brevity ... */
    );

    return this;
}

Note that neither Product::loadFromFile(...) nor KitchenProduct::loadFromFile(...) are using their first argument anymore. These can be removed, leaving just the BufferedReader bin argument. At this point, the function arguments become identical, which allows us to use the annotation @Override.

class Product {

    public Product loadFromFile(BufferedReader bin) throws IOException, ParseException {
        edit( /* ... omitted for brevity ... */ );
        return this;
    }

    ...
}

class KitchenProduct {

    @Override
    public KitchenProduct loadFromFile(BufferedReader bin) throws IOException, ParseException {
        super.loadFromFile(bin)
        setFragile(convertTextToBoolean(bin.readLine()));
        return this;
    }

    ...
}


...

    case "Kitchen":
        KitchenProduct kProduct = new KitchenProduct();          
        Products.add(kProduct.loadFromFile(bin)); 
        break;

Improving the Load

We're now ready to start improving the dynamic loading code.

First of all, all Product classes now @Override a common base class loadFromFile method. This means we can call the method on any derived class from the base class, without knowing what kind of base class we have.

So this code:

switch(bin.readLine()){

    case "Electronic":
      ElectronicProduct eProduct = new ElectronicProduct();          
      Products.add(eProduct.loadFromFile(eProduct,bin)); 
      break;

    case "Kitchen":
      KitchenProduct kProduct = new KitchenProduct();          
      Products.add(kProduct.loadFromFile(kProduct,bin)); 
      break;

    case "Food":
      FoodProduct fProduct = new FoodProduct();          
      Products.add(fProduct.loadFromFile(fProduct,bin)); 
      break;

    case "Book":
      BookProduct bProduct = new BookProduct();          
      Products.add(bProduct.loadFromFile(bProduct,bin)); 
      break;
}

can now be written as:

Product product = null;

switch (bin.readLine()) {
    case "Electronic":
        product = new ElectronicProduct();          
        break;

    case "Kitchen":
        product = new KitchenProduct();          
        break;

    case "Food":
        product = new FoodProduct();          
        break;

    case "Book":
        product = new BookProduct();          
        break;
}

if (product != null) {
    Products.add(product.loadFromFile(bin));
}

As a further improvement, I'd make all of the loadFromFile(...) methods return void, and write:

if (product != null) {
    product.loadFromFile(bin);
    Products.add(product);
}

Now, you can create a factory method which turns a string into the correct product class, and write the code as:

String product_name = bin.readLine();
Product product = create_product(product_name);
product.loadFromFile(bin);
Products.add(product);

And your factory method might look like:

private static Product create_product(String product_name) {
    switch (product_name) {
        case "Electronic": return new ElectronicProduct();
        case "Kitchen":    return new KitchenProduct();
        case "Food":       return new FoodProduct();
        case "Book":       return new BookProduct();
        default: throw new UnknownProductException(product_name);
    }
}

Dynamic Construction

It has been a long road, but to finish, this is one way to implement dynamic construction (untested):

@SuppressWarnings("unchecked")
private static Product create_product(String product_name) {
    try {
        Class<?> product_class = Class.forName(product_name + "Product");
        return (Product) product_class.getDeclaredConstructor().newInstance();
    } except (ReflectiveOperationException e) {
        throw new UnknownProductException(product_name);
    }
}

You will need "your.package." + product_name + "Product" if your derived Product classes exist in a package.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for your detailed response to this. I am currently working towards a tight deadline so once I have completed vital tasks I will read through this in depth and apply to my code (and replace this comment) \$\endgroup\$ Feb 18, 2020 at 15:40
4
\$\begingroup\$

Is inheritance really the answer?

I think it's been covered that you could dynamically build your product instances, but it's not really clear to me why you'd need to. The object model feels a bit wrong to me. From what I can see you're reading all of the objects in, then storing them in an ArrayList<Product>. There doesn't appear to be a lot of processing (although that could be in the code you've not shared), so I can't help but wonder if you're really getting much more from having these classes than you would get with a simple map of field types to values. So, you'd have a ProductDefinition something like:

ProductDefinition
    ProductType  productType
    OrderedList<Field> fields

FieldDefinition
    String fieldName       // Name used to retrieve field
    FieldType fieldType    // String/Boolean/Integer etc

You could then simply maintain a Map of ProductIdentificationStrings, so given a "Kitchen" product identifier, the Matching product definition is used to determine which fields to read... these would then be stored in a Product which has it's own map of field/values... This lends itself quite readily to a table driven approach whereby adding support for a new type of product is really just about creating the new product definition entries...

Other Stuff

  • Seperation of concerns seems a bit skewed. Your ProductList knows about swing and also about reading/writing products out to files. This seems like too much responsibility. You really want to try to separate the front end from the back a bit more.
  • Quite a lot of your functions define variables that really aren't needed. They don't add extra clarity. If a function can be one line, simply make it one line... Instead of

    public double calculateOrderCost(Product chosenProduct, int orderQuantity){
        Double orderCost = 0.0;
        orderCost= orderQuantity * chosenProduct.getUnitCost();
        return orderCost;
    }
    

    Just do:

    public double calculateOrderCost(Product chosenProduct, int orderQuantity){
        return orderQuantity * chosenProduct.getUnitCost();
    }
    

    Similarly, consider simplifying your ifs, rather than if (x || y) return true;return false; just do return x || y. The function name should indicate what the returned value represents.

  • Consider making your product file more self describing. Files like this can be a pain. If you're writing them out in text anyway, then using something like YAML can make it easier to understand the file if things go wrong and you need to fix it in a text editor. In the example you've posted (which may have been redacted), it looks like the product type is actually the 2nd/3rd from last field depending on the product type... making it the first line would seem more natural.

\$\endgroup\$
3
\$\begingroup\$

I think there are two choices with little or no in-betweens:

  1. formally specify the products and use a parser
  2. keep the current behavior, which means determining the product and switching.

so in the end, you have to either take a big step and create a formal description and a parser (parser generation is a university course, mind you). Or you'll have to perform some smart but elaborate branching.


A possible enhancement is to first load into memory and then switch. This would decouple the file handling from the parsing of the products. That way you can also test the parsing of the products without relying on files (!).


Another possible enhancement is to use factory methods / classes. In that case a factory is used to aggregate the various data of a product performing validation on the input. Then it can be used to create the actual (Potatoes) object after making sure that all necessary components are present and not in conflict.

This could also make it easier to use one factory for multiple products, so you can group together the handling of similar products. Of course, you may need to create a product -> factory mapping - but that should not be hard. In Java you can also use reflection to then instantiate the right factory.


Let me end by: never trust your input! I see too little input checking and no clear strategy (such as throwing a checked exception) in case the input is invalid.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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