4
\$\begingroup\$

I've used Decorator to create a Dataset class used to build a Dataset Basic Empty (ConcreteComponent) and Iris class (ConcreteDecorated) used to populate it.

Decorator allows a user to add new functionality to an existing object without altering its structure

For example, Iris has a its own way to load and store the data in a dataset structure. I've also added the name features to track the string name of their features.

My purpose is to add more ConcreteDecorator, like Iris, that they have a own way to load and store file/data and to track the name features. The code runs, but my "doubts" is that a dataset is an empty table (define only the data structure) and the population depends on a concrete decorator that uses it. It seems to me that does not respect the SOLID principles (for example DIP). What do you think about this simple implementation?

Component

public interface Dataset
{ 
    public double Distanza(int i, ArrayList<Double> centroide);     
    public double getCella(int i, int j);
    public void inizializzaTabella();   
    public void stampaDataset();
    public void setColonna( ArrayList<Double> colonnaValori);
    public ArrayList<ArrayList<Double>> getTabella();
    public int getnFeature(); //ok
    public int getnRecord() ;
    public void setnFeature(int nFeature); 
    public void setnRecord(int nRecord);    
    public void setTabella(int Colonna, Double Valore); 
    public String getNomeDataset(); 
    public void setNomeDataset(String nomeDataset);
    public double[][] toMatrix();
    public ArrayList<Integer> getFeatureUsate(); 
    public void setNomiFeature(String[] nomiFeature);

}

Concrete component

public class DatasetConcreto implements Dataset
{
    private int nFeature;
    private int nRecord;
    private ArrayList<Integer> featureUsate; // quelle selezionate
    private String nomeDataset;
    private String[] nomiFeature;

    //public double Tabella [][];
    private ArrayList<ArrayList<Double>> Mat;

    public double Distanza(int i, ArrayList<Double> centroide)
    {
        double Sum=0;
        for(int j=0; j<nFeature; j++)
            Sum+=Math.pow((centroide.get(j) - Mat.get(j).get(i)), 2);
        return Math.sqrt(Sum);
    }

    public double getCella(int i, int j)
    {
        return Mat.get(j).get(i);
    }

    public void inizializzaTabella()
    {
       Mat = new ArrayList<ArrayList<Double>>();
       featureUsate = new ArrayList<Integer>();

       for(int i=0; i< nFeature; i++)
       {
           Mat.add(new ArrayList<Double>());
           featureUsate.add(i); // uso tutte le feature
       }
    }


    public void stampaDataset()
    {
        System.out.println("Le feature selezionate sono");
        for(int j = 0; j<nFeature; j++)
            System.out.println(nomiFeature[j].toString());

        for(int i=0; i< nRecord; i++)
        {
            for(int j=0; j < nFeature; j++)
            {
                System.out.print( Mat.get(j).get(i)+ " ");
            }
            System.out.println("\n");
        }
    }

    public ArrayList<Double> getRecord(int indiceRandom)
    {
        ArrayList<Double> record = new ArrayList<Double>();
        for(int i=0; i<nFeature; i++)
            record.add( Mat.get(i).get(indiceRandom));

        return record;
    }

    public DatasetConcreto(int nFeature, String Nome) 
    {       
        setnFeature(4);
        setNomeDataset("Iris.data");
        inizializzaTabella();
    }

    public DatasetConcreto(ArrayList<ArrayList<Double>> MatInput, ArrayList<Integer> featureSelezionate, int nRecord)
    {
        Mat = new ArrayList<ArrayList<Double>>();
        this.featureUsate = new ArrayList<Integer>(featureSelezionate);
        this.nRecord = nRecord;
        this.setnFeature(featureSelezionate.size());

        for(int i=0; i<featureSelezionate.size(); i++)
            setColonna( MatInput.get( featureSelezionate.get(i)));

    }

    public DatasetConcreto() {
    }


    public void setColonna( ArrayList<Double> colonnaValori)
    {
        this.Mat.add(colonnaValori);
    }

