current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

I have been told for my code below i need to "Improve and optimise the codebase of the application to reflect modern and best coding practices". Can anyone help me in doing this? As I know the service worker code can't be optimised so I am not sure what other code I can change/improve for best coding practices. The HTML page attached to this is mainly just buttons.

//Make IE play nice with HTML5 elements
document.createElement('header');
document.createElement('section');
document.createElement('article');
document.createElement('footer');



// ServiceWorker is a progressive technology. Ignore unsupported browsers
if ('serviceWorker' in navigator) {
  console.log('CLIENT: service worker registration in progress.');
  navigator.serviceWorker.register('/service-worker.js').then(function() {
    console.log('CLIENT: service worker registration complete.');
  }, function() {
    console.log('CLIENT: service worker registration failure.');
  });
} else {
  console.log('CLIENT: service worker is not supported.');
}






var notepad = {
	notes:[]
};

var currentNoteKey,
	viewEdit,
	viewList,
	noteTitleElement,
	noteContentElement;


function createNote(){
	//Create a blank new note
	var newNote = {
		'name':'New Note',
		'content':'Hello World',
		'last-modified':+new Date()
	}

	//Add the note to the array, and keep track of it's key
	var newNoteKey = 0;
	while(typeof notepad.notes[newNoteKey] !== 'undefined'){
		newNoteKey++;
	}
	currentNoteKey = newNoteKey;
	notepad.notes[newNoteKey] = newNote;

	//Redraw the list of notes, and show the edit view
	drawNotesList();
	changeView();
}

function drawNotesList(){
	//Sort the notes by most recently modified first
	notepad.notes.sort(function(a,b){
		if(a['last-modified'] < b['last-modified']){
			return 1;
		}else if(a['last-modified'] > b['last-modified']){
			return -1;
		}else{
			return 0;
		}
	});
	
	//Generate & Apply the HTML
	var notesList = '';
	for(key in notepad.notes){
		var thisNote = notepad.notes[key];
		notesList += '<li><a href="#" data-key="' + key + '" onclick="noteListClick(this)">' + thisNote.name + '</a></li>';
	}
	if(notesList == ''){
		notesList = '<li class="info">No notes yet</li>';
	}
	document.getElementById('notes-list').innerHTML = notesList;

}

function noteListClick(link){
	currentNoteKey = link.getAttribute('data-key');
	changeView();
}

function changeView(){
	//Used to change between note list and note edit views
	if(viewEdit.className === 'visible'){
		//Transitioning to the Notes List view
		viewEdit.className = '';
		viewList.className = 'visible';
		currentNoteKey = null;
		drawNotesList();
	}else{
		//Transitioning to the Note Edit view
		if(populateEditView()){
			viewEdit.className = 'visible';
			viewList.className = '';
		}
	}
}

function saveNote(){
	notepad.notes[currentNoteKey].content = noteContentElement.value;
	notepad.notes[currentNoteKey]['last-modified'] = +new Date();
}

function populateEditView(){
	if(typeof currentNoteKey === 'undefined' || typeof notepad.notes[currentNoteKey] === 'undefined'){
		alert("Oops, can't find that note!");
		return false;
	}

	noteTitleElement.innerHTML = notepad.notes[currentNoteKey].name;
	noteContentElement.value = notepad.notes[currentNoteKey].content;

	return true;

}

function renameNote(){
	var newName = prompt("New note name:", notepad.notes[currentNoteKey].name);
	notepad.notes[currentNoteKey].name = newName;
	notepad.notes[currentNoteKey]['last-modified'] = +new Date();
	noteTitleElement.innerHTML = newName;
	
}

function deleteNote(){
	if(typeof currentNoteKey !== 'undefined' && typeof notepad.notes[currentNoteKey] !== 'undefined' && confirm("Are you sure?")){
		delete notepad.notes[currentNoteKey];
		changeView();
	}
}

window.onload = function(){
	//Grab references to DOM elements
	viewEdit = document.getElementById('view-edit');
	viewList = document.getElementById('view-list');
	noteTitleElement = document.getElementById('note-title');
	noteContentElement = document.getElementById('note-content');

	//Set up events
	document.getElementById('btn-new').onclick = createNote;
	document.getElementById('btn-back').onclick = changeView;
	document.getElementById('btn-rename').onclick = renameNote;
	document.getElementById('btn-delete').onclick = deleteNote;
	document.getElementById('note-content').onkeyup = saveNote;
	
}
<!doctype html>
<html><head>
		<title>Notepad (Online)</title>
        
		<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1" />
		<link rel="stylesheet" href="notepad.css" />
		
        
	</head>
	<body>

    
		<section id="view-list" class="visible">
			<header>
				<button id="btn-new" class="right">New</button>
                <a href="login.html">
                <button id="btn-login" class="right">Login</button>
                </a>
				<h1>Notepad</h1>
			</header>
			<ul id="notes-list">
				<li class="info">No notes yet</li>
				<noscript>
					<li><a href="http://enable-javascript.com"><strong>Unfortunately, you'll need JavaScript enabled to use Notepad</strong></a></li>
				</noscript>
			</ul>
		</section>
		<section id="view-edit">
			<header>
				<button id="btn-back">Back</button>
				<h1 id="note-title">Loading Note...</h1>
			</header>
			<textarea id="note-content">Loading...</textarea>
			<footer>
				<button id="btn-rename">Save Note</button>
				<button id="btn-delete">Delete</button>
			</footer>
		</section>
        <script src="notepad.js"></script>
        <script src="service-worker.js"></script>
		
        <script src="localstorage.js"></script>
        
	</body>
</html>

share|improve this question
    
Avoid global variables and use closure. – Bálint May 11 at 8:00
    
can you give me an example on this? – skyrimveteran May 11 at 8:16
up vote 2 down vote

Javascript developers don't like to put hardly anything on the global scope.

To achieve this, surround your script in an auto running function.

(function scope() {
// your variables and functions go here
})();

If you really need to put something in the global scope (there are only very few cases where this applies), put it inside an object:

var globalVariables = {
    variable1: 1,
    variable2: []
}

Then you can acces it by doing

globalVariables.variable1; // returns 1

When you want to access a variable in the closure, return a function from it.

 var sayHello = (function() {
      var closureVariable = "hello";
      return function() {return closureVariable;};
 })();
 sayHello(); //returns hello

Side note: DOCTYPE should be uppercase

share|improve this answer
1  
I think the way to use variables as you describe is good, but your reasons for doing so is bad, javascript in browser already runs in a client, so you are running in a hostile environment from the start anyway (people could edit the code they download from your server) – Pimgd May 11 at 10:26
    
@Pimgd but not on runtime. – Bálint May 11 at 10:33
    
@Quill removed. – Bálint May 11 at 10:36
    
@Bálint runtime editing - stackoverflow.com/questions/7343276/… it's pretty easy - comes as a standard feature in chrome. – Pimgd May 11 at 10:43
    
You can already modify variables at runtime through a breakpoint, even if they're not on the global scope. The main reason we don't like the global scope is because, like every other language, it's really hard to test and bad at encapsulation. Additionally, if you can access something from a closure, you can almost always pass it through as a function argument. – Dan Pantry May 11 at 10:47

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.