Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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:

JavaScript:

// 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 = "<iframe src='http://www.hemingwayapp.com/' height='700' width='90%'></iframe>";
});

// 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('<div class="alert alert-danger alert-dismissible" role="alert"><button type="button" class="close" data-dismiss="alert"><span aria-hidden="true">&times;</span><span class="sr-only">Close</span></button> You need to enter a company name for me to search. </div>');
    } 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('<div class="alert alert-danger alert-dismissible" role="alert"><button type="button" class="close" data-dismiss="alert"><span aria-hidden="true">&times;</span><span class="sr-only">Close</span></button> You need to enter a company name for me to search. </div>');
    } else {
        window.open("http://www.google.com/search?q=" + $('#company_name').val() + " address");
    }
});

// this adds and deletes rows from the skill bank when the 
// add row / delete row buttons are pressed
$(document).ready(function () {
    var i = 1;
    $("#add_row").click(function () {
        $('#addr' + i).html('<td> <div class="input-group"> <span class="input-group-addon"> <input type="checkbox" checked></span> <textarea class="form-control" placeholder=\'Describe a skill. Use \"company\" and \"position\" as placeholders (quotes included), but do not overdo it. A few mentions make the letter look tailored but too many look weird.\' rows=3 style="width:100%"></textarea></div><!-- /input-group --> </td>');

        $('#tab_logic').append('<tr id="addr' + (i + 1) + '"></tr>');
        i++;
    });
    $("#delete_row").click(function () {
        if (i > 1) {
            $("#addr" + (i - 1)).html('');
            i--;
        }
    });

})


// this repaces javascript newlines with html breaks 
// when called, I don't think I'm using it anymore
// but I have plans that might entail using it
function htmlForTextWithEmbeddedNewlines(text) {
    var htmls = [];
    var lines = text.split(/\n/);
    alert("it is used");
    // The temporary <div/> is to perform HTML entity encoding reliably.
    //
    // document.createElement() is *much* faster than jQuery('<div></div>')
    // http://stackoverflow.com/questions/268490/
    //
    // You don't need jQuery but then you need to struggle with browser
    // differences in innerText/textContent yourself
    var tmpDiv = jQuery(document.createElement('div'));
    for (var i = 0 ; i < lines.length ; i++) {
        htmls.push(tmpDiv.text(lines[i]).html());
    }
    return htmls.join("<br>");
}

// this copies the text in the output to the user's 
// clipboard when the copy button is clicked
$("#copy_button").on('click', function copy_content(e) {
    e.preventDefault();
}).zclip({
    path: 'http://www.steamdev.com/zclip/js/ZeroClipboard.swf',
    copy: function () {
            var html_text = $('#preview_div').html();
            html_text = html_text.replace(/<br> /g, '\n');
            html_text = html_text.replace(/<br>/g, '\n');
            html_text = html_text.replace(/<b>/g, '');
            html_text = html_text.replace(/<\/b>/g, '');
            return html_text;
    }
});

// this allows the user to go through the different input areas
// using the enter button instead of the tab button 
$('body').on('keydown', 'input, select, textarea', function(e) {
    var self = $(this)
      , form = self.parents('form:eq(0)')
      , focusable
      , next
      ;
    if (e.keyCode == 13) {
        focusable = form.find('input,a,select,textarea').filter(':visible');
        next = focusable.eq(focusable.index(this)+1);
        if (next.length) {
            next.focus();
        } else {
            form.submit();
        }
        return false;
    }
});

// this stops the form from being submitted when the user presses
// the enter button
$("form").submit(function(e) {
    e.preventDefault();
});

$('#salutation2').hide();

// this just allows the same generate function to be 
// run from two different buttons with different IDs
$('#preview1').click(get_data);
$('#preview2').click(get_data);

// this organizes the header info into the right format
// and returns it as a string
function get_header(company_name, company_address, position_title, position_code, salutation){

    var d = new Date();
    var monthNames = ["January", "February", "March", "April", "May", "June", "July",
        "August", "September", "October", "November", "December"];
    var date = monthNames[d.getMonth()] + " " + d.getDate() + ", " + d.getFullYear() + "<br><br>";

    var header = date;
    company_name + "<br>";
    header += company_address + "<br>";
    header += "<br> Re: " + position_title + " Position " + position_code + "<br><br>";
    header += salutation;

    header += "<br><br>";

    return header;
};

