Refactoring - dobrze kombinuję czy przegiąłem?

Na prostym przykładzie:

jest meetoda:

// ************* I *******************

@Override
public List<T> scrape(String text) {
	text = text.toLowerCasse();
	text = text.replaceAll("(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]", "");
	text = text.replaceAll("<a href=.*</a>", "");
	text = text.replace("cwiartka", "0.25");
		
	
	return this.getHander.handle(text).stream()
			.flatMap(x ->  finder.find("jakistampattern").stream())
			.map(this.converter.apply)
			.filter(this.isValid)
			.collect(Collectors.toList());
}

wujek Bob: a single responsibility?.
Julian: ok, już robię...

//************* II *******************

@Override
public List<T> scrapeFrom(String text) {
	
	text = this.clean(text);
	
	return this.getHander.handle(text).stream()
			.flatMap(x ->  finder.find("jakistampattern").stream())
			.map(this.converter.apply)
			.filter(this.isValid)
			.collect(Collectors.toList());
}

private String clean(String text) {
	return text.toLowerCase()
	    .replaceAll("(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]", "");
	    .replaceAll("<a href=.*</a>", "");
	    .replace("cwiartka", "0.25");
}

wujek Bob: single responsibility, synek
Julian: ok, ok...

//************* III *******************


public interface Cleaner extends UnaryOperator<String> {
	
}

public class CleanerDefault implements Cleaner {
	
	@Override
	public String apply(String text) {
		return text.toLowerCasse()
		    .replaceAll("(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]", "");
		    .replaceAll("<a href=.*</a>", "");
		    .replace("cwiartka", "0.25");
	}
}

@Override
public List<T> scrapeFrom(String text) {
	
	text = this.cleaner.apply(text);
	
	return this.getHander.handle(text).stream()
			.flatMap(x ->  finder.find("jakistampattern").stream())
			.map(this.converter.apply)
			.filter(this.isValid)
			.collect(Collectors.toList());
}

wujek Bob: single responsibility, powiedziałem!

//************* IV *******************


public interface Cleaner extends UnaryOperator<String> {
	
}

public class CleanerDefault implements Cleaner {
	
	@Override
	public String apply(String text) {
		text = text.toLowerCasse();
		text = this.cleanFromHttps(text);
		text = this.cleanFromHrefs(text);
		text = this.cleanFromCwiartka(text);
		return text;
	}
	
	private String cleanFromHttps(String text) {
		return text.replaceAll("(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]", "");
	}
	
	private String cleanFromHrefs(String text) {
		return text.replaceAll("<a href=.*</a>", "");
	}
	
	private String cleanFromCwiartka(String text) {
		return text.replace("cwiartka", "0.25");
	}
}

@Override
public List<T> scrapeFrom(String text) {
	
	text = this.cleaner.apply(text); // wstrzykniéty CleanerDefault
	
	return this.getHander.handle(text).stream()
			.flatMap(x ->  finder.find("jakistampattern").stream())
			.map(this.converter.apply)
			.filter(this.isValid)
			.collect(Collectors.toList());
}

