The purpose of this file is to make an Ajax call to a certain endpoint, and update a certain fields on the HTML page, auto populate the dropdown-menu, table, and piechart.

My script will turn this:

enter image description here

to this:

enter image description here

I'm not sure whether or not I should do all my logic within my Ajax success. Should I separate it? This is what I have. Is it really that bad?

'use strict';

define(['jquery', 'moment'], function($, moment) {

    $(function() {

        var report_type_car   = $('#as-report-type-car');
        var report_type_cdr   = $('#as-report-type-cdr');
        var report_type_title = $('#as-report-type-title');
        var section_num       = $('#as-section-num');
        var problem_set       = $('#as-problem-set');
        var start_time        = $('#as-start-time');
        var due_time          = $('#as-due-time');
        var student_am        = $('#as-student-am');
        var student_total     = $('#as-student-total');
        var submit_am         = $('#as-submit-am');
        var submit_total      = $('#as-submit-total');
        var avg_score         = $('#as-avgscore');
        var danger            = $('#pc-danger');
        var warning           = $('#pc-warning');
        var success           = $('#pc-success');
        var danger_list       = $('#pc-danger-list');
        var warning_list      = $('#pc-warning-list');
        var success_list      = $('#pc-success-list');

        //var basePath = "/BIM/resources/js/reports/teacher/section-exercise/assignment/";


            url: "/BIM/rest/report/assignment",
            type: "POST",
            dataType: "json",
            data: {
                assessmentId: "206a9246-ce83-412b-b8ad-6b3e28be44e3",
                classroomId: "722bfadb-9774-4d59-9a47-89ac9a7a8f9a"

            success: function(objects) {

                var x = 0;

                function updateInfo(x) {

                    var json = objects.assignments[x].header;
                    var name = objects.assignments[x].name;

                    // Check for space in report_type
                    if (json.report_type.indexOf(' ') >= 0) {

                        var report_type_full = json.report_type.split(/(\s+)/);
                        var car = report_type_full[0];
                        var cdr = report_type_full[2];

                        report_type_title.html(car + " " + cdr + " " + json.section_num);

                    } else {

                        var car = json.report_type;
                        report_type_title.html(car + " " + json.section_num);


                    // Not show date if summary
                    if (name == "Summary") {

                        start_time.html(" ");
                        due_time.html(" ");


                    } else {


                        start_time.html(moment(parseInt(json.start_time)).format("MM/DD/YYYY h:mm A"));
                        due_time.html(moment(parseInt(json.due_time)).format("MM/DD/YYYY h:mm A"));





                // Load the Visualization API and the piechart package.
                google.load("visualization", "1", {
                    packages: ["corechart"]

                function drawChart() {

                    var options = {
                        width: 160,
                        height: 160,
                        chartArea: {
                            left: 10,
                            top: 20,
                            width: "100%",
                            height: "100%"

                        colors: ['#F46E4E', '#F9C262', '#ADB55E', ],
                        legend: 'none',
                        enableInteractivity: false,
                        pieSliceText: 'none',


                    var chart = new google.visualization.PieChart(document.getElementById('piechart'));
                    var data = {};

                    $.each(objects.assignments, function(i, v) {

                        var header = v.header;
                        var name  = v.name;
                        var total = header.danger + header.warning + header.success;

                        data[i] = new google.visualization.arrayToDataTable([

                            ['Piechart', 'Number of Skills'],
                            ['danger', (header.danger / total) * 100],
                            ['warning', (header.warning / total) * 100],
                            ['success', (header.success / total) * 100],


                        // Auto Populate the dropdown-menu
                        if (i == 0) {
                            $("#as-dd.dropdown").append('<option value="' + name + '"> Summary </option>');
                        } else {
                            $("#as-dd.dropdown").append('<option value="' + name + '">' + name + '</option>');


                    // Dropdown-menu change
                    $('#as-dd').on('change', function() {
                        var value = $(this).val();
                        chart.draw(data[value], options);

                    // Initializing
                    chart.draw(data[0], options);





  • 1
    \$\begingroup\$ When you ask if you should do all logic within Ajax success, it may mean two different things. First, physically speaking: IMO you might consider to locate your updateInfo() and drawChart() functions outside of the Ajax success:, for readalility. This way, the main body of the success process appears more clearly. Then, logically speaking: I didn' take time to precisely understand how this works, but I guess you should have some code (may be the Initializing sequence?) outside of the success: part, something like a always:part, to cover the case of potential errors. \$\endgroup\$
    – cFreed
    Jul 16, 2015 at 15:35

1 Answer 1


I'm agree with the improvements suggested by @cfreed, you should extract updateInfo() and drawChart() outside of the ajax call. This should be a better split btw the data/view. A side effect, could be doesn't have all jquery selectors on the top of the request.

Also, I would like add my owns:

  • If you are using $.ajax with that configuration, you can cosidere use the shortwhand mehtods like $.post
  • is a good convention use camelCase instead of sanake_case for variable names.
  • you should consider use some kind of default value for the assigments:

    var json = objects.assignments[x].header || {} not always the server response with the full data. Asume null values and considere to use null object pattern.

  • Be more expresive in name variables:

    $.each(objects.assignments, function(i, v) {//who is v?


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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