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.

We have a database with fixed width columns. The API is really simple and returns everything the database does with little to no actual processing. This results in some unnecessarily long strings in the model. The model has child objects who have grandchildren and can get quite large at times.

I've written this function to clean up a model right after it's received from the API and right before the rest of the client does anything with it:

function DeepTrim(obj) {
    for (var prop in obj) {
        if (obj.hasOwnProperty(prop)) {
            if (typeof obj[prop] == 'string') {
                obj[prop] = obj[prop].trim();
            } else if (typeof obj[prop] == 'object') {
                DeepTrim(obj[prop]);
            }
        }
    }
}

I know browsers don't really pay attention to consecutive spaces, but this is causing problems when we determine if a field has any valuable content. We can't simply check for a falsey from the string because being filled with spaces makes it just as true as it being filled with an address, phone numbers, or other meaningful data.

This seems to function properly for the few cases I've tried, but it's slow for large objects, especially in IE9 (the oldest browser we're supporting).

Performance is all I care about. It won't be running a thousand times a second, but it might get large, deep objects. Is there any way to speed this up?

share|improve this question
    
You could use a regular expression to see if the field only contains spaces /^\s*$/.test(' ') and wrap this within a isEmpty function or something descriptive of it's business purpose or you could simply lazily trim fields when needed rather than upfront. If Proxy would be supported, that would be quite simple, but you could also provide your view models with a get method which understands deep property lookups and trims the result before returning. –  plalx Aug 8 at 20:54
    
You could also use a reviver function when parsing your JSON. JSON.parse('{"a": " ", "b" : { "c": " ", "d": "test"}}', function (key, val) { return typeof val === 'string'? val.trim() : val; }); –  plalx Aug 8 at 21:05
    
Angular is parsing the response, so a reviver would be tricky to implement. –  Corey Ogburn Aug 8 at 21:07
    
can you show us how you are filling the json object. –  paritosh Aug 8 at 21:07
    
@CoreyOgburn I guess that means you are using $resource. Cant you be able to use a custom reviver by using config options such as transformResponse? –  plalx Aug 8 at 21:17

2 Answers 2

I see a bug for a null property case as typeof null == "object". You should check if it's not null or an object :)

Another less important? bug is you can't use the key hasOwnProeprty on any of your objects. You can get around this by using Object.prototype.hasOwnProperty.call(obj, key)

Only obvious optimization I see is to move the hasOwnProperty check after the typeof check. hasOwnProperty is a relatively expensive function so you will probably see some gains off that. I expect this should be even faster than doing Object.keys which is known to be faster than your for in has loop...

I assume you're polyfilling trim for older browsers right?

function DeepTrim(obj) {
    for (var prop in obj) {
        var value = obj[prop], type = typeof value;
        if (value != null && (type == "string" || type == "object") && obj.hasOwnProperty(prop)) {
            if (type == "object") {
                DeepTrim(obj[prop]);
            } else {
                obj[prop] = obj[prop].trim();
            }
        }
    }
}
share|improve this answer
    
I didn't think about null being an object. Good catch. I am curious as to why I can't expect my own objects to have hasOwnProperty, what's up with that? –  Corey Ogburn Aug 8 at 22:13
    
Consider deepTrim({hasOwnProperty: "things are about to get hairy"}) –  megawac Aug 8 at 23:46
    
@megawac » polyfilling trim for older browsers« the oldest browser he's using is IE9, which supports trim very well (developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/…) :] –  Thomas Junk Aug 9 at 2:55
    
Ah my mistake, don't have the kangax tables memorized xD. Thanks @ThomasJunk –  megawac Aug 9 at 17:56

I have significant concerns about some of your statements:

Performance is all I care about. It won't be running a thousand times a second, but it might get large, deep objects. Is there any way to speed this up

If this is true, you have to start from the source.

Database performance is almost entirely related to the amount of data that gets moved around, how much space you have to move it around in, and how fast that space is.... (memory being the fastest space)

If your data is more compact (i.e. you do not use fixed-length fields unnecessarily), then:

  • your queries run faster (less IO)
  • you can cache more data in memory
  • network transfers are faster

If you want the best performance, then change your database schema.

A second option is to trim the values on the database side before returning them to the client.... adding rtrim() to your SQL will remove the need for any handling on the client side.

What you are doing is the worst of the options.... handling the trim in the client, after all the hard work is being done by the network, database, and now the browser. Doing it right the first time would remove the need for anything to 'suffer'.

As for the JavaScript handling, it has issues too, it is processing values which have no need for processing, and it is introducing bugs too. You should probably inspect the metadata of the query, and identify fields that need processing, instead of checking each and every value. In other words, if the query was select name, id from users your code is checking every name, and every ID, and there could be many of them. Instead, you should check the name column, and identify it as String results, then check the ID column, and identify it as numberic. Then, you should only process values in the name fields, and not even check values in the ID.

Finally, what if white-space is important. What if the value in the table is... padded numbers, for example, or... a formatted field, like

   <indented>xml</indented>`

which has 4 leading significant white-spaces.... your code is introducing bugs.

The bottom line, is that you should only be storing significant data. It makes everything faster, smaller, and more efficient.

share|improve this answer
    
I think you read a little too much into the statement. I'm only talking about this function. I can't overhaul the entire system the way you're talking. The database isn't even mine and the Department of Education is not going to make changes on my request. I only want to quickly ready the view model before binding. This won't have the edge cases you're talking about either. –  Corey Ogburn Aug 10 at 7:21

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.