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.

So I have a function which creates a Playlist object. Currently, this function accepts two paramaters: id and name. However, it also exposes a method called 'loadData' which can set the entire object after construction. I'm realizing this is a poor way to go about things and would like to modify my code such that the constructor accepts a config object.

I am wondering a couple of things:

  • How can I prevent a caller from extending my Playlist object with additional, unintended properties? Should I even worry about this?
  • I could use some advice on how to best consume my config object. Should I just called jQuery extend?

Here's what my code looks like before refactoring:

define(['yt_helper', 'song_builder'], function(ytHelper, songBuilder){
    //Maintains a list of song objects as an array and exposes methods to affect those objects to Player.
    return function(id, name) {
        "use strict";
        var playlist = {
            id: id ? id : Helpers.generateGuid(),
            title: name ? name : "New Playlist",
            selected: false,
            shuffledSongs: [],
            songHistory: [],
            relatedVideos: [],
            songs: []
        };

     return {
        loadData: function(data){
            playlist = data;
        }
     };
   };
};

//TODO: Just pass in the object and extend a playlist.
//Load default playlists.
var grizPlaylist = new playlistFunc();
grizPlaylist.loadData({
    id:"d6103404-1568-4d18-922b-058656302b22",
    title:"GRiZ",
    selected:true,
    shuffledSongs:[{"id":"4696c63f-8dae-4803-8e19-2b89c339f478","videoId":"CI-p4OkT3qE","url":"http://youtu.be/CI-p4OkT3qE","title":"Griz - Smash The Funk | Mad Liberation (2/12)","duration":"411"},{"id":"2ef29e04-d038-44f9-b3a4-ad163bbec459","videoId":"G5w7MIKwSO0","url":"http://youtu.be/G5w7MIKwSO0","title":"GRiZ - Vision of Happiness [HD]","duration":"198"},{"id":"27492b00-8230-48c4-90b9-237be3f07502","videoId":"xvtNnCs6EFY","url":"http://youtu.be/xvtNnCs6EFY","title":"GRiZ - Wheres The Love?","duration":"374"},{"id":"c278ea8e-6877-4658-a017-0a6ea5ccbff3","videoId":"0Gz96ACc45U","url":"http://youtu.be/0Gz96ACc45U","title":"Griz - Mr. B (feat. Dominic Lalli) | Mad Liberation (7/12)","duration":"349"}],
    songHistory:[],
    songs:[{"id":"a207502e-e68e-40e2-a5b1-a94e638a731b","videoId":"3AXu6l3GOYE","url":"http://youtu.be/3AXu6l3GOYE","title":"Griz - Blastaa | Mad Liberation (4/12)","duration":"269"},{"id":"4696c63f-8dae-4803-8e19-2b89c339f478","videoId":"CI-p4OkT3qE","url":"http://youtu.be/CI-p4OkT3qE","title":"Griz - Smash The Funk | Mad Liberation (2/12)","duration":"411"},{"id":"c278ea8e-6877-4658-a017-0a6ea5ccbff3","videoId":"0Gz96ACc45U","url":"http://youtu.be/0Gz96ACc45U","title":"Griz - Mr. B (feat. Dominic Lalli) | Mad Liberation (7/12)","duration":"349"},{"id":"27492b00-8230-48c4-90b9-237be3f07502","videoId":"xvtNnCs6EFY","url":"http://youtu.be/xvtNnCs6EFY","title":"GRiZ - Wheres The Love?","duration":"374"},{"id":"2ef29e04-d038-44f9-b3a4-ad163bbec459","videoId":"G5w7MIKwSO0","url":"http://youtu.be/G5w7MIKwSO0","title":"GRiZ - Vision of Happiness [HD]","duration":"198"}]
});

Here's how I think it should look afterward:

define(['yt_helper', 'song_builder'], function(ytHelper, songBuilder){
    //Maintains a list of song objects as an array and exposes methods to affect those objects to Player.
    return function(config) {
        "use strict";
        var playlist = $.extend({
            id: Helpers.generateGuid(),
            title: "New Playlist",
            selected: false,
            shuffledSongs: [],
            songHistory: [],
            relatedVideos: [],
            songs: []
        }, config);
    }
}

var grizPlaylist = new playlistFunc({
        id:"d6103404-1568-4d18-922b-058656302b22",
        title:"GRiZ",
        selected:true,
        shuffledSongs:[{"id":"4696c63f-8dae-4803-8e19-2b89c339f478","videoId":"CI-p4OkT3qE","url":"http://youtu.be/CI-p4OkT3qE","title":"Griz - Smash The Funk | Mad Liberation (2/12)","duration":"411"},{"id":"2ef29e04-d038-44f9-b3a4-ad163bbec459","videoId":"G5w7MIKwSO0","url":"http://youtu.be/G5w7MIKwSO0","title":"GRiZ - Vision of Happiness [HD]","duration":"198"},{"id":"27492b00-8230-48c4-90b9-237be3f07502","videoId":"xvtNnCs6EFY","url":"http://youtu.be/xvtNnCs6EFY","title":"GRiZ - Wheres The Love?","duration":"374"},{"id":"c278ea8e-6877-4658-a017-0a6ea5ccbff3","videoId":"0Gz96ACc45U","url":"http://youtu.be/0Gz96ACc45U","title":"Griz - Mr. B (feat. Dominic Lalli) | Mad Liberation (7/12)","duration":"349"}],
        songHistory:[],
        songs:[{"id":"a207502e-e68e-40e2-a5b1-a94e638a731b","videoId":"3AXu6l3GOYE","url":"http://youtu.be/3AXu6l3GOYE","title":"Griz - Blastaa | Mad Liberation (4/12)","duration":"269"},{"id":"4696c63f-8dae-4803-8e19-2b89c339f478","videoId":"CI-p4OkT3qE","url":"http://youtu.be/CI-p4OkT3qE","title":"Griz - Smash The Funk | Mad Liberation (2/12)","duration":"411"},{"id":"c278ea8e-6877-4658-a017-0a6ea5ccbff3","videoId":"0Gz96ACc45U","url":"http://youtu.be/0Gz96ACc45U","title":"Griz - Mr. B (feat. Dominic Lalli) | Mad Liberation (7/12)","duration":"349"},{"id":"27492b00-8230-48c4-90b9-237be3f07502","videoId":"xvtNnCs6EFY","url":"http://youtu.be/xvtNnCs6EFY","title":"GRiZ - Wheres The Love?","duration":"374"},{"id":"2ef29e04-d038-44f9-b3a4-ad163bbec459","videoId":"G5w7MIKwSO0","url":"http://youtu.be/G5w7MIKwSO0","title":"GRiZ - Vision of Happiness [HD]","duration":"198"}]
    });
share|improve this question
2  
Why is it important to prevent a caller from extending my Playlist object with additional, unintended properties? –  ANeves Nov 12 '12 at 12:29
add comment

2 Answers 2

up vote 2 down vote accepted

As for your question:

How can I prevent a caller from extending my Playlist object with additional, unintended properties? Should I even worry about this?

I wouldn't worry about this too much. If you don't want the API consumer to inject additional properties through config object, use the construct st-boost suggested. If you don't want the consumer to access the Playlist internals at all, contain it in a closure and only expose functions that do work on this object.

That said, however, JavaScript's properties as a language encourage a different mindset than the one you might have when working with languages that have extensive access control capabilities, like Java or C#. There, it is considered the API designer's responsibility to prevent any possible misuse of the API. In JavaScript, the mindset is more "junk in, junk out", and it's more of the caller's responsibility to ensure that he's not doing anything stupid, simply because JavaScript gives him more power to do stupid things.

Looking back at your problem, you might have grounds to worry if you're iterating over all properties of the Playlist object for some reason and your code might break if there are some unaccounted for properties. I don't see a case for something like that here, however.

share|improve this answer
add comment

You could use || instead of $.extend. eg:

id: config.id || Helpers.generateGuid(),

That has the benefit of not creating properties you don't use.

Also, did you strip out some code? Because it seems like all you're doing is storing a reference to an object. Why not just use the object?

share|improve this answer
add comment

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.