    public ArrayList<ArrayList<Double>> getTabella()
    {
        return this.Mat;
    }
    public int getnFeature() {
        return this.nFeature;
    }
    public int getnRecord() {
        return this.nRecord;
    }
    public void setnFeature(int nFeature) 
    {
        this.nFeature = nFeature;
        return;
    }
    public void setnRecord(int nRecord) {
        this.nRecord=nRecord;
        return;
    }

    public void setTabella(int Colonna, Double Valore) 
    {
        Mat.get(Colonna).add(Valore);
    }



    public String getNomeDataset() {
        return this.nomeDataset;
    }

    public void setNomeDataset(String nomeDataset) {
         this.nomeDataset = new String(nomeDataset);
    }

    public double[][] toMatrix()
    {
        double[][] matrix = new double[this.getnRecord()][this.getnFeature()];
        for(int i=0; i< nRecord; i++)
        {
            for(int j=0; j < nFeature; j++)
            {
                matrix[i][j] = Mat.get(j).get(i);
            }
        }
        return matrix;
    }

    public ArrayList<Integer> getFeatureUsate()
    {return this.featureUsate;}

    @Override
    public void setNomiFeature(String[] nomifeature) 
    {
        this.nomiFeature = new String[nFeature];
        this.nomiFeature = nomifeature;

    }
}

Decorator

public abstract class DatasetDecorator implements Dataset {
protected Dataset dataset;

public DatasetDecorator(Dataset dataset){
this.dataset=dataset;
}

@Override
public double Distanza(int i, ArrayList<Double> centroide) {
    return this.dataset.Distanza(i, centroide);
}

@Override
public double getCella(int i, int j) {
    return this.dataset.getCella(i, j);
}

@Override
public void inizializzaTabella() {
    this.dataset.inizializzaTabella();
}

@Override
public void stampaDataset() {
    this.dataset.stampaDataset();
}

@Override
public void setColonna(ArrayList<Double> colonnaValori) {
    this.dataset.setColonna(colonnaValori);
}

@Override
public ArrayList<ArrayList<Double>> getTabella() {
    return this.dataset.getTabella();
}

@Override
public int getnFeature() {
    return this.dataset.getnFeature();
}

@Override
public int getnRecord() {
        return this.dataset.getnRecord();
}

@Override
public void setnFeature(int nFeature) {
    this.dataset.setnFeature(nFeature); 
}

@Override
public void setnRecord(int nRecord) {
    this.dataset.setnRecord(nRecord);
}

@Override
public void setTabella(int Colonna, Double Valore) {
    this.dataset.setTabella(Colonna, Valore);
}

@Override
public String getNomeDataset() {
    return this.dataset.getNomeDataset();
}

@Override
public void setNomeDataset(String nomeDataset) {
this.dataset.setNomeDataset(nomeDataset);   
}

@Override
public double[][] toMatrix() {
    return this.dataset.toMatrix();
}

@Override
public ArrayList<Integer> getFeatureUsate() {
    return this.dataset.getFeatureUsate();
}

public void setNomiFeature(String[] nomifeature) 
{this.dataset.setNomiFeature(nomifeature);}

}

Concrete decorator

public class Iris extends DatasetDecorator
{       
     private final String[] nomiFeature = {"LunghezzaSepalo",
                                                "LarghezzaSetalo",
                                                 "LunghezzaPetalo",
                                                 "LarghezzaPetalo"};    

    public Iris(String NomeFile, Dataset dataset) throws IOException
    {
        super(dataset);
        super.dataset.setnFeature(4);
        super.dataset.setNomiFeature(nomiFeature);
        super.dataset.inizializzaTabella(); //For init dataset from here                    
        super.dataset.setnRecord( CaricaDataset(NomeFile) );
    }
    protected int CaricaDataset(String pathFile) throws IOException
    {
        int iRecord = 0;
        BufferedReader bufferLetto = null;
        String line = "";
        String cvsSplitBy = ",";

        try {
            bufferLetto = new BufferedReader(new FileReader(pathFile));

            while ((line = bufferLetto.readLine()) != null) 
            {
                if (line.length() > 0) 
                {
                    String[] cell = line.split(cvsSplitBy);
                    this.addRow(cell);
                    iRecord++;
                }
            }
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        } catch (IOException e) {
            e.printStackTrace();
        } finally {
            if (bufferLetto != null) {
                try {
                    bufferLetto.close();
                } catch (IOException e) {
                    e.printStackTrace();
                }
            }
        }
        return iRecord;
    }
    // new row
    public void addRow(Object cell[]) 
    {
        System.out.println(this.nomiFeature.length);
        for(int i=0; i<this.nomiFeature.length; i++)
            super.dataset.setTabella(i, Double.parseDouble(cell[i].toString()));
    }


}   

