Skip to content

Homework 102502041 #33

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.

Sign up for GitHub

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

Open
YenchungChen wants to merge 4 commits into
base: homework
Choose a base branch
from
Open

Homework 102502041 #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c5994b
bind this in event handlers
Oct 28, 2015
f269a74
deal with object properties w/ constructor & prototype
Oct 28, 2015
a6a104f
deal with asynchronous flows with promise
Oct 28, 2015
9833e29
write my first test case for a pure function
Oct 28, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 34 additions & 31 deletions homework/content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,40 @@
'use strict';

(function() {
var _wrapper = document.querySelector('#note-content-wrapper');

function start() {
window.addEventListener('note-open', function(event) {
var note = event.detail;
resetWrapper();
drawNote(note);
});
(function(exports) {
var ContentManager = function(){
this._wrapper = document.querySelector('#note-content-wrapper');
}

function resetWrapper() {
_wrapper.innerHTML = '';
}
ContentManager.prototype ={

start(){
window.addEventListener('note-open',(function(event){
var note = event.detail;
this.resetWrapper();
this.drawNote(note);
}).bind(this));
},

function drawNote(note) {
var title = note.title;
var h = document.createElement('h2');
h.textContent = title;
var passages = note.passages;
var buff = document.createDocumentFragment();
passages.forEach(function(passage) {
var p = document.createElement('p');
p.classList.add('note-passage');
p.textContent = passage;
buff.appendChild(p);
});
_wrapper.appendChild(h);
_wrapper.appendChild(buff);
}
resetWrapper(){
this._wrapper.innerHTML = '';
},

drawNote(note){
var title = note.title;
var h = document.createElement('h2');
h.textContent = title;
var passages = note.passages;
var buff = document.createDocumentFragment();
passages.forEach(function(passage){
var p = document.createElement('p');
p.classList.add('note-passage');
p.textContent = passage;
buff.appendChild(p);
});
this._wrapper.appendChild(h);
this._wrapper.appendChild(buff);
}
};

document.addEventListener('DOMContentLoaded', function(event) {
start();
});
})();
exports.ContentManager = ContentManager;
})(window);
1 change: 1 addition & 0 deletions homework/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<title> Homework - Note List </title>
<script defer src="list.js"></script>
<script defer src="content.js"></script>
<script defer src="main.js"></script>
<link rel="stylesheet" type="text/css" href="main.css" />
</head>
<body>
Expand Down
140 changes: 72 additions & 68 deletions homework/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,79 +1,83 @@
'use strict';

(function() {
(function(exports) {

var _listNoteContent = [];
var _wrapper = document.querySelector('#note-list-wrapper');

function start() {
fetchList(function(data) {
updateList(data);
drawList();
preloadFirstNote();
});
window.addEventListener('click', function(event) {
onNoteOpen(event);
});
var ListManager = function(){
this._listNoteContent = [];
this._wrapper = document.querySelector('#note-list-wrapper');
}

function onNoteOpen(event) {
if (event.target.classList.contains('note-title')) {
var id = event.target.dataset.noteId;
var content = _listNoteContent[id];
window.dispatchEvent(new CustomEvent('note-open',
{ detail: content }));
};
}
ListManager.prototype = {
start(){
this.fetchList().then((function(data){
this.updateList(data);
this.drawList();
this.preloadFirstNote();
}).bind(this)).catch((function(){
Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

Serious issue: if you catch it without logging AND re-throwing it, the following chains (if any) will not get interrupted and in the console there will be nothing about the error. It could be a disaster in real programs. So please correct it.

Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

And the reason we need both logging and re-throwing is because you can prevent the error is covered in the following chains by logging it, while to re-throw it is to make sure the following chain will get interrupted, or some undefined behaviors may cause troubles.


function preloadFirstNote() {
if (_listNoteContent.length !== 0) {
var content = _listNoteContent[0];
window.dispatchEvent(new CustomEvent('note-open',
{ detail: content }));
}
}
}).bind(this));
window.addEventListener('click',(function(event){
this.onNoteOpen(event);
Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

It's a convention to prefix on with a method handling the corresponding event. So it should be renamed as onClick or onNoteClicked actually. This is a hidden issue about convention and expressiveness.

}).bind(this));
},

function updateList(list) {
_listNoteContent = list;
}
onNoteOpen(event){
if(event.target.classList.contains('note-title')){
var id = event.target.dataset.noteId;
var content = this._listNoteContent[id];
window.dispatchEvent(new CustomEvent('note-open',
{ detail: content }));
};
),

function drawList() {
var list = _listNoteContent;
var ul = document.createElement('ul');
ul.id = 'note-title-list';
var buff = document.createDocumentFragment();
list.forEach(function(note, i) {
var li = document.createElement('li');
li.dataset.noteId = i;
li.classList.add('note-title');
li.textContent = note.title;
// Note: buff is captured, so we now have a
// little closure naturally.
buff.appendChild(li);
});
ul.appendChild(buff);
_wrapper.appendChild(ul);
}

function fetchList(afterFetch) {
var xhr = new XMLHttpRequest();
xhr.open('GET', 'http://127.0.0.1:8000/demo-list-notes.json', true);
xhr.responseType = 'json';
xhr.onreadystatechange = function(e) {
// Watch out: we have a mysterious unknown 'this'.
if (this.readyState === 4 && this.status === 200) {
var listData = this.response;
// The flow ends here.
afterFetch(listData);
} else if (this.status !== 200 ){
// Ignore error in this case.
preloadFirstNote(){
if(this._listNoteContent.length !== 0){
Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

nit: If a function is only with one branch of conditional statement, the condition should be moved into the caller, rather the callee.

var content = this._listNoteContent[0];
window.dispatchEvent(new CustomEvent('note-open',
{ detail: content }));
}
};
xhr.send();
}
},

updateList(list){
this._listNoteContent = list;
},

document.addEventListener('DOMContentLoaded', function(event) {
start();
});
drawList(){
var list = this._listNoteContent;
var ul = document.createElement('ul');
ul.id = 'note-title-list';
var buff = document.createDocumentFragment();
list.forEach(function(note, i){
var li = document.createElement('li');
li.dataset.noteId = i;
li.classList.add('note-title');
li.textContent = note.title;
// Note: buff is captured, so we now have a
// little closure naturally
buff.appendChild(li);
});
ul.appendChild(buff);
this._wrapper.appendChild(ul);
},

})();
fetchList(){
return new Promise(function(resolve, reject){
Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

Good. You have successfully "Promisified" it.

var xhr = new XMLHttpRequest();
xhr.open('GET', 'http://127.0.0.1:8000/demo-list-notes.json', true);
xhr.responseType = 'json';
xhr.onreadystatechange = function(e){
if(xhr.readyState === 4 && xhr.status === 200){
var listData = xhr.response;
// The flow ends here.
resolve(listData);
}
else if (xhr.status !== 200){
reject(xhr);
}
};
xhr.send();
});
}
}
exports.ListManager = ListManager;
})(window);
9 changes: 9 additions & 0 deletions homework/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict'

// Kick off
document.addEventListener('DOMContentLoaded',function(event){
var contentManager = new ContentManager();
var listManager = new ListManager();
contentManager.start();
listManager.start();
});
2 changes: 1 addition & 1 deletion homework/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"sinon": "^1.17.1"
},
"scripts": {
"test": "mocha"
"test": "./node_modules/karma/bin/karma start"
},
"author": "",
"license": "ISC"
Expand Down
22 changes: 19 additions & 3 deletions homework/test/test-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,24 @@
describe('Test > ', function() {
describe('Test ListManager', function() {
var subject;

beforeEach(function() {
subject = new ListManager();
});

it('will test some pure functions', function() {
// Write any pure function assertion here.
describe('updateList',function(){
it('will update the inner list of the manager', function(){
var manager = subject;
var dummuyList = [
{ "title": "US signals shift in Syria-Iraq campaign against Islamic State",
"passages": [
"The US has indicated a shift in its campaign against Islamic State (IS) militants in Iraq and Syria, including the use of direct ground raids.",
"Defence Secretary Ash Carter said there would also be more air strikes against high-value targets."
]}
];

manager.updateList(dummuyList);
Copy link
Contributor

@snowmantw snowmantw Nov 10, 2015

Choose a reason for hiding this comment

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

It's good to see you test a real instance and method, rather than a mock one.


assert.equal(subject._listNoteContent, dummyList);
});
});
});