3
\$\begingroup\$

This is the JavaScript file that i use for my editor that allows people to edit questions from a form.

I'm wondering if this sort of approach to use JavaScript objects is good or that I kind of miss the point here?

$(document).ready(function() {
    manager = new Customerformquestionmanager();
    manager.formId = firstFormId;
    manager.getCategorys();


    $('#formSelect').change(function() {
        manager.getCategorys();
    });

    $('#categorySelect').change(function() {
        manager.categoryId = $(this).val();
        manager.getQuestions();
    });

    $('.dataTable').on('click', '.editBtn', function() {
        parent = $(this).parent();
        id = parent.find('.rowId').html();
        question = parent.find('.rowQuestion').html();
        type = parent.find('.rowType').html();
        answers = parent.find('.rowAnswer').html();
        customType = parent.find('.rowCustomType').html();


        $('#editId').val(id)
        $('#editQuestion').val(question);
        $('#editType').val(type);
        $('#editAnswers').val(answers);
        $('#editCustomType').val(customType);
    });

    $('#saveBtn').click(function() {
        questionObj = {
            question: $('#editQuestion').val(),
            type: $('#editType').val(),
            custom_answers: $('#editAnswers').val(),
            custom_type: $('#editCustomType').val()
        };

        if($('#editId').val() != '') {
            questionObj.id = $('#editId').val();
        }

        manager.saveQuestion(questionObj);
    });
});

function Customerformquestionmanager() {
    this.formId = false;
    this.currentQuestionId = false;
    this.categoryId = false;
    this.categorySelect = $('#categorySelect');
    this.dataTable = $('.datatTable');
    this.handler = '/handlers/backend/customerformhandler';
}

Customerformquestionmanager.prototype.getCategorys = function() {
    if(this.formId) {
        obj = this;
        $.ajax({
            'url': this.handler,
            'type': 'POST',
            'data': {
                'method': 'getcategorysbyformid',
                'id': this.formId
            }
        }).done(function(data) {
            if(typeof data != 'undefined')
                data = JSON.parse(data);
            if(typeof data != 'undefined') {
                obj.categorySelect.find('option').remove();

                length = data.length;
                for(i = 0; i < length; i++) {
                    selected = '';
                    if(i == 0) {
                        obj.categoryId = data[0].id;
                        selected = 'selected="selected"';
                        obj.getQuestions();
                    }
                    obj.categorySelect.append('<option' + selected + ' value="' + data[i].id + '">' + data[i].title + ' (id: ' + data[i].id + ')</option>');
                }
            }
        });
    }
}

Customerformquestionmanager.prototype.getQuestions = function() {
    if(this.categoryId) {
        obj = this;
        $.ajax({
            'url': this.handler,
            'type': 'POST',
            'data': {
                'method': 'getquestionsbycategoryid',
                'id': this.categoryId
            }
        }).done(function(data) {
            obj.dataTable.find('tr').not(':first').remove();
            if(typeof data !== 'undefined' && data) 
                data = JSON.parse(data);
            if(typeof data !== 'undefined') {
                length = data.length;
                for(i = 0; i < length; i++) {
                    obj.dataTable.append(
                        '<tr id="category' + data[i].id + '">' +
                            '<td class="rowId">' + data[i].id + '</td>' +
                            '<td class="rowQuestion">' + data[i].question + '</td>' +
                            '<td class="rowType">' + data[i].type + '</td>' +
                            '<td class="rowAnswer">' + data[i].custom_answers + '</td>' +
                            '<td class="rowCustomType">' + data[i].custom_type + '</td>' +
                            '<td class="editBtn">edit</td>' +
                            '<td class="redClickable">delete</td>' +
                        '</tr>'
                    );
                }
            }
        });
    }
}

Customerformquestionmanager.prototype.saveQuestion = function(dataobj) {
    if(typeof dataobj != 'undefined') {
        dataobj.method = 'savequestion';
        dataobj.category_id = this.categoryId;
        obj = this;
        if(typeof dataobj.id !== 'undefined')
            this.currentQuestionId = dataobj.id;
        $.ajax({
            'url': this.handler,
            'type': 'POST',
            'data': dataobj
        }).done(function(data) {
            if(typeof data !== 'undefined' && data) {
                data = JSON.parse(data);
                if(typeof data !== 'undefined') {
                    obj.dataTable.append(
                        '<tr id="category' + data.id + '">' +
                            '<td class="rowId">' + data.id + '</td>' +
                            '<td class="rowQuestion">' + dataobj.question + '</td>' +
                            '<td class="rowType">' + dataobj.type + '</td>' +
                            '<td class="rowAnswer">' + dataobj.custom_answers + '</td>' +
                            '<td class="rowCustomType">' + dataobj.custom_type + '</td>' +
                            '<td class="editBtn">edit</td>' +
                            '<td class="redClickable">delete</td>' +
                        '</tr>'
                    );
                    alert('question added');
                }
            }
            else {
                $('category' + obj.currentQuestionId + ' .rowQuestion').html(dataobj.question);
                $('category' + obj.currentQuestionId + ' .rowType').html(dataobj.type);
                $('category' + obj.currentQuestionId + ' .rowAnswer').html(dataobj.answers);
                $('category' + obj.currentQuestionId + ' .rowCustomType').html(dataobj.custom_type);                

                alert('question saved');
            }

        });
    }
}