wujek Bob: `nosz kurtka maćka, z Krakowa jesteś? SINGLE RESPONSIBILITY!

//************* V *******************


public interface Cleaner extends UnaryOperator<String> {
	
}

public class CleanerDefault implements Cleaner {
	
	private String text;
	
	@Override
	public String apply(String text) {
		this.text = text;
		return this.text.toLowerCase().cleanFromHttps().cleanFromHrefs().cleanFromCwiartka();
	}
	
	private CleanerDefault cleanFromHttps() {
		this.text = this.text.replaceAll("(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]", "");
		return this;
	}
	
	private CleanerDefault cleanFromHrefs() {
		this.text = this.text.replaceAll("<a href=.*</a>", "");
		return this;
	}
	
	private CleanerDefault cleanFromCwiartka() {
		this.text = this.text.replace("cwiartka", "0.25");
		return this;
	}
}

@Override
public List<T> scrapeFrom(String text) {
	
	text = this.cleaner.apply(text); // wstrzykniéty CleanerDefault
	
	return this.getHander.handle(text).stream()
			.flatMap(x ->  finder.find("jakistampattern").stream())
			.map(this.converter.apply)
			.filter(this.isValid)
			.collect(Collectors.toList());
}

wujek Bob: nosz, kurrrrtka, S I N G L E R E S P O N S I B I L I T Y K U R T K A J E G O MAĆka!!!

//************* VI *******************


public interface Cleaner {
	Cleaner clean();
	String getText();
}

public class CleanerDefault implements Cleaner {
	
	private String text;
	
	public CleanerDefault(String text) {
		this.text = text;
	}
	
	@Override
	public String getText() {
		return text;
	}
	
	@Override
	public Cleaner clean() {
		thix.text = this.text.toLowerCase().cleanFromHttps().cleanFromHrefs().cleanFromCwiartka();
		return this;
	}
	
	private Cleaner cleanFromHttps() {
		this.text = this.text.replaceAll("(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]", "");
		return this;
	}
	
	private Cleaner cleanFromHrefs() {
		this.text = this.text.replaceAll("<a href=.*</a>", "");
		return this;
	}
	
	private Cleaner cleanFromCwiartka() {
		this.text = this.text.replace("cwiartka", "0.25");
		return this;
	}
}

@Override
public List<T> scrapeFrom(String text) {
	
	cleaner = this.cleanerclass.getConstructor(String.class).newInstance(text);
	text = cleaner.clean().getText();
	
	return this.getHander.handle(text).stream()
			.flatMap(x ->  finder.find("jakistampattern").stream())
			.map(this.converter.apply)
			.filter(this.isValid)
			.collect(Collectors.toList());
}

...

Czytałeś ostatnią książkę Wuja Boba w którym tłumaczy co oznacza single responsibility? Chodzi o to żeby nie mieszać kodu który zależy od innych działów/ludzi w firmie, jak osoba A każe Ci napisać wyświetlanie tekstu w sposób X, i osoba B też chce go wyświetlać w sposób X, a następnie po jakimś czasie osoba A zmienia zdanie i chce go wyświetlać w sposób Y to zmieniłeś też wyświetlanie tekstu dla osoby B chociaż tego nie chciała.

Też kiedyś rozumiałem to w ten sposób opisany wyżej przez Ciebie, potem wujek Bob się skrażył, że nikt go nie zrozumiał i się z tego tłumaczył.

@Julian_ dorzuciłem Ci fragmenty z książki https://helion.pl/ksiazki/czysta-architektura-struktura-i-design-oprogramowania-przewodnik-dla-profesjonalistow-robert-c-martin,czarch.htm

Powiem krótko: wzorce projektowe żyją dla Nas, a nie my dla nich. Wrzucanie ich wszędzie gdzie się da jest idiotyzmem, podobnie jak pisanie kodu "na zaś, bo się może przydać".

cleaner = this.cleanerclass.getConstructor(String.class).newInstance(text); cudo!

Powiem tak: dla mnie wszystkie wersje nie do końca wprowadzają poprawki. Fakt są inne ale nie wiem czy lepsze. Według mnie ponad SRP powinno być szeroko rozumiana domena. Czy Twój kod po zmianach uwypukla domenę? Nie. Weźmy pod analizę wersję V.

public interface Cleaner extends UnaryOperator<String> {
}

public class CleanerDefault implements Cleaner {

Nie widzę sensu dodawania takiego interfejsu skoro jest tylko jedna implementacja. Dodatkowo nazwa CleanerDefault nic mi nie mówi o zachowaniu tej klasy. Jaki jest jej API / interfejs? Ma tylko jedną metodę publiczną apply czyli coś ala ~doMagic(). text = this.cleaner.apply(text); na cleaner wywołujemy apply - ale co to robi to wiemy dopiero gdy spojrzymy do implementacji. To nie powinno tak być, już na poziomie interfejsów powinnyśmy wiedzieć co się dzieje. Dodatkowo w klasie CleanerDefault jest kilka metod prywatnych. W jaki sposób chciałbyś tę klasę rozszerzyć? Gdzie tutaj jest Open / close principle? Żeby dodać nowe funkcjonalności musisz zmienić kod klasy - defacto zmieniasz 100% zainteresowanych klas. Tutaj nie ma za gram SRP.

Można byłoby zrobić dla każdej ~transformacji osobną implemetancję coś ala:

public class HttpsCleaner extends UnaryOperator<String>{
@Override
    public String apply(String text) {
        return text.replaceAll("(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]", "");
    }
}

Pytanie tylko w ilu miejscach potrzebujesz takiego rozwiązania. Jeśli w jednym pierwsze rozwiązanie jest do przyjęcia.

lubie_programowac napisał(a):

W jaki sposób chciałbyś tę klasę rozszerzyć? Gdzie tutaj jest Open / close principle? Żeby dodać nowe funkcjonalności musisz zmienić kod klasy - defacto zmieniasz 100% zainteresowanych klas. Tutaj nie ma za gram SRP.

jak potrzebowałem rozszerzyć dla zmiennej Experience to zrobiłem tak o:

public class TextCleanerForExperience implements TextCleaner {

	@Override
	public String apply(String text) {
		return new TextCleanerDefault().apply(text).replaceAll("pół", "0.5");
	}
}

Można byłoby zrobić dla każdej ~transformacji osobną implemetancję coś ala:

public class HttpsCleaner extends UnaryOperator<String>{
@Override
    public String apply(String text) {
        return text.replaceAll("(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]", "");
    }
}

Pytanie tylko w ilu miejscach potrzebujesz takiego rozwiązania. Jeśli w jednym pierwsze rozwiązanie jest do przyjęcia.

potrzebuję w około 10 miejscach.
Główny cel całego problemu: dla każdego spośród 2000 tekstów zescrapować po 10 zmiennych: m.in. age, experience, postnumbers, location, price i zunifikować np. przeliczając waluty. Fragmenty, które zawierają wartości zmiennych są pisane na dziesiątki sposobów. Chwytam zmienne po różnych słowach-kluczach. Po słowach kluczowych rozpoznaję w jakiej są jednostce i odpowiednio przeliczam. Rozwiązałem to wszystko już półtorej roku temu componentami (jeden w drugim, a drugi w trzecim itd. 50 klas po 10 linijek), nie dbałem też o nazwy zmiennych czy nazwy metod i teraz to jest to nieczytelne, więc chcę to napisać lepiej.

Ok, chcesz się rozwijać. Z mojej strony polecam kilka razy w tygodniu wchłonąć jedną z prezentacji Panów:
a) Jakuba Kubryńskiego (np: www.youtube.com/watch?v=yM_CMWutuzI )
b) Jakuba Nabrdalika (np. www.youtube.com/watch?v=ILBX9fa9aJo )
c) Sławomira Sobótki (np. www.youtube.com/watch?v=iaLeKHbspLg )

Opisany przez Ciebie problem / zagadnienie nie jest do rozwiązania od hoc - bez dokładnej wiedzy co trzeba zrobić możemy tylko spekulować nad wartością danej poprawki. Mówię o sytuacji gdy np tak jak w Twoim przykładzie z pierwszego postu poprawimy tylko jedną klasę a widzimy później w TextCleanerForExperience że jest inna zasada biznesowa replaceAll("pół", "0.5") która jest niemal identyczna jak w replace("cwiartka", "0.25"). Manipulacja na DOMie mogłyby być w innym miejscu - byłoby to np pierwsza faza konwersji plików bądź normalizacji danych.