// this gets the skill data and formats it into a paragraph
// and returns it as a string
function get_skills(e){
    var skill_list = [];
    var check_list = [];
    // get the skills
    $("#tab_logic").find('textarea').each(function (i, el) {
        var skill = $(this).val();
        skill_list.push(skill);
    });

    if( salutation === "Other") {
            salutation = $('#salutation2').val();
    }
    if (skill_list[0] == "") {
        skill_list[0] = 'I have a strong programming foundation and am motivated to learn more, so I would be an asset for your "position" position.';
     }

    if (skill_list[1] == "") {
        skill_list[1] = 'As well as technical skills, I have good interpersonal and communication skills and would fit in well with your workplace culture.'; 
     }
    $("#tab_logic").find('input[type="checkbox"]').each(function (i, el) {
        var check = $(this).prop("checked");
        check_list.push(check);
    });
    var skill_string = "";

    for (var i = 0; i < skill_list.length; i++) {
        if (check_list[i] == true) {
            skill_string += " " + skill_list[i];
        }
    }
    return skill_string;
}

// this gets all the data from all the fields,
// replaces it with placeholders if they're undefined
// organizes the header, output, skills, and paragraphs
// into the long string, then outputs it
function get_data(e) {

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

    var company_name = $('#company_name').val();
    var company_address = $('#company_address').val();
    var position_title = $('#position_title').val();
    var position_code = $('#position_code').val();
    var reason = $('#reason').val();
    reason = reason.replace("I want to work here because ", "");
    var opening_paragraph = $('#opening_paragraph').val();
    var closing_paragraph = $('#closing_paragraph').val();
    var name = $('#name').val();
    var email = $('#email').val();
    var phone = $('#phone').val();
    var salutation = $('#salutation').val();
    var signature = $('#signature').val();

    if (company_name == "") {
        company_name =  $("#company_name").attr('placeholder');
    }
    if (company_address == "") {
        company_address =  $("#company_address").attr('placeholder');
    }
    if (position_title == "") {
        position_title =  $("#position_title").attr('placeholder');
    }
    if (position_code !== "") {
        position_code = "#" + position_code;
    }
    if (reason == "") {
        reason = '<b>"reason"</b>';
    }
    if (email == "") {
        email = '<b>"email"</b>';
    }
    if (phone == "") {
        phone = '<b>"phone"</b>';
    }
    if (name == "") {
        name = '<b>"name"</b>';
    }
    if (opening_paragraph == "") {
        opening_paragraph = '<b>"opening paragraph"</b>';
    }
    if (closing_paragraph == "") {
        closing_paragraph = '<b>"closing paragraph"</b>';
    }

    var header = get_header(company_name, company_address, position_title, position_code, salutation);

    var skill_string = get_skills();

    var paragraphs = opening_paragraph + "<br><br>"
    paragraphs += skill_string + "<br><br>";
    paragraphs += closing_paragraph;

    var closing = "<br><br>Thank you for your time and consideration.";
    closing += '<br><br>' + signature + '<br><br>"name"';

    var output_text = header + paragraphs + closing;

    output_text = output_text.replace(/"reason"/g, reason);
    output_text = output_text.replace(/"company"/g, company_name);
    output_text = output_text.replace(/"phone"/g, phone);
    output_text = output_text.replace(/"email"/g, email);
    output_text = output_text.replace(/"position"/g, position_title);
    output_text = output_text.replace(/"name"/g, name);

    output_text = output_text.replace("..", ".");

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

HTML:

<!DOCTYPE html>
<html lang="en">


    <head>
        <meta charset="utf-8">
        <meta http-equiv="X-UA-Compatible" content="IE=edge">
        <meta name="viewport" content="width=device-width, initial-scale=1">
        <title>Cover Letter Tutor</title>
        <!-- Bootstrap core CSS -->
        <link href="css/bootstrap.min.css" rel="stylesheet">
        <!-- Custom styles for this template -->
        <link href="custom_style.css" rel="stylesheet">
        <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
        <script type="text/javascript" src="js/jquery.zclip.min.js"></script>
        <script type="text/javascript" src="js/bootstrap.min.js"></script>
         <script type="text/javascript" src="http://www.steamdev.com/zclip/js/jquery.snippet.min.js"></script>


         <link rel="icon" href="favicon.ico" type="image/x-icon" />

    </head>

    <body>

        <!-- The Header and Navigation Tabs -->
        <div class="container">
            <div class="header">
                <ul class="nav nav-pills pull-right">
                    <li class=""><a id="reset_company" href="#">Start New Letter</a>
                    </li>
                    <li class=""><a id="about" href="about.html">About</a>
                    </li>
                     <li class="dropdown"> <a class="dropdown-toggle" data-toggle="dropdown" href="#">
          Download <span class="caret"></span>
        </a>

                        <ul class="dropdown-menu" role="menu">
                            <li class="disabled"><a href="#">As Word Doc (Upcoming)</a>
                            </li>
                            <li class="disabled"><a href="#">As PDF (Upcoming)</a>
                            </li>
                            <li class="divider"></li>
                            <li class=""><a id="" href="javascript:void(0);" onclick="copy_content()">Copy To Clipboard</a>
                            </li>
                        </ul>
                    </li>
                </ul>
                 <h1 class="text-muted">Cover Letter Tutor</h1>

            </div>
        </div>
        <!-- GRAYED AREA -->
        <div style="text-align:center;" class="preview_button">
            <button type="button" id="preview1" class="btn btn-success btn-md"> <span class="glyphicon glyphicon-file"></span> Preview Example</button>
        </div>
        <br>
        <div class="container">
            <div class="jumbotron" style="text-align:left; padding-top:15px;" id="company_div">
                <!-- Form Name -->
                <legend>Company Information</legend>
                <form id="company_form" class="form-horizontal">
                    <table>
                        <tr>
                            <td style="padding-top:15px; vertical-align:top;">Company Name</td>
                            <td id="company_name_td">
                                <div style="width:100%" class="input-group">
                                    <input id="company_name" style="width:100%" class="form-control" type="text" data-toggle="popover" data-trigger="select" data-placement="top" placeholder="Pied Piper" data-content="Don't be  too formal with the name, 'Pied Piper Corporation Ltd.' doesn't sound natural." /> <span style="width:35%" class="input-group-btn">
      <button id="google_company" class="btn btn-primary" type="button" style="width:100%" >Google Company</button>
      </span>
                                </div>
                            </td>
                        </tr>
                        <tr>
                            <td style="padding-top:15px; vertical-align:top;">Company Address</td>
                            <td id="company_address_td">
                                <div style="width:100%" class="input-group">
                                    <input style="width:100%" type="text" id="company_address" class="form-control" placeholder="200 Bachman Rd <br> Silicon Valley, CA 95101" type="text" data-toggle="popover" data-trigger="select" data-placement="top" data-content="For multi addresses, write <br> where you want to start a new line." /> <span style="width:35%" class="input-group-btn">
      <button style="width:100%" id="google_address" class="btn btn-primary" type="button">Google Address</button>
      </span>
                                </div>
                            </td>
                        </tr>
                        <tr>
                            <td>Position Title/Code</td>
                            <td id="company_address_td">
                                <div class="input-group">
                                    <input type="text" id="position_title" class="form-control" placeholder="Software Developer" style="width:65%;" class="form-control" type="text" /> <span class="">
      <input type="text" id="position_code" style="width:35%;" class="form-control" placeholder="123 (If applicable)" type="text" />      </span>
                                </div>
                            </td>
                        </tr>
                        <tr>
                            <td>Complete the
                                sentence</td>
                            <td>
                                    <textarea style="width:100%" id="reason" class="form-control" data-toggle="popover" data-trigger="select" data-placement="top" data-content="This is the most important part of the letter. It's worth reading up on the company a bit to make this stand out. And be genuine!" >I want to work here because </textarea>
                            </td>
                        </tr>
                        <tr>
                            <td></td>
                            <td>
                                <button type="button" id="blob" class="btn btn-md btn-danger pull-right" data-toggle="popover" data-placement="left" title="Google the company and find any of the following" data-content="Mention a specific project &bullet; Mention something about the workplace culture &bullet; Mention a certain practice the company has &bullet; Mention the location and why you'd want to move there">Click here for ideas</button>
                            </td>
                        </tr>
                    </table>
                </form>
            </div>
            <div class="jumbotron" style="text-align:left; padding-top:15px;">
                <legend>Describe Your Skills</legend>This is your skill bank. Describe a few technical and personal skills, such as programming, Microsoft Office, communication, or teamwork. Read through the job description and customize each cover letter by checking and unchecking which skills they're looking for.
                <br>
                <br>
                <div class="row clearfix">
                    <div class="col-md-12 column">
                        <table class="table table-hover" id="tab_logic" style="width:100%;">
                            <tbody>
                                <tr id='addr0'>
                                    <td>
                                        <div class="input-group"> <span class="input-group-addon">
        <input type="checkbox" checked>
      </span> 
                                            <textarea style="width:100%" class="form-control" rows="2" style="width:100%" placeholder='I have a strong programming foundation and am motivated to learn more, so I would be an asset for your "position" position.'></textarea>
                                        </div>
                                        <!-- /input-group -->
                                        <div class="col-lg-6">
                                    </td>
                                </tr>
                                <tr id='addr0'>
                                    <td>
                                        <div class="input-group"> <span class="input-group-addon">
        <input type="checkbox" checked>
      </span>

                                            <textarea class="form-control" rows="2" style="width:100%" placeholder='As well as technical skills, I have good interpersonal and communication skills and would fit in well with your workplace culture.'></textarea>
                                        </div>
                                        <!-- /input-group -->
                                        <div class="col-lg-6">
                                    </td>
                                </tr>
                                <tr id='addr1'></tr>
                            </tbody>
                        </table>
                        </div>
                        </div>  <a id="add_row" class="btn btn-primary pull-left btn-sm">Add Skill</a><a id='delete_row' class="pull-right btn btn-primary btn-sm">Remove Skill</a> 
                        <br>
                    </div>
                    <div class="jumbotron" style="text-align:left; padding-top:15px;">
                        <!-- Form Name -->
                        <legend>Letter Content</legend>This is where most of the content for your cover letter is. Edit these paragraphs to suit your style, using "position" and "company" where relevant. The opening paragraph is the only place you should include "reason". Apart from those placeholders, these templates will be the same for all your cover letters.
                        <br>
                        <br>
                        <form class="form-horizontal">
                            <table>
                                <tr>
                                    <td>Opening
                                        <br>Paragraph Template</td>
                                    <td>
                                        <textarea id="opening_paragraph" rows=4 cols=50 class="form-control" data-trigger="select" data-placement="top" data-content='Express enthusiasm to work for the company, tell them why you are a good fit for the position, and give a specific reason you want to work there.'>I believe I am a good candidate for the "position" position you have available. The reason I want to work specifically at "company" is because "reason".</textarea>
                                    </td>
                                </tr>
                                <tr>
                                    <td>Closing
                                        <br>Paragraph Template</td>
                                    <td>
                                        <textarea id="closing_paragraph" rows=4 cols=50 class="form-control" data-toggle="popover" data-trigger="select" data-placement="top" data-content='Tell them how you could contribute to the company, invite them to give you the chance to, and give them your contact information.' >"company" is a company that I think I could contribute to and I would appreciate the opportunity to prove it. To arrange an interview please contact me at "email" or "phone".</textarea>
                                    </td>
                                </tr>
                                <tr>
                                    <td>Salutation</td>
                                    <td>
                    <select class="form-control" id='salutation' name = 'color' onchange = "if ($('#salutation').val() == 'Other') {
                      $('#salutation2').show();
                    } else {
                      $('#salutation2').hide();
                    }">
       <option value="Dear Hiring Manager,">Dear Hiring Manager</option>
       <option value="Dear Sir/Madam,">Dear Sir/Madam</option>
       <option value="To Whom It May Concern,">To Whom It May Concern</option>
       <option value="Other">Other (specific names are a good idea)</option>

                                        </select>

