tamazlykar/todo-list

todo-list's Issues

[Рефакторинг и обратное проектирование] Задание 2

Перед рефакторингом необходимо покрыть код тестами. Недочеты, выбранные в Задании 1 невозможно покрыть юнит-тестами, так как или для случая нельзя написать тест или код будет перенесен/удален/изменен и тесты придется писать снова. Поэтому было решено написать e2e тесты.
Тестовый файл
Вспомогательный файл страницы

Результат запуска тестов:

[Рефакторинг и обратное проектирование] Задание 1

1. Ленивый класс и класс данных. Класс не делает ничего. В текущей реализации можно заменить на интерфейс или выделить функции и добавить в класс.
   

   todo-list/src/app/todo-list/todo.model.ts

   Lines 1 to 10 in c0b6b01

   	export class Todo {
   	title: string;
   	isCompleted: boolean;
   	created: any;
   	constructor(title: string) {
   	this.title = title;
   	this.isCompleted = false;
   	}
   	}

2. Каждый элемент имеет текущее состояние (Добавлен, Изменен или Удален). Из-за этого код содержит множество switch условий, в разных классах, что будет затруднять дальнейшее сопровождение. Если потребуется вносить изменения, то придется найти все switch условия и изменить каждый.
   

   todo-list/src/app/todo-list/todo-list.component.ts

   Lines 104 to 116 in c0b6b01

   	private handleData(todoList: TodoMetadata[]) {
   	todoList.forEach(todo => {
   	switch (todo.type) {
   	case ChangeType.added:
   	this.addListData(todo);
   	break;
   	case ChangeType.modified:
   	case ChangeType.removed:
   	this.updateListData(todo);
   	break;
   	}
   	})
   	}

   
   

   todo-list/src/app/todo-list/todo-item/todo-item.component.ts

   Lines 113 to 121 in c0b6b01

   	switch (tempTodo.type) {
   	case ChangeType.modified:
   	this.handleModifiedChangeType(tempTodo, newTitle);
   	break;
   	case ChangeType.removed:
   	this.handleRemovedChangeType(newTitle);
   	break;
   	}

Ангуляр это MVVM фреймворк, основными блоками которого являются "компоненты" и "сервисы".

• Компонент это класс, связанный с View, который может передавать данные во View и реагировать на события View. Он является View Model из MVVM. Класс должен получать данные и отображать их. В идеале без какой либо логики. Компоненты делятся на smart и dumb, которые содержат логику (знают как получить данные, создать что-то и тд) и которые не содержат логику, а только отображают данные, соответственно.
• Сервис это класс с Dependency Injection. Сервисы должны содержать всю бизнес логику и обратку в приложении.

3. Компонент todo-item.component.ts, который отвечает за отображение на экране одного элемента списка, содержит больше количество логики внутри себя. Это затрудняет понимание класса и создает путаницу. Из очевидного:

• Необходимо вынести код, который отвечает за открытие диалоговых окон и их вид.
• Необходим вынести код, который отвечает за обработку конфликтов при параллельном изменении одних и тех-же данных в разных сессиях.

4. Странное решение сгруппировать все типы действий, которые можно совершить над элементов списка и передать их через один "Output" (выход) в родительский класс, вместо того чтобы создать на каждое действие свой выход.
   

   todo-list/src/app/todo-list/todo-item/todo-item.component.ts

   Lines 70 to 84 in c0b6b01

   	private create() {
   	this.emitAction(ActionType.add);
   	}
   	private update() {
   	this.emitAction(ActionType.update);
   	}
   	public remove() {
   	this.emitAction(ActionType.remove);
   	}
   	private emitAction(type: ActionType) {
   	this.action.emit({ type, todo: this.todo });
   	}

   
   В итоге из-за этого имеем лишний switch.
   

   todo-list/src/app/todo-list/todo-list.component.ts

   Lines 51 to 66 in c0b6b01

   	public onItemAction(action: Action) {
   	switch (action.type) {
   	case ActionType.add:
   	this.add(action.todo.data);
   	break;
   	case ActionType.remove:
   	if (action.todo.type !== ChangeType.removed) {
   	this.remove(action.todo);
   	}
   	this.removeListData(action.todo);
   	break;
   	case ActionType.update:
   	this.update(action.todo);
   	break;
   	}
   	}

   
   Если не вдаваться в код, то так же странно, что элемент списка имеет действие "создать". По идее, над элементом можно совершить 2 действия или обновить или удалить. В идеале необходимо избавиться от этого "лишнего" действия.

