HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Cover letter generator: guides writing a template and autofills information

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
templatewritingandgeneratorinformationguideslettercoverautofills

Problem

This is my first public web-application. I'm a beginner to JavaScript and this is my first time using jQuery. It's not complete, but I'd love some feedback on the programming and the functionality. As I said, I'm just learning and I'm not sensitive, so feel free to be thorough with your criticism/feedback.

You can view source/see in action here, and here's the current code:



`// for bootstrap popup tips
$("#blob").popover();
$('#company_name').popover();
$('#company_address').popover();
$('#reason').popover();
$('#opening_paragraph').popover();
$('#closing_paragraph').popover();

// this resets the company information fields and
// scrolls to the section
$('#reset_company').click(start_new_letter);
$('#restart_button').click(start_new_letter);

function start_new_letter(e) {
$('#company_address').val("");
$('#company_name').val("");
$('#position_title').val("");
$('#position_code').val("");
$('#reason').val("I want to work here because ");

$("div.preview_div").html("");

$('html,body').animate({
scrollTop: $('#company_div').offset().top
}, 'slow');
};

// this opens an iframe when the editor button is clicked
$('#editor').click(function () {
document.getElementById("hemingway_editor").innerHTML = "";
});

// this googles the company name + "company" when clicked
// or gives an alert if no company name has been entered
$('#google_company').click(function () {
if ($('#company_name').val() == "") {
$('#company_name_td').append('×Close You need to enter a company name for me to search. ');
} else {
window.open("http://www.google.com/search?q=" + $('#company_name').val() + " company");
}
});

// this does the same but for the company address
$('#google_address').click(function () {
if ($('#company_name').val() == "") {
$('#company_address_td').append('×Close You need to enter a company name for me to search. ');
} else {
window.open("http://www.google.com/se

Solution

First of all: be consistent.

$("#blob").popover();
$('#company_name').popover();


Mixed usage of single or double quotes is allowed in JS, but is sloppy.
Use either one or the other, but be consistent.

$("#copy_button").on('click', function copy_content(e) {
$('#google_address').click(function () {


You could not decide, which way to bind the click event?

Then: avoid repetitions in coding. You have to be lazy as a programmer - but smart. Nothing is more tedious than writing the same code over and over again. The bad programmer begins to copy and paste - the good one finds a way, so that the language does the work.

$("#blob").popover();
$('#company_name').popover();
$('#company_address').popover();
$('#reason').popover();
$('#opening_paragraph').popover();
$('#closing_paragraph').popover();


Why not simply make a function:

function makePopover(tags){
    tags.map(function(tag){ $('#'+tag).popover(); });
}


Then you could easily pass an array like

tags=['blob','company_name', 'company_address', 'reason', 'opening_paragraph', 'closing_paragraph']
makePopover(tags);


Done.

Third: Your Javascript is completely unstructured.
Sorry Bro, but that is a complete mess. You have to start over.
First you do all this popoverstuff, then you bind click-Events, after that you declare functions. I do not want to read this jungle of code.

I suggest the following, which I explain in a minute:

myApp=function(){
  function init(){
    /* do all initialization here */
  };

  /* all application logic goes here */

  return {
    init:init
  };

}();

$(function(){ myApp.init(); });


What is going on in this weird looking code?

1) a variable myApp is declared and defined.
The advantage of this structure is, that you litter the global namespace only with one variable: myApp.

2) then I use a neat trick from the JS toolbelt, which you have to build yourself. It is called Immediate Invoked Function Expression or IIFE for short.

The structure is like this:

a=function(){}();


As the name suggests it is a) a function (expression) and b) it is immediately invoked.
That means, the moment, a is defined, the function gets executed - this happens, because I wrote () after the declaration of the function.

3) an object is returned. The object is defined with a notation called Java Script Object Notation - which is otherwise known as JSON. This allows to notate an object in a nice, simple and clean way.

person={name:"Doe", firstName:"John"};


This is much cleaner than to instantiate a new object.
In our case, I return an object, which has a property named init and which is bound to the function of the same name.
What happens is the following:
After all is done, myApp is an object, with a "public" function, which itself has access to the context of the anonymous function. which was invoked at the time of the definition of myApp. This preserves all the state including the variables. The function closes over the state. This concept is known as a closure. Do not mind if you do not get it all. I show you the path and you should go along and play with it.

Here is a Fiddle to play with.

4) To get an even better structure you could do the following

$('#editor').click(function () {


This pattern to bind the function is very hard to read. I have to parse, what your anonymous inner function does, before I know what you are going to do.
Split the binding of the behaviour from the definition of the behaviour:

function iAmGoingToDoThisWhenClicked(){};
$('#element'). on('click', iAmGoingToDoThisWhenClicked);


To make a long story short. This is an unstructured mess (which seems to contain copy and paste elements from across several tuts found on the net). And you have a lot of cleanup work to do.

Code Snippets

$("#blob").popover();
$('#company_name').popover();
$("#copy_button").on('click', function copy_content(e) {
$('#google_address').click(function () {
$("#blob").popover();
$('#company_name').popover();
$('#company_address').popover();
$('#reason').popover();
$('#opening_paragraph').popover();
$('#closing_paragraph').popover();
function makePopover(tags){
    tags.map(function(tag){ $('#'+tag).popover(); });
}
tags=['blob','company_name', 'company_address', 'reason', 'opening_paragraph', 'closing_paragraph']
makePopover(tags);

Context

StackExchange Code Review Q#57483, answer score: 7

Revisions (0)

No revisions yet.