<input class="form-control" type='text' id='salutation2' placeholder="Dear Ms. Smith,"/>



                                        <tr>
                                            <td>Signature</td>
                                            <td>
                                                <select class="form-control" id="signature">
                                                    <option value="Sincerely,">Sincerely</option>
                                                    <option value="Yours truly,">Yours truly</option>
                                                    <option value="Thankfully,">Thankfully</option>
                                                    <option value="Regards,">Regards</option>
                                                    <option value="Cheers,">Cheers</option>
                                                    <option value="All the best,">All the best</option>
                                                </select>
                            </table>
                        </form>
                    </div>
                </div>
            </div>
            <div class="container">
                <div class="jumbotron" style="text-align:left; padding-top:15px;">
                    <legend>Personal Information</legend>You can leave this part blank if you prefer, but rest assured that this website doesn't store or do anything with the entered content. I don't even know how to do that.
                    <br>
                    <br>
                    <form class="form-horizontal">
                        <table>
                            <tr>
                                <td>Your name</td>
                                <td>
                                    <input type="text" size="40" class="form-control" id="name" placeholder="crclayton" style="width:100%">
                                </td>
                            </tr>
                            <tr>
                                <td>Your email</td>
                                <td>
                                    <input type="text" class="form-control" id="email" placeholder="techsavvy@comcast.com" style="width:100%">
                                </td>
                            </tr>
                            <tr>
                                <td>Your number</td>
                                <td>
                                    <input type="text" class="form-control" id="phone" placeholder="000-123-4567" style="width:100%">
                                </td>
                            </tr>
                    </form>
                    </table>
                    <div></div>
                </div>
            <div style="text-align:center">
                <button type="button" id="preview2" class="btn btn-success btn-md"> <span class="glyphicon glyphicon-file"></span> Generate</button> <button id='copy_button' class="btn btn-success btn-md"><span class="glyphicon glyphicon-arrow-down"></span> Copy</button> <button id='restart_button' class="btn btn-success btn-md"><span class="glyphicon glyphicon-repeat"></span> Restart </button> <button id='editor' class="btn btn-success btn-md"><span class="glyphicon glyphicon-edit"></span> Editor </button>
                    </div></div>
            <br>
            <br>
            <div class="preview_div" id="preview_div" style="font-family:Times New Roman; font-size:12; padding-right:20%; padding-left:20%;"></div>
            <br>
            <div id="hemingway_editor" style="text-align:center;">
                </div>
            <div class="footer container">
                <p>&copy; crclayton 2014</p>
            </div>
            </div>

</div>
share|improve this question

1 Answer 1

First of all: be consistent

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

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

$("#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.

share|improve this answer
    
Okay, thank you for all your points! Genuinely. which seems to contain copy and paste elements from across several tuts found on the net that's exactly right, I managed to hack together something that functioned based on things I've seen online but with no real understanding of what's considered good. Most of my background is C, Assembly, and playing around in Python. I'll work on all of those things. –  crclayton Jul 20 at 1:48
    
Feel free to ask, whenever and whatever. And do not be afraid, although some comments read (perhaps) harsh ;) I think as a genuine C hacker, you have a hard time, because JS is quite different. I stumbled some days ago over some videos, which might help you a bit understanding JS: youtube.com/watch?v=JEq7Ehw-qk8 and I recommend reading this tour de force through all of JS: 2ality.com/2013/06/basic-javascript.html –  Thomas Junk Jul 20 at 7:51
1  
Great review, just one comment: that makePopover function can be replaced by $("blob,company_name,company_address,reason,opening_paragraph,closing_paragraph‌​").popover() (twbs allows it) –  megawac Jul 21 at 4:57
    
The good one, knows how to use the language and the master knows the framework ;-) –  Thomas Junk Jul 21 at 5:49

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.