Refactoring metody

Cześć,
Piszę sobie projekt i jedna z metod bardzo mi się nie podoba, może wy wymyślicie jak ją przerobić na czytelną. Metoda jest w klasie Files która obsługuje pliki:

public function changeFilename(){
		if($this->input->post('filename') != NULL AND $this->input->post('customer_filename') != NULL){
			if($this->input->post('customer_filename') != ''){
				if($this->files_model->changeFilename($this->input->post('customer_filename'), $this->input->post('filename'))){
					responseAjax(AJAX_SUCCESS, $this->lang->line('files_filename_changed'));
				} else {
					responseAjax(AJAX_ERROR, $this->lang->line('error_messages_unknown_error'));
				}
			} else {
				responseAjax(AJAX_ERROR, $this->lang->line('files_filename_cant_be_empty'))
			}
		} else {
			responseAjax(AJAX_ERROR, $this->lang->line('error_messages_unknown_error'));
		}
	}

Na samym początku przerób to tak, aby rzucało wyjątek zamiast paćkało się z jakimś responseAjax.
Jeśli responseAjax jest funkcją globalną, to nawet się nie przyznawaj, tylko dopisz ją także do listy rzeczy, które trzeba zrefaktoryzować.

Plus dorzuć jakiś kontekst - to co pokazujesz jest częścią kontrolera (co by uzasadniało odnoszenie się do treści żądania, choć kontroler nie powinien zawierać w sobie bezpośrednio logiki) czy serwisu (co by oznaczało, że nie powinien się on odnosić do żądania)?

Z rzucaniem wyjątków się z Tobą nie zgodzę. Funkcja responseAjax jest helperem służącym do zwracania komunikatów. Frontend przerabia sobie to na komunikat do wyświetlenia w przeglądarce. Tz. (w wielkim skrócie) pierwszy parametr oznacza kolor boxa z monitem a drugi jego treść. Chociaż jak teraz myślę responseAjax mógłbym dać jako widok.
A co do logiki w kontrolerze to przepraszam gdzie ona miała by zachodzić? W widoku? W modelu?

Odnośnie responseAjax - w najbardziej prymitywnym przypadku może to wyglądać tak:

<?php

class MyController {

	private $myService;
	
	public function __construct($myService) {
		$this->myService = $myService;
	}

	public function changeFilenameAction() {
		try {
			$fileName = $this->input->post('filename');
			$customerFilename = $this->input->post('customer_filename');
			
			$this->myService->changeFilename($filename, $customerFilename);
			
			responseAjax(AJAX_SUCCESS, $this->lang->line('files_filename_changed'));
		} catch (MyServiceException $ex) {
			responseAjax(AJAX_ERROR, $this->lang->line($ex->getMessage()));
		}
	}

}

// ...

class MyService {
	
	public function changeFilename($filename, $customerFilename) {
		if (empty($filename) && empty($customerFilename)) {
			throw new MyServiceException('error_messages_unknown_error');
		}
		
		if (empty($customerFilename)) {
			throw new MyServiceException('files_filename_cant_be_empty');
		}
		
		// i tak dalej
	}
	
}

Oczywiście tutaj w dalszym ciągu mamy pole do popisu:

1. 'error_messages_unknown_error' wraz z resztą powinny być enumami.
2. Powinien istnieć globalny handler wyjątków zamiast try..catch w changeFilenameAction.
3. Przybiłbym do krzyża osobę, która stwierdziła, że responseAjax jako globalna funkcja jest dobrym pomysłem.
4. Niektórzy (TM) optowaliby dodatkowo za utworzeniem interfejsu MyServiceInterface, lecz nie jestem pewien czy w Twoim przypadku to nie byłaby już czysto masturbacja kodem (musiałbym poznać całość architektury, a to z kolei już nie jest tematem tego wątku :P).

A co do logiki w kontrolerze to przepraszam gdzie ona miała by zachodzić? W widoku? W modelu?
W warstwie serwisowej.
Jeśli wykorzystujesz z jakiegoś powodu czyste MVC (brrr), powinien być to model.

