Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

What's "string math" and why is it bad?

Tags:

javascript

I was recently berated by a fellow developer for using "string math" in an app I wrote. I'm pretty new to the whole development thing, with no formal training, and I haven't heard of this issue. What is it?

Code in question:

$('.submit-input').click( function() {
    var valid = true;
    $('input, select, radio').removeClass('error');
    $('.error-message').hide();

    $('.validate').each( function() {
        if($(this).val() == $(this).attr('default')){
            valid = false;
            $(this).addClass('error');
        }
    });

    if(!$('select[name="contact"] option:selected').val() != ''){
        $('select[name="contact"]').addClass('error');
        valid = false;
    }

    if(!$('input[name="ampm"]:checked').length){
        $('input[name="ampm"]').addClass('error');          
        valid = false;
    }

    if(!valid){
        $('.error-message').css('display','block');
        return false;
    } else {

        var services_selected = 'Services Selected: ';
        services_selected += $('.l3').text() + ', ' + $('.l4').text() + ', ' + $('.l5').text() + '; ' + $('.l6').text();
        var prices = 'Prices: ';
        prices += $('.l7').text() + ', ' + $('.l8').text() + ', ' + $('.l9').text() + ', ' + $('.l10').text();
        var name = 'Name: ';
        name += $('input[name="name"]').val();  
        var phone = 'Phone: ' 
        phone += $('input[name="phone"]').val();
        var time = 'Preferred contact time: ';
        time += $('select[name="contact"] option:selected').val() + $('input[name="ampm"]:checked').val();

        $.ajax({
            url: 'php/mailer.php',
            data: 'services_selected=' + services_selected +'&prices=' + prices + '&name=' + name + '&phone=' + phone + '&time=' + time,
            type: "POST",
            success: function() {
                $('#email_form_box .container').children().fadeOut(500, function() {
                    $('#email_form_box .container').html('<div style="margin:20px auto;text-align:center;width:200px;">yada yada yada<br /><span class="close">Close</span></div>');
                });
            }
        });
    }

});

Edit: The gist I'm getting here is that this isn't a standard development colloquialism, and I should probably talk to the guy who gave me guff in the first place. So I'll do that. Thanks guys. I'll be back with an answer, or to check off whoever knew already.

like image 869
dclowd9901 Avatar asked Aug 03 '10 14:08

dclowd9901


2 Answers

In most Javascript browser implementations, concatenating strings is slow due to excessive copying. See JavaScript: String Concatenation slow performance? Array.join('')?

Preferred method is to use an array and join:

var pieces = ["You purchased "];
pieces.push(num, " widgets.");
el.innerHTML = pieces.join('');

Added more:

I think you may have a lurking bug in your code: you don't appear to be escaping your data values. If any of them include an ampersand, you'd be in trouble. Use escape() for all of your data values.

ps. And this is a real bug that the other developer missed. The string math issue is a performance / maintainability issue.

Added:

I rewrote your email composition section (quickly). I think it's cleaner (and will be slightly faster) when using a piece array.

....
} else {

var d = []; // the post_data pieces table

d.push ('services_selected='); // Start the services_selected value
d.push ('Services Selected: ');
d.push ($('.l3').text(), ', ', $('.l4').text(), ', ', $('.l5').text(),
        '; ', $('.l6').text());

d.push ('&prices='); // Start the prices value
d.push ('Prices: ');
d.push ($('.l7').text(), ', ', $('.l8').text(), ', ', $('.l9').text(),
        ', ', $('.l10').text());

d.push ('&name='); // Start the name value
d.push ('Name: ', $('input[name="name"]').val());

d.push ('&phone='); // Start the phone value
d.push ('Phone: ', $('input[name="phone"]').val());

d.push ('&time='); // Start the timevalue
d.push ('Preferred contact time: ',
        $('select[name="contact"] option:selected').val(),
        $('input[name="ampm"]:checked').val());

    $.ajax({
        url: 'php/mailer.php',
        data: d.join(''),
        type: "POST",
        success: function() {
            $('#email_form_box .container').children().fadeOut(500, function() {
                $('#email_form_box .container').html('<div style="margin:20px auto;text-align:center;width:200px;">yada yada yada<br /><span class="close">Close</span></div>');
            });
        }
    });
}
like image 170
Larry K Avatar answered Sep 30 '22 12:09

Larry K


Okay, so here's the answer he told me:

I should have said inline string concatenation/parsing, which is a potential injection vulnerability and a sign of sloppy code or bypassing the framework.

Which doesn't exactly fit the other answers we have here. I'm going to give the check to the answer with the most upvotes, as it's probably the most useful, but just wanted to inform.

like image 29
dclowd9901 Avatar answered Sep 30 '22 12:09

dclowd9901