Meinung zum verwendeten Design Pattern

[jCoder1984]

Hallo in die Runde.
Ich habe heute keine konkrete Frage, sondern möchte gerne eure Meinung hören. Ich bin dabei ein kleines Programm zu schreiben, was eine CSV Datei mit Sportergebnissen einliest und daraus Objekte baut - zum Beispiel Season, Competition, Match, Team usw.

Hier mal beispielhaft ein paar Zeilen Code :
[CODE lang="java" title="Competition Object"]@NoArgsConstructor(access = AccessLevel.PRIVATE)
@EqualsAndHashCode
@ToString
@Entity
public class Competition implements IModelObject {

@Id
@GeneratedValue(strategy = GenerationType.AUTO)
@EqualsAndHashCode.Exclude
private Integer id;


@Getter
private String name;

@ManyToOne(fetch = FetchType.EAGER)
@EqualsAndHashCode.Exclude
private Season season;

/**
* constructor
*
* @param name the competition name
* @param season the season reference
*/
protected Competition(@NonNull String name, Season season) {
this.name = name;
this.season = season;
}

public Optional<Season> getSeason() {
return Optional.ofNullable(season);
}
}[/CODE]
[CODE lang="java" title="Hier der Competition Builder :"]public class CommonCompetitionBuilder extends CompetitionBuilder {

private static final Logger logger = LogManager.getLogger(CommonCompetitionBuilder.class);

@Override
public boolean validateData() {
boolean validationResult = true;

// check required parameter
if (name == null || name.isEmpty()) {
logger.error("the competition name is required and can't be empty or NULL");
validationResult = false;
}

// check optional parameter
if (season == null) {
logger.warn("the season object is NULL and will be ignored ");
}

return validationResult;
}
}[/CODE]

[CODE lang="java" title="Hier die factory"]@Service
public class CommonModelFactoryService extends ModelFactoryService {

@Autowired
@Override
public void init() {
associationBuilder = new CommonAssociationBuilder();
seasonBuilder = new CommonSeasonBuilder();
competitionBuilder = new CommonCompetitionBuilder();
...
}
}
[/CODE]

Einstiegspunkt ist eine weitere Klasse - Importer. Der Importer liest eine CSV Datei und erzeugt daraus ImportedData und reicht diese an die factory weiter :

[CODE lang="java" title="CSV importer"]public class CSVImporter implements IImporter {

private static final Logger logger = LogManager.getLogger(CSVImporter.class);
...


@Autowired
ModelFactoryService modelFactoryService;

@NonNull
private final Path csvFilePath;
private final String associationName;

/**
* @param csvFilePath - the path to the csv file
* @param associationName - the association name
*/
public CSVImporter(Path csvFilePath, String associationName) {
this.csvFilePath = csvFilePath;
this.associationName = associationName;
}

@Override
public List<IImportedData> importData() {
logger.info("import csv file {}", csvFilePath.getFileName());
...[/CODE]


Ich hoffe damit ist so in etwas klar was ich vorhabe. Funktional ist alles super.

Aber meine Bedenken :

Wenn ich die Daten können nicht nur aus einer SCV Datei kommen sondern auch aus einer anderen Schnittstelle oder sollen später auch vom User eingegeben wernden können. Die vewendetetn Builder in der Model Factory validieren ja die Daten und ertsellen daraus dann entweder ein "ModelObject" oder eben nicht.

Das funktionert auch schon. Meine Frage ist aber ob dies so in Ordnung ist oder wo es Stellschrauben gibt etwas besser zu machen.

Vielen lieben Dank für die hinweise

 

[mrBrown]

Naja, allzu viel kann man bei den knappen Ausschnitten nicht sagen, aber zu dem was man sieht sag ich mal was. Aber mehr als so "formale" Dinge kann man da nicht beurteilen 🙂

• IModelObject
• CommonModelFactoryService
• ModelFactoryService

Das sind schon mit die schlimmsten Namen, die ich mir vorstellen kann 🙂

---

Was soll CommonCompetitionBuilder machen, baut der eine Spezialisierung von Competition, die der normale CompetitionBuilder nicht bauen kann? Builder in Verbindung mit Vererbung können recht schwierig und umständlich sein, da das ganze zu sehe wäre interessant.

validateData darin würde ich persönlich weg lassen. Der Builder kann einfach direkt dafür sorgen, dass nur valide Daten zulässig sind. Der kann zB erzwingen, dass ein Name gesetzt wird, und dabei oder beim Bauen dann eine Exception werfen. Eine extra validate-Methode macht das oft nur umständlicher, vor allem weil man dann immer dran denken muss, die auch zu benutzen.

---

Was soll CommonModelFactoryService machen? Das, was man dort sieht, ist eigentlich nur Unsinn.

Wenn die Klasse Beans der anderen Typen bereitstellen soll, gibt es dafür bessere Wege (zB Configuration mit @Bean-Methoden), aber eine Autowired Methode ohne Parameter, die Instanzvariablen setzt, ist eher sehr schlechte Praxis.

---

CSVImporter sieht zumindest komisch aus, da Objekte wohl "per Hand" erzeugt werden sollen, aber Spring gleichzeitig auch noch Felder injecten soll. Sowohl die Fieldinjection als auch die Mischung von Spring und "per Hand" ist nicht wirklich gut, und führt auf lange Sicht potentiell nur zu Problemen.

---

@Id
@GeneratedValue(strategy = GenerationType.AUTO)
@EqualsAndHashCode.Exclude
private Integer id;

Das ist etwas, was man vermeiden sollte – damit gibt es auf Datenbank- und auf Applikationsebenen zwei unterschiedliche Definitionen für "gleich". Im Idealfall verwendet man für Entitäten immer nur die ID für Gleichheit, und ignoriert alle anderen Felder.

 

[jCoder1984]

Das ist etwas, was man vermeiden sollte – damit gibt es auf Datenbank- und auf Applikationsebenen zwei unterschiedliche Definitionen für "gleich". Im Idealfall verwendet man für Entitäten immer nur die ID für Gleichheit, und ignoriert alle anderen Felder.

Danke für deine Antwort. Ich werde versuchen etwas zu ändern.

Eine Frage habe ich noch :
Bezüglich der Entitäten. Es soll zu Beginn alle Objekte aus der Datenbank gelesen. Der Importer erzeugt dann beim Parsen der CSV Datei neue Objekte sofern sie noch nicht existieren und diese "neuen" Objekte werden dann in der Datenbank gespeichert.
Somit kann ich sicherstellen, dass jedes Objekt nur einmal vorhanden ist.

 

[mrBrown]

Bezüglich der Entitäten. Es soll zu Beginn alle Objekte aus der Datenbank gelesen. Der Importer erzeugt dann beim Parsen der CSV Datei neue Objekte sofern sie noch nicht existieren und diese "neuen" Objekte werden dann in der Datenbank gespeichert.
Somit kann ich sicherstellen, dass jedes Objekt nur einmal vorhanden ist.

Ich hab gar keinen Zweifel daran, dass du in deiner Applikation aktuell sicherstellst, dass es keine zwei Teams mit dem gleichen Namen gibt 🙂

Aber das Grundproblem existiert trotzdem: Für Competition gibt es zwei verschiedene Arten von "Gleichheit", das mag aktuell keine Probleme verursachen, aber das Potentiell für große Probleme ist vorhanden – bei gleichzeitig minimalem Aufwand für eine Änderung.

 

[jCoder1984]

Okay auch dir vielen lieben Dank für dein Antwort.

Aber wie würde man es denn besser machen