Refectoring - BadSmells

Margie

Mitglied
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

Java:
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

Bekanntes Mitglied
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

Mitglied
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

Top Contributor
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

Top Contributor
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.
 
Zuletzt bearbeitet:

eRaaaa

Top Contributor
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

Mitglied
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

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

Margie

Mitglied
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 )
 
Zuletzt bearbeitet:

Neue Themen


Oben