5. Компонент todo-list.component.ts сам создает новые элементы списка. Если у нас будут другие компоненты, где можно создать новый элемент, то потом придется менять код создания во всех местах. Создание должно проходить в единственном месте. Необходимо его вынести в сервис.
   

   todo-list/src/app/todo-list/todo-list.component.ts

   Lines 44 to 48 in c0b6b01

   	.then(() => this.todoService.add(new Todo(title)));
   	return;
   	}
   	this.add(new Todo(title));

6. Компонент todo-list.component.ts оперирует двумя интерфейсами для элементов. Где-то используется Todo, а где-то TodoMetadata. Это вводит путаницу.
   

   todo-list/src/app/todo-list/todo-list.component.ts

   Lines 68 to 78 in c0b6b01

   	private add(todo: Todo) {
   	this.todoService.add(todo);
   	}
   	private update(todo: TodoMetadata) {
   	this.todoService.update(todo);
   	}
   	private remove(todo: TodoMetadata) {
   	this.todoService.remove(todo);
   	}

   
   Тем более класс TodoMetadata зависит от Todo. Можно было бы изначально использовать 2 класса. Первый, как модель хранится на сервере, второй, серверная модель + дополнительные данные, типо id и прочее, которые уже используются приложением, без лишней вложенности.
   

   todo-list/src/app/todo-list/todo.model.ts

   Lines 1 to 16 in c0b6b01

   	export class Todo {
   	title: string;
   	isCompleted: boolean;
   	created: any;
   	constructor(title: string) {
   	this.title = title;
   	this.isCompleted = false;
   	}
   	}
   	export interface TodoMetadata {
   	id: string;
   	type: ChangeType;
   	data: Todo;
   	}

7. Временное поле.
   Данное поле заполняется и используется только когда существует конфликт при одновременном изменении одного и того же элемента. Данное поле должно удалиться при выделении логики и конфликтах в самостоятельный класс.
   

   todo-list/src/app/todo-list/todo-item/todo-item.component.ts

   Line 21 in c0b6b01

   private tempTodo: TodoMetadata;

8. Компонент todo-item.component.ts, имеет функциональность изменения заголовка элемента. В текущей реализации мы сами проверяем целостность данных, введенных пользователем. Данный функционал можно реализовать через Reactive Form и добавить валидацию предоставляемую формами, и сделать более гибкую настройку. К тому же при изменении требований по валидации, очень легко и можно добавлять свои собственные функции для валидации ввода.

9. Компонент todo-item.component.ts содержит много логики, которой он содержать не должен. Данный компонент должен лишь отображать список Todo. Компонент в свою очередь знает как получить данные, как обрабатывать эти данные в зависимости от типа с сервера и на основании решения добавляет или удаляет в список, создает новые списки Todo.

10. Отсутствует обработки ошибок.
    Сервис может выбросить ошибку, при неверных данных, а так как используется класс без геттеров и сеттверов, то поле может быть не валидным или быть удалено.
    

    todo-list/src/app/todo-list/todo-list.service.ts

    Lines 18 to 20 in c0b6b01

    	if (!id) {
    	throw Error('Todo list ID should be provided!');
    	}

    
    

    todo-list/src/app/todo-list/todo-list.service.ts

    Lines 35 to 37 in c0b6b01

    	if (!this.currentList) {
    	throw Error('Сurrent list reference is not installed');
    	}

    
    Но, в коде который вызывает эти функции отсутствует перехватчик ошибок и код может поломаться.

[Рефакторинг и обратное проектирование] Задание 3

До рефакторинга система имела следующий вид, изображенный на схеме:

Данной системе был произведен рефакторинг с учетом задания #1 и еще некоторых размышлений автора.
В итоге была реализованная следующая система, изображенная на схеме:

Что было сделано:

• Вся логика была вынесена из компонентов. Компонент TodoListPageComponent знает как получать список задач и передает его компонентам для отображения. Остальные компоненты только отображают данные и могут хранить некоторое внутренне состояние компонента.
• Компоненты используют один интерфейс модели Todo.
• Выделен отдельный сервис для работы с диалоговыми окнами.
• Добавлен Store это библиотека ngrx, реализующая паттерн Flux. Таким образом имеем единый источник хранения данных в системе и все действия проходят через него.
• Выделены FirebaseTodoBackendService и TodoService. FirebaseTodoBackendService отвечает за работу с Backend. TodoService предоставляет интерфейс для работы приложению и содержит логику обработки конфликтов.

После рефакторинга была проверена работоспособность системы используя тесты написанные в рамках задания #2

Код с изменениям можно найти в PR #4