# Refectoring - BadSmells



## Margie (7. Jan 2010)

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 (7. Jan 2010)

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 (7. Jan 2010)

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 (7. Jan 2010)

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 (7. Jan 2010)

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 (7. Jan 2010)

Landei hat gesagt.:


> 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 (7. Jan 2010)

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 (7. Jan 2010)

eRaaaa hat gesagt.:


> 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 (7. Jan 2010)

Landei hat gesagt.:


> Es gib einen leeren Konstruktor



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


----------



## Margie (7. Jan 2010)

Landei hat gesagt.:


> 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 )


----------

