#12 ревью

composer.json

@@ -24,9 +24,6 @@
 	},
 	"require-dev": {
 		"barryvdh/laravel-ide-helper": "dev-master",
-		"phpunit/php-code-coverage": "~2.0@dev",
-		"phpunit/phpunit-mock-objects": "2.3.*@dev",
-		"phpunit/phpunit": "3.7",
 		"zizaco/factory-muff": "dev-master",
 		"mockery/mockery": "0.9.*",
 		"orchestra/testbench": "2.2.*"

workbench/fintech-fab/actions-calc/public/js/manage.js

@@ -70,6 +70,11 @@ $(document).ready(function () {
 		return false;
 	});
 
+	// не могу прям придраться сильно, но я бы по другому делал.
+	// событие лучше вешать на submit формы, а не на кнопку
+	// т.к. submit может произойти и по кнопке, и без кнопки
+	// зачем body непонятно, т.к.: $('#button-event-create').on('click', function(){ .... });
+
 	// events:
 	// modal event create
 	$body.on('click', '#button-event-create', function (e) {
@@ -79,16 +84,23 @@ $(document).ready(function () {
 		var $button = $(this);
 		var $form = $button.closest('form');
 
-		buttonSleep($button);
+		buttonSleep($button); // глобальные функции в js также не хорошо, как и глобалы в других языках
 
 		$.post('/actions-calc/event/create',
 			$form.serialize(),
 			function (oData) {
+				// здесь желательно код тоже оптимизировать, как минимум так:
+				// if(revealFormErrors(oData)){
+				//     return false;
+				// }
+				// // код без ошибок
+				// еще круче ошибку с сервера возвращать другим кодом HTTP (не 200 ОК)
+				// и тогда ошибки здесь можно ловить одним обработчиком единообразно на все формы
 				if (oData.status == 'success') {
-					toastr.success(oData.message);
+					toastr.success(oData.message); // это лучше делать глобальным обработчиком, чтобы не писать в каждом вызове
 					$('#modal-event-create').foundation('reveal', 'close');
 					updateEventsTable();
-					clearFormErrors($form);
+					clearFormErrors($form); // это лучше перед сабмитом выше
 				} else if (oData.status == 'error') {
 					revealFormErrors($form, oData.errors);
 				}
@@ -518,7 +530,7 @@ $(document).ready(function () {
 			'json'
 		).always(function () {
 				buttonWakeUp($th);
-			}).fail(function (xhr) {
+			}).fail(function (xhr) { // в глобальный обработчик!
 				alert(xhr.responseText);
 			});
 
@@ -648,6 +660,7 @@ $(document).ready(function () {
 		buttonSleep($button);
 
 		$.ajax({
+			// DELETE - гуууд
 			method: 'DELETE',
 			url: '/signal/' + iSignalId,
 			success: function (oData) {
@@ -677,7 +690,14 @@ $(document).ready(function () {
 
 		buttonSleep($button);
 
+		// вообще правильно делать, если все формы строятся примерно одинаково,
+		// свое небольшое расширение jquery для форм, специально чтобы и кнопки в нем включались-выключались,
+		// и контент форм обновлялся, и ошибки показывались и так далее.
+		// это спасло бы от копипаста и глобальных функций типа buttonSleep
+		// ну а в этом коде "плохо" - только копипаст.
+
 		$.ajax({
+			// PUT - гуууд
 			method: 'PUT',
 			url: '/signal/' + iSignalId,
 			data: $form.serialize(),
@@ -805,6 +825,24 @@ $(document).ready(function () {
 
 });
 
+// скажу так:
+// мне все это не нравится, т.к. код совсем непонятен.
+// и в случае проблем придется сильно напрягать мозг.
+// вижу кашу из селекторов и onClick-ов и цепочек parent().prev() и прочих closest()
+// все это весьма хрупкая система - верстальщику не отдать, все сломает сразу.
+//
+// но при этом, задача конечная и подобное решение не так плохо в этом разрезе
+// и т.к. оно работает, и соответствует условию задачи, вряд ли придется это сильно поддерживать
+//
+// и еще:
+// такие задачи чисто на jquery красиво сделать можно только через выделенную библиотеку
+// решающую вполне конкретную задачу, не связанную с контекстом этой задачи (как любое расширение jquery).
+//
+// итог: у меня нет рекомендаций "что доработать".
+// и это решается либо так, как есть, либо надо фреймворк посерьезнее jquery
+//
+// но в деталях - код можно подчистить точно также, как и в любом другом ЯП
+
 // Rules factory class
 // needs template
 // container :: rules container :: input\selects

workbench/fintech-fab/actions-calc/src/FintechFab/ActionsCalc/Components/AuthHandler.php

@@ -2,8 +2,8 @@
 
 namespace FintechFab\ActionsCalc\Components;
 
-use FintechFab\ActionsCalc\Models\Terminal;
 use Config;
+use FintechFab\ActionsCalc\Models\Terminal;
 
 /**
  * Class AuthHandler
@@ -23,6 +23,7 @@ class AuthHandler
 	public static function checkSign($aRequestData)
 	{
 		/** *@var Terminal $terminal */
+		// выбирать только нужные данные - хорошая практика
 		$terminal = Terminal::find($aRequestData['terminal_id'], ['id', 'key']);
 
 		if (is_null($terminal)) {
@@ -41,11 +42,8 @@ public static function checkSign($aRequestData)
 	 */
 	public static function isClientRegistered()
 	{
+		// проще можно. и еще terminal_id не похоже на $iClientId
 		$iClientId = Config::get('ff-actions-calc::terminal_id');
-		$iClientId = (int)$iClientId;
-
-		$terminal = Terminal::find($iClientId, ['id']);
-
-		return !(is_null($terminal));
+		return Terminal::find($iClientId, ['id']);
 	}
 }

workbench/fintech-fab/actions-calc/src/FintechFab/ActionsCalc/Components/CalcHandler.php

@@ -2,10 +2,10 @@
 
 namespace FintechFab\ActionsCalc\Components;
 
-use Log;
 use App;
 use FintechFab\ActionsCalc\Models\Event;
 use FintechFab\ActionsCalc\Models\Rule;
+use Log;
 
 /**
  * Class CalcHandler
@@ -120,6 +120,9 @@ private function checkRule($oRuleTerm)
 	 * @param $dataValue
 	 *
 	 * @return bool
+	 *
+	 * нравится: операции разбиты по методам
+	 *
 	 */
 	private function bool($ruleValue, $dataValue)
 	{

workbench/fintech-fab/actions-calc/src/FintechFab/ActionsCalc/Components/Registrator.php

@@ -31,6 +31,7 @@ public static function registerEvent($aRequestData)
 		$aEventAttributes['event_id'] = $aEventAttributes['id'];
 		$aEventAttributes['data'] = $aRequestData['data'];
 
+		// как то некрасиво. но предложений нет
 		unset($aEventAttributes['id']);
 		unset($aEventAttributes['created_at']);
 		unset($aEventAttributes['updated_at']);
@@ -75,7 +76,7 @@ public static function registerSignal($aSignalAttributes, $setFlagUrl = false, $
 		$oRegisterSignal->setRawAttributes($aSignalAttributes);
 
 		if ($oRegisterSignal->save()) {
-			$sWhichFlag = $setFlagUrl ? 'CURL' : 'Result to Queue to Queue XD';
+			$sWhichFlag = $setFlagUrl ? 'CURL' : 'Result to Queue to Queue XD'; // да уж, оборжаться. ))
 			Log::info("Register signal($sWhichFlag)", $aSignalAttributes);
 		}
 	}

workbench/fintech-fab/actions-calc/src/controllers/AuthController.php

@@ -9,9 +9,9 @@
 use Hash;
 use Input;
 use Redirect;
+use Request;
 use Session;
 use Validator;
-use Request;
 use View;
 
 /**
@@ -51,13 +51,15 @@ public function registration()
 		$validator = Validator::make($aRequestData, Validators::getTerminalValidators());
 
 		if ($validator->fails()) {
+			// копипаст (выше уже есть)
 			$iTerminalId = Config::get('ff-actions-calc::terminal_id');
-			$aRequestData['id'] = $iTerminalId;
-
+			$aRequestData['id'] = $iTerminalId; // зачем?
 			return Redirect::to(route('auth.registration'))->withInput($aRequestData)->withErrors($validator);
 		}
 
 		// data valid
+		// это не задача контроллера, т.к. относится чисто к созданию терминала
+		// хорошо убирать либо в модель терминала, либо в компонент (если он есть)
 		$aRequestData['password'] = Hash::make($aRequestData['password']);
 		$aRequestData['key'] = (strlen($aRequestData['key']) < 1) ?
 			sha1($aRequestData['name'] . microtime(true) . rand(10000, 90000)) : $aRequestData['key'];
@@ -80,6 +82,7 @@ public function profile() // TODO: Too fat. To several methods.
 		$oTerminal = Terminal::find($iTerminalId);
 
 		// on GET only opening, and fill in
+		// ай-яй-яй. GET/POST это делать роутингом! и больше никак.
 		if (Request::isMethod('GET')) {
 			return View::make('ff-actions-calc::auth.profile', ['terminal' => $oTerminal]);
 		}
@@ -88,25 +91,31 @@ public function profile() // TODO: Too fat. To several methods.
 
 		// validation fails
 		if ($validator->fails()) {
+			// было бы лучше
+			// $this->error('auth.profile', $aRequestData, $validator)
+			// т.к. оч. часто повтоярется
 			$oErrors = $validator->errors();
 
 			return Redirect::to(route('auth.profile'))->withInput($aRequestData)->withErrors($oErrors);
 		}
 
+		// а смену пароля вообще лучше всегда делать отдельным разделом "изменить пароль"
+		// а не смешивать вместе с редактированием профиля
 		// password change
 		if (isset($aRequestData['change_password']) && $aRequestData['change_password'] == 1) {
 
 			$validator = Validator::make($aRequestData, Validators::getProfileChangePassValidators());
 
 			if ($validator->fails()) {
-
+				// мелкий, но копипаст
 				$oErrors = $validator->errors();
 
 				return Redirect::to(route('auth.profile'))->withInput($aRequestData)->withErrors($oErrors);
 			}
 
 			// current password check
 			if (Hash::check($aRequestData['current_password'], $oTerminal->password) === false) {
+				// мелкий, но копипаст
 				$oErrors = $validator->errors();
 				$oErrors->add('current_password', 'Текущий пароль и введённый, не совпадают.');
 
@@ -128,6 +137,7 @@ public function profile() // TODO: Too fat. To several methods.
 			return Redirect::to(route('auth.profile'))->withInput($aRequestData);
 		}
 
+		// чуть выше такая же строка
 		return Redirect::to(route('auth.profile'))->withInput($aRequestData);
 	}
 

workbench/fintech-fab/actions-calc/src/controllers/EventController.php

@@ -4,11 +4,11 @@
 
 use FintechFab\ActionsCalc\Components\Validators;
 use FintechFab\ActionsCalc\Models\Event;
+use Input;
 use Paginator;
+use Request;
 use Validator;
-use Input;
 use View;
-use Request;
 
 /**
  * Class EventController
@@ -33,9 +33,14 @@ public function create()
 		$oRequestData = Input::all();
 		$oRequestData['terminal_id'] = $this->iTerminalId;
 
+		// стиль названий переменный - какой то один лучше выбрать... сразу на двух - не гуд
+		// сейчас мы сами уходим от венгерки ($oRequestData) в сторону psr ($requestData)
 		$validator = Validator::make($oRequestData, Validators::getEventRules());
 
 		if ($validator->fails()) {
+			// ай-яй, ну копипаст же, весь контроллер в этих практически одинаковых вызовах return json_encode
+			// делаешь просто return $this->error($validator) и return $this->success($message)
+			// и красота!
 			return json_encode(['status' => 'error', 'errors' => $validator->errors()]);
 		}
 
@@ -58,6 +63,8 @@ public function update($id)
 		$event = Event::find($id);
 
 		// only view on GET
+		// роутингом, роутингом! в методе update REST-контроллера - разрешать GET и генерить форму... ну гадость же :-)
+		// а сам метод-то, метод... и json может вернуть, и html... как ему не стыдно?
 		if (Request::isMethod('GET')) {
 			return View::make('ff-actions-calc::event.update', ['event' => $event]);
 		}
@@ -67,6 +74,9 @@ public function update($id)
 
 		$aValidators = Validators::getEventRules();
 		// ignoring uniquiness of event_sid on update
+		// не хорошо так... надо Validators::getEventRules($id)
+		// раз уж есть метод, который генерит правила, вот и пусть он их генерит
+		// чтобы генерил умно, надо дать ему параметров, а не доделывать за него работу в контроллере
 		$aValidators['event_sid'] = $aValidators['event_sid'] . ',' . $id;
 		$validator = Validator::make($oRequestData, $aValidators);
 
@@ -97,10 +107,13 @@ public function delete()
 		$event = Event::find((int)$aRequest['id']);
 
 		if ($event->rules->count() > 0) {
+			// здесь return array....
 			return ['status' => 'error', 'message' => 'Сначала удалите правила.'];
 		}
 
 		if ($event->delete()) {
+			// а тут return json???
+			// возвращаемые данные из одного метода должны быть одинаковые!
 			return json_encode(['status' => 'success', 'message' => 'Событие удалено.']);
 		}
 
@@ -121,6 +134,9 @@ public function updateEventsTable()
 		Paginator::setCurrentPage($iPage);
 
 		$aoEvents = Event::where('terminal_id', '=', $this->iTerminalId)->orderBy('created_at', 'desc')->paginate(10);
+		// Event::where('terminal_id', '=', $this->iTerminalId)
+		// есть лайфхак
+		// Event::whereTerminalId($this->iTerminalId)
 		$aoEvents->setBaseUrl('/actions-calc/events/table');
 
 		return View::make('ff-actions-calc::calculator._events_table', [

workbench/fintech-fab/actions-calc/src/controllers/SignalController.php

@@ -2,13 +2,13 @@
 
 namespace FintechFab\ActionsCalc\Controllers;
 
+use App;
 use FintechFab\ActionsCalc\Components\Validators;
 use FintechFab\ActionsCalc\Models\Rule;
 use FintechFab\ActionsCalc\Models\Signal;
-use Validator;
 use Input;
+use Validator;
 use View;
-use App;
 
 /**
  * Class SignalController
@@ -36,6 +36,8 @@ public function store()
 		$aReturnData = [];
 
 		if (!$oSignal->push()) {
+			// аааааа! в BaseController $this->error($message) или $this->jsonError($message)
+			// каждый раз весь массив... я бы умер от перенагрузки :-)
 			return ['status' => 'error', 'message' => 'Не удалось создать сигнал'];
 		}
 
@@ -58,6 +60,8 @@ public function edit($id)
 	{
 		$signal = Signal::find($id);
 
+		// ну мне бы надоело везде префикс писать...
+		// я бы: return $this->view('signal.edit', compact('signal'));
 		return View::make('ff-actions-calc::signal.edit', compact('signal'));
 	}
 
@@ -113,6 +117,7 @@ public function destroy($id)
 		$oSignal = Signal::find($id);
 
 		$oRules = Rule::where('signal_id', '=', $id)->first();
+		// Rule::whereSignalId($id)->first();
 
 		if (!is_null($oRules)) {
 			return ['status' => 'error', 'message' => 'Сигнал используется.'];