Według mnie (tak zostałem nauczony) kontroler ma sterować co się dzieje podczas uruchamiania aplikacji, a model "pracą" na danych.
Framework na którym pracuję nawet nie ma metod do pobrania innych kontrolerów. Jak i excepton (jak nazwa wskazuje wyjątek, coś czego się nie spodziewaliśmy) nie jest do zwracania danych przy sprawdzaniu czy coś istnieje. Kolega chyba kiedyś pisał w c++ (lub c) bo tam by to miało sens. W PHP nie da rady zaimplementować MVC w czystej postaci.

Framework na którym pracuję nawet nie ma metod do pobrania innych kontrolerów
Nie rozumiem, w jaki sposób to zdanie łączy się z kontekstem, tudzież: do czego się odnosi. Kontrolery pod żadnym wyjątkiem nie powinny być od siebie zależne.

Jak i excepton (jak nazwa wskazuje wyjątek, coś czego się nie spodziewaliśmy) nie jest do zwracania danych przy sprawdzaniu czy coś istnieje.
Fakt, znacznie prościej czytać i zarządzać tą drabinką ifów ;-)
Zadam pytanie z innej beczki: gdzie w takim razie byś chciał wykorzystać wyjątki?

Kolega chyba kiedyś pisał w c++ (lub c) bo tam by to miało sens.
Drabinki ifów właśnie tworzyło się wieki temu w C, gdy nie było żadnego mechanizmu, który by umożliwił metodzie stwierdzenie dobra, screw it, nie dam sobie z tym rady.

Patryk27 napisał(a):

Nie rozumiem, w jaki sposób to zdanie łączy się z kontekstem, tudzież: do czego się odnosi. Kontrolery pod żadnym wyjątkiem nie powinny być od siebie zależne.

Zrozumiałem że ma to być jako nowy kontroller.

Patryk27 napisał(a):

Fakt, znacznie prościej czytać i zarządzać tą drabinką ifów ;-)
Zadam pytanie z innej beczki: gdzie w takim razie byś chciał wykorzystać wyjątki?

Przy np. pobieraniu danych z bazy gdy spodziewamy się jakiś konkretnych wyników (rekordy lub ich brak) ale np. nie spodziewamy się że serwer bazodanowy nie odpowiada.

A i nie zrozum mnie źle. To nie jest tak że zapytałem się o coś a potem mówię że odpowiedź jest zła. Po paru latach pracy w paru firmach nauczyłem się rzeczy które "nie zgadzają mi się" z tym co piszesz. Dlatego wolałbym na ten temat podyskutować.

Zrozumiałem że ma to być jako nowy kontroller.
Nie, nie. MyService w moim przykładzie należy do warstwy serwisowej (czyli powiedzmy, że u Ciebie jest modelem) - wrzuciłem akurat jedno pod drugim, ponieważ było prościej.

Przy np. pobieraniu danych z bazy gdy spodziewamy się jakiś konkretnych wyników (rekordy lub ich brak) ale np. nie spodziewamy się że serwer bazodanowy nie odpowiada.
Zatem brak odpowiedzi od serwera bazy danych = wyjątek, lecz już wywołanie akcji kontrolera bez oczekiwanych przez nią argumentów (u Ciebie filename i customer_filename) skutkuje tylko komunikatem błędu? Czym te dwa przypadki się od siebie różnią i gdzie jest granica?

To nie jest tak że zapytałem się o coś a potem mówię że odpowiedź jest zła.
Przecież się nie focham, od dyskusji jest właśnie forum dyskusyjne ;-)

Patryk27 napisał(a):

Zatem brak odpowiedzi od serwera bazy danych = wyjątek, lecz już wywołanie akcji kontrolera bez oczekiwanych przez nią argumentów (u Ciebie filename i customer_filename) skutkuje tylko komunikatem błędu? Czym te dwa przypadki się od siebie różnią i gdzie jest granica?

Zawsze uważałem wyjątki za mechanizm do kontroli serwera a nie usera, którego nie powinno się używać zamiast ifa sprawdzającego czy dane są wpisan. Tz. user nie ma wpływu na działanie (lub niedziałanie) silnika bazodanowego ale ma wpływ na zawartość formularzy. Ale rozumiem jak miałoby to działać i muszę Ci przyznać rację.