Test client

public class Client {

    public static void main(String[] args) throws IOException 
    {
        Dataset ds = new DatasetConcreto();
        Dataset iris = new Iris("./src/Iris.data", ds);
        ds.stampaDataset();
    }

}
\$\endgroup\$
  • \$\begingroup\$ This looks to me like pseudocode. DatasetConcreto or DatasetDecorator are very generic names, typical for examples in a book. They don't tell anything about what the class is, thus I don't believe it's real code because no one would use such names. \$\endgroup\$ – t3chb0t Feb 2 '17 at 11:30
  • \$\begingroup\$ @t3chb0t Nono, it's Java code, try it. However you're right, but I was following the Decarator Pattern \$\endgroup\$ – Giuseppe Accardo Feb 2 '17 at 11:45
  • \$\begingroup\$ I would not add loading functionality to the DataSet. Instead, create some DataSetReader and implement what you need there. \$\endgroup\$ – RobAu Feb 2 '17 at 14:31
  • 2
    \$\begingroup\$ Design patterns are not goals, they are means :) \$\endgroup\$ – RobAu Feb 2 '17 at 19:10
  • 1
    \$\begingroup\$ @RobAu Yes, infact I've used Template Methods, where I've a basic dataset and iris extends this dataset implementing for example 'loadDataset' specific for each different dataset and It was better \$\endgroup\$ – Giuseppe Accardo Feb 2 '17 at 19:51
2
\$\begingroup\$

Maintenance concerns

Let's start with the really "simple" stuff that has been skipped in favor of reviewing the design in Peter Rader's answer:

  • Your method names are a mess of English and Italian thrown into a blender. This introduces additional cognitive load when processing those names. I strongly suggest you stick to naming your methods in one or the other. The same applies to variables.

  • Fields, Variables, Parameters in java are named in camelCase. This btw. explains the messy syntax highlighting in your question. The pretty-printing code assumes certain things when it sees on a question, one being that names starting with an uppercase letter are classes and not variables.

  • As mentioned in the other answer, your interface relies on certain implementations. Instead of returning an ArrayList you should return a List. The same applies for parameter types when you pass them into methods (see Distanza), as well as field types where possible (see Mat in DatasetConcreto)

  • nFeature and nRecord are named ... dangerously. It seems the n stands for number. Interestingly these two are the only place this happens. Nothing like sNomeDataset or setdlColonna ... long story short: don't do that. It's needless complication.

Inconsistencies

There's multiple inconsistencies in how the code is built:

  • You're violating java conventions for braces in DatasetConcreto by putting the opening brace on a dedicated line. But you don't do that in DatasetDecorator

  • Sometimes oneliner for-loop bodies have no braces (see DatasetConcreto#Distanza), sometimes they do have the braces (see DatasetConcreto#stampaDataset())
    I strongly recommend always placing the braces

  • Java conventions recommend putting the Constructor in the first spot after the field declarations. For a class it's important to know which fields there are in it and how they are initialized. Everything else comes after that. Yet in DatasetConcreto the constructor is somewhere in the middle... Interestingly that's not the case in DatasetDecorator and Iris

Simplifications

in Iris#CaricaDataset the "old" java way of handling resources and IO is used. Also that method unnecessarily declares throws IOException. I strongly suggest you read up on try-with-resources. Additionally note that FileNotFoundException is a subclass of IOException. Even if it weren't you're not handling these exceptions any different from one another:

try (BufferedReader bufferLetto = new BufferedReader(new Filereader(pathFile))) {
    // ...
} catch (FileNotFoundException | IOException e) {
    e.printStacktrace();
}

note that this is not the code's full potential. the "new" java io package java.nio available since java 7 introduced Files.newBufferedReader:

try (BufferedReader bufferLetto = Files.newBufferedReader(Paths.get(pathFile))) {
    String line = "";
    int iRecord;
    while ((line = bufferLetto.readLine()) != null) {
        this.addRow(line.split(cvsSplitBy));
        iRecord++;
    }
    return iRecord;
} catch (IOException e) {
    e.printStackTrace();
}

If you didn't need the separate save responsibility for the number of records in your dataset, this method could be tremendously simplified with Files.lines to the following:

try (Stream<String> lines = Files.lines(Paths.get(pathFile))) {
    lines.map(l -> l.split(cvsSplitBy)).forEach(this::addRow);
} catch (IOException e) {
    e.printStackTrace();
}
\$\endgroup\$
  • \$\begingroup\$ May drop public from interface's methods. Also I think about Set<List<Double>> getTabella(); but I am not sure if null is a valid value. If null is not a vaild value: the method should be removed to use toMatrix() instead. And many more ... \$\endgroup\$ – Peter Rader Feb 4 '17 at 13:07
  • \$\begingroup\$ @PeterRader yup... basically too many to mention them all at once without completely overloading anyone \$\endgroup\$ – Vogel612 Feb 4 '17 at 13:10
  • \$\begingroup\$ @PeterRader Thanks for answer (Peter and Vogek)! Yes, you right and sorry for this mistakes, but I'm a beginner Java programmer. However, As regards the use of Decorator, I implemented Dataset and Iris by using Template Methods pattern that simplifies code and eliminates some problems (like interfaces. This pattern allows me to create, having a basic skeleton of a dataset, more concrete datasets that need to implement CaricaDataset. I ask you experts, is it a good idea? \$\endgroup\$ – Giuseppe Accardo Feb 4 '17 at 15:07
  • \$\begingroup\$ @giuseppeaccardo Carica means Load/Read? Lets learn by play, I ask a question and you answer. What is the responsibility of the method CaricaDataset and what is the responsibility of the class the method CaricaDataset is in? \$\endgroup\$ – Peter Rader Feb 4 '17 at 20:05
4
\$\begingroup\$

The interface

Only some small hints: You are using ArrayList but a List, Collection or a Set is more open for extensions (D and O in SOLID). The interface is a big one, nFeature and nomeDataset should have its own interface (I in SOLID). Using setTabella you can add a null-value but toMatrix can not return a null-value, this breaks the SVOT (S and L in SOLID).

The implementation

Only some small hints: You have a list (A singular) of lists (B plural) but the lists (B plural) can have different lengths. If the columns length of row1 differ from the length in row2 you can not say if row1 or row2 tell the truth - this breaks the SSOT (S in SOLID) - at least when you use the toMatrix() you have to decide who tell the truth and it is more than a function to a matrix.

The decorator

Only one hint: A decorator decorates exactly one object but the field to store the dataset is not final, you can switch the object, you modify the decorator what is not allowed (O in SOLID).

I stop right now because I think it is a little bit more than small hints :)

EDIT: Do not be confused, S in SOLID means SRP(!) and SVOT and/or SSOT is a decision what kind of SRP to use.

\$\endgroup\$
  • \$\begingroup\$ As you arguing with the SOLID principles (what I also prefer) I want to notice that they do not directly say do this or don't do that. They only say: If you violate me then you have to expect these drawbacks. If you do not want the drawbacks you have to think about how to solve this. And that can be a complex task. \$\endgroup\$ – oopexpert Feb 4 '17 at 12:13
  • \$\begingroup\$ Furthermore I cannot really grap SVOT and SSOT and their relationship to SRP. I'll have my detailed resaerch on this. But I think mentioning SRP is sufficient as whatever decision made should be transparent to SRP. \$\endgroup\$ – oopexpert Feb 4 '17 at 12:19
  • \$\begingroup\$ @oopexpert Every principal covered by specification is strict! Like a NTP-Server as SSOT for money movement (actually have no good example so faar). Scientific work is collective work - could you please share your results of research in a blog? \$\endgroup\$ – Peter Rader Feb 4 '17 at 13:00

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.