Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quiz#3 #4

Open
wants to merge 1 commit into
base: example3
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions the-badass-todo-list-part-1/QUIZ3.md
@@ -0,0 +1,4 @@
# Quiz#3

Consider what will happen when we try to test these global object.
Other defects are still avaiable.
6 changes: 6 additions & 0 deletions the-badass-todo-list-part-1/input.js
Expand Up @@ -5,6 +5,7 @@ var TodoInputManager = {
_input: null, // Will be an element when starting up.
drawInput(motd) {
var input = TodoInputManager._input;
// Quiz#3
var tidyUp = function() {
input.value = '';
input.disabled = false;
Expand All @@ -24,6 +25,7 @@ var TodoInputManager = {
if (this.readyState == 4 && this.status == 200) {
var motd = this.response;
// The flow ends here.
// Quiz#3
afterFetch(motd);
} else {
// We omit the error in this novice's example.
Expand All @@ -32,11 +34,14 @@ var TodoInputManager = {
xhr.send();
},

// Quiz#3
saveData(todoItem, afterSave) {
console.log('saved');
// Quiz#3
afterSave();
},

// Quiz#3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

要測試這個東西還要傳一個 event 進來,感覺其實應該可以不用

onUserCreateNewTodo(event, afterCreation) {
var input = TodoInputManager._input;
if (0x0D === event.keyCode) {
Expand All @@ -62,6 +67,7 @@ var TodoInputManager = {
}
};

// Quiz#3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. init
  2. fetchData
  3. drawInput
  4. onUserCreateNewTodo
  5. saveData
  6. tidyUp

看懂這執行順序天殺的花了我好多時間
你還是砍掉重練吧
不然至少加點註解啊QAQ

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know there are thousands code like this torture people everyday. So it's a "real" case, and that's what I want you to catch

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

達仁好兇XD

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

對不起 <(_ _)>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine. Because when I saw some code in real codebase like this I will also spit some f* words. And it's always good to see you can pick such issues.

exports.TodoInputManager = TodoInputManager;
})(window);

1 change: 1 addition & 0 deletions the-badass-todo-list-part-1/main.js
@@ -1,6 +1,7 @@
'use strict';

// Kick off
// Quiz#3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

未來要init東西若多,按這寫法不就....,應該用個list放入要init的物件並用迴圈去初始化。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. We have some similar methods in Gaia. For example:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/lockscreen/js/lockscreen_state_manager.js#L481

Although to deal with real bootstrapping flow, you need some more delicate and complete solutions.

document.addEventListener('DOMContentLoaded', function(event) {
TodoListManager.init();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

比起 manager, component 的命名會比較好吧?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes...that's one issue I always wonder why Gaia people named things like this so thoughtlessly. However, I need to remind you that component is actually another pattern in the book by GoF, so it still may cause some confusions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Component somehow feel like a static object and work passively. While involving control logic, manager seems more suitable in this case.

TodoInputManager.init();
Expand Down
3 changes: 3 additions & 0 deletions the-badass-todo-list-part-1/test/test-list.js
Expand Up @@ -5,6 +5,7 @@ describe('Test TodoListManager', function() {
assert.isNull(TodoListManager._wrapper.querySelector('li'));
});

// Quiz#3
it('Can save some data', function() {
var dummyList = [
{checked: false, description: 'Dummy Todo #1'},
Expand All @@ -15,9 +16,11 @@ describe('Test TodoListManager', function() {
});
});

// Quiz#3
it('Will change one "checked" when the event is coming (BAD PATTERN)',
function() {
// Check if it is false before the test
// Quiz#3
assert.isFalse(TodoListManager._listTodoItem[0].checked);
var dummyCheckElement = document.createElement('div');
dummyCheckElement.dataset.todoId = '0';
Expand Down