Refectoring - BadSmells

[Margie]

Hallo an Alle !

Im Zuge einer Aufgabe in Software Design , muss ich Refactorn .
Leider habe ich noch keinen Erfahrung damit , und hoffe ,dass mir hier wer weiter helfen kann

Zuerst muss ich nach BadSmells und BadPractises suchen , und hab keine Ahnung wie ich das angehn soll .

Hier die 1. Klasse , hoffe es kann mir jemand weiter helfen

package at.fhj.itm;

import java.util.ArrayList;

public class Project
{
        protected String name;
        protected ArrayList<String> propertyName = new ArrayList<String>();
        protected ArrayList<String> propertyLocation =  new ArrayList<String>();
        protected ArrayList<String> targetName =  new ArrayList<String>();
        protected ArrayList<String> targetDescription =  new ArrayList<String>();
        protected ArrayList<String> dependentTargetNames = new ArrayList<String>();
        protected String defaultTargetName;
        protected String defaultTargetDescription;
        protected ArrayList<ArrayList<Task>> targetsTasks = new ArrayList<ArrayList<Task>>();
       
        public Project()
        {
        }
       
        public Project(String name, ArrayList<String> properties, ArrayList<String> propertylocations,
                        ArrayList<String> targetname, ArrayList<String> targetdesc, ArrayList<String> dependentargets,
                        String defaulttarget, String defaulttargetdesc, ArrayList<ArrayList<Task>> targetstasks)
        {
                this.name=name;
                this.propertyName = properties;
                this.propertyLocation = propertylocations;
                this.targetName = targetname;
                this.targetDescription = targetdesc;
                this.dependentTargetNames = dependentargets;
                this.defaultTargetName = defaulttarget;
                this.defaultTargetDescription = defaulttargetdesc;
                this.targetsTasks = targetstasks;
        }
       
       
        public String toXml()
        {
                String s = "<project name=\"" + name + "\"" +
                                " default=\"" + defaultTargetName + "\">\n";
               
                s += "\n";
               
                for (int i = 0; i<propertyName.size();i++)
                {
                        s+=("\t<property name=\"" + propertyName.get(i) +"\" location=\"" + propertyLocation.get(i) + "\"/>")+ "\n";
                }
               
                s += "\n";
               
                for (int i = 0; i<targetName.size();i++)
                {
                        s += "\t<target name=\"" + targetName.get(i) + "\" ";
                       
                        if (targetDescription.size() > 0)
                        {
                                s += "description=\"" + targetDescription.get(i) + "\" ";
                        }
                       
                        if (dependentTargetNames.size() > 0)
                        {
                                s += "depends=\"";
                                for(int j=0; j<dependentTargetNames.size();j++)
                                {
                                        s += dependentTargetNames.get(j) + " ";
                                }
                                s += "\"";
                        }
                       
                        s += ">\n";
                                               
                        ArrayList<Task> targetTask = targetsTasks.get(i);
                        for (int j=0; j<targetTask.size(); j++)
                        {
                                s += "\t\t" + targetTask.get(j).toXml();
                        }
                        s += "\n";
                       
                        s += "\t</target>\n\n";
                }
               
                s += "</project>";
               
                return s;
        }
}

 

[0din]

du wirst ja wissn was du suchen musst... also warum schauste net einfach erstmal nach smells un dann schritt für schritt nach anti-pattern?
als tipp, schau dir mal den constructor an...

 

[Margie]

Hab den Konstrukter Protected gesetzt.
Dann ist es sinnvol Stringbuiler statt den +s zu verwenden ?

die ArrayList hab ich auch private gesetzt

und soll ich die schleifen in eine eigene KLasse auslagern ?

ja ich weiß ungefähr wonach ich suchen soll , aber da dies mein erstes Refactorn ist ...

danke

 

[kama]

Hallo,

bevor Du anfängst zu "Refaktorisieren" sind Unit Tests angesagt, damit Du nach dem "Refaktorisieren" überhaupt noch weißt, ob alles noch funktioniert...

Weiterhin fällt mir auf das der Konstruktor zu viele Parameter hat ...solltest Du mal drüber nachdenken...

Gruß
Karl Heinz Marbaise

 

[Landei]

Statt ArrayList<String> propertyName = new ArrayList<String>(); usw. sollte man das Interface verwenden, also
List<String> propertyName = new ArrayList<String>();
Genauso für die Konstruktorparameter.
ArrayList<ArrayList<X>>> ist ganz böse, muss List<List<X>> sein(mit ArrayList<List<X>> initialisiert), aber vermutlich ist das sowieso eine ungeeignete Datenstruktur.

Die ganzen Stringadditionen in der toXML-Methode sind schlecht. Wenn das halbwegs zeitkritischer Code ist, gehört da ein StringBuilder verwendet.

 

[eRaaaa]

Statt ArrayList<String> propertyName = new ArrayList<String>(); usw. sollte man das Interface verwenden, also
List<String> propertyName = new ArrayList<String>();

Zusatz:
Aber Allgemein macht`s doch hier auch gar keinen Sinn die Listen direkt zu initialisieren, wenn man eh davon ausgeht, dass die Listen im Konstruktor injiziert werden oder?

 

[Margie]

Naja , ich habe ja nicht das ganze Projekt gepostet , gibt ja noch weitere Klassen . und so machts dann schon Sinn die gleich zu initalisieren ...

 

[Landei]

Zusatz:
Aber Allgemein macht`s doch hier auch gar keinen Sinn die Listen direkt zu initialisieren, wenn man eh davon ausgeht, dass die Listen im Konstruktor injiziert werden oder?

Es gib einen leeren Konstruktor, da wären sie sonst null. Das "sauberste" wäre, alle Listen final zu machen und in beiden Konstruktoren zu initialisieren (einmal mit leeren Listen, das andere mal mit den Parametern)

 

[eRaaaa]

Es gib einen leeren Konstruktor

Ich hätte schwören können, der war eben noch nicht da Dann ignoriert meinen Einwand !

 

[Margie]

Es gib einen leeren Konstruktor, da wären sie sonst null. Das "sauberste" wäre, alle Listen final zu machen und in beiden Konstruktoren zu initialisieren (einmal mit leeren Listen, das andere mal mit den Parametern)

du meinst zuerst mit final die Lists normal initialisieren und im noch leeren Konstrukter diese einfach nochmal , aber ohne Parameter
Da gibt er bei mir den Fehler aus im leeren Konstruktor ( the blank final field name may not have been initaliziesed )