This is the PHP/HTML template that it works with (not intended to be the focus of this review):

<?php 
    require_once __DIR__ . '/../classes/database.php';
    require_once __DIR__ . '/../classes/security.php';
    require_once __DIR__ . '/../includes/checkauth.php';
    require_once __DIR__ . '/../includes/backendincludes.php';

    $db = Database::getInstance();
    $forms = $db->getAsArray('SELECT id, title FROM ea_customer_form');
    $lastForm = end($forms);
    $lastForm = $lastForm['id'];
    reset($forms); 
?>
<script type="text/javascript" src="/js/backend/customerformquestion.js"></script>
<script type="text/javascript">var firstFormId = <?php echo $lastForm; ?>;</script>
<div id="maincontent">
    Select form:
    <br />
    <select id="formSelect">        
        <?php 
            foreach($forms as $form) {
                echo '<option value="' . $form['id'] . '">' . $form['title'] . '</option>';
            }
        ?>
    </select>
    <br /><br />

    Select category in form:<br />

    <select id="categorySelect"></select>

    <br /><br />    
    <input type="button" value="clear fields (back to add new)" id="clearBtn">

    <br /><br /><hr><br />

    Required:
    <table>
        <tr>
            <td>Id: </td>
            <td><input type="text" id="editId" readonly="readonly"></td>
        </tr>
        <tr>
            <td>Question: </td>
            <td><input type="text" id="editQuestion" style="width: 600px;"></td>
        </tr>
        <tr>
            <td>Type: </td>
            <td>
                <select id="editType">
                    <option value="textarea">Text area</option>
                    <option value="radio">Radio</option>
                </select>
            </td>
        </tr>
    </table>
    <br /><br />
    Optional: 
    <table>
        <tr>
            <td>Answers: </td>
            <td><input type="text" id="editAnswers" style="width: 600px;" placeholder="answers comma seperated (for example: yes,no,maybe)"></td>
        </tr>
        <tr>
            <td>Custom type:</td>
            <td>
                <select id="editCustomType">
                    <option value="">none</option>
                    <option value="grouped">Grouped</option>
                </select>
            </td>
        </tr>
        <tr>
            <td><input type="button" value="save" id="saveBtn"></td>
        </tr>
    </table>

    <br /><br /><hr><br />

    <table class="dataTable">
        <tr>
            <th>Id</th>
            <th>Question</th>
            <th>Type</th>
            <th>Answers</th>
            <th>Custom type</th>
            <th>Edit</th>
            <th>Delete</th>
        </tr>
    </table>
</div>
\$\endgroup\$
1
\$\begingroup\$

The main point of classes is to be able to create multiple instances independently from each other. So if you had the same "widget" multiple times on your page, they could work without needing to write duplicate code. The main problem is that you have things like IDs and URLs hard-coded into your class, and have relevant parts (namely the event handlers) outside of the class.

So, yes, you are missing the point.

If you want to encapsulate everything, you should drop the IDs and use classes (or data- attributes) and hang up everything from a surrounding element, e.g.:

<form class="questioneditor">
   <!-- put everything your widget need in here marked up with classes, not IDs -->
</form>

$(".questioneditor").each(function() {
   new Customerformquestionmanager(this);
});

There are several other (small) problems with your code that could be optimized. Here just a quick overview:

  • If you have your JSON server-side script return the correct mime-type "application/json" or at least set the option dataType: "json" in the ajax() call, then you don't need to parse the server response yourself. jQuery ajax() will do that for your.
  • <br> is for line breaks in things like poems and addresses, not for layout.
  • The way you generate the table rows (which is duplicated code BTW) and read the data from them into the input fields looks very dangerous. At the very worst you are opening yourself up to cross-site scripting attacks. At the very least it will probably lead to to headaches with (un)encoding HTML properly.

EDIT: Infos to cross-site scripting:

If your data contains any kind of HTML (especially including scripts), then just appending it like this '<td class="rowQuestion">' + data[i].question + '</td>' it won't be escaped and a forgotten <, or a stray </table> will break your layout. And if somehow an infinite loop such as <script>while(1){alert();}</script> get's in, your site will be unusable. At the worst AJAX script could be smuggled in and send personal data to a third party site.

Instead make sure to escape the data by creating proper text nodes. With jQuery, for example, by using the .text() method:

$("<td>").addClass("rowQuestion").text(data[i].question);
\$\endgroup\$
  • \$\begingroup\$ the row generation i fail to see how this open up cross-site scripting attacks, how would you go about generating those? \$\endgroup\$ – herriekrekel Nov 18 '15 at 15:57
  • \$\begingroup\$ @herriekrekel See edit. \$\endgroup\$ – RoToRa Nov 24 '15 at 9:00
  • \$\begingroup\$ i see your point, i was thinking it was pointless since i filter it on the server side but what your saying is basically what if it gets in there somehow anyway using the text instead of inside html would make the scripting less harmful. Thanks appreciate you taking the time to comment back on it \$\endgroup\$ – herriekrekel Nov 24 '15 at 9:24

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.