1

I came across an interesting challenge, I have the following code:

Sm.screenfragment(function (screen, viewModel) {
    //This can grow very quickly and turn into a mess
    var attribA = screen.get('title'),
        ... 
        ... 
        attribZ = screen.get('status');

    var setAttribA = function (container) {
        //Do Stuff
    };
    ...
    ...
    var setAttribZ = function(event, viewName) {
        //Do Stuff
    };

    //So this can grow hundreads of lines and get messy.
    return {
        model: {
            //Do Stuff
        },
        create: function () {
            //Do Stuff
        },
        prepare: function (callback, config) {
            //Do Stuff
        },
        enter: function () {
            //Do Stuff
        },
        exit: function (callback) {
            //Do Stuff
        }
    };
});

I have tried a few ideas, but they are little more than messing with the syntax:

I thought about adding a new util object, I can construct util objects, so it just adds a little structure, but not much more.

Sm.screenfragment(function (screen, viewModel) {
    //Still can grow into a mess
    var util = {
            attribA : screen.get('title'),
            attribB : screen.get('status'),
            setAttribA : function (container) {
            //Do Stuff
            },
            setAttribB : function(event, viewName) {
            //Do Stuff
            }   
    };

    return {
        model: {
            //Do Stuff
        },
        create: function () {
            //Do Stuff
            util.setAttribA...
        },
        prepare: function (callback, config) {
            //Do Stuff
        },
        enter: function () {
            //Do Stuff
        },
        exit: function (callback) {
            //Do Stuff
        }
    };
});

Then use dot notation to get the attributes, but this does not make the mess go away. I am re-reading stuff on the Module Pattern to see if I can apply something here, I have extreme cases where I can have dozens of proprieties and functions on the top of the file, and it just breaks the structure. How can I arrange the code in a more modular way? So that it does not get cluttered.

7
  • 1
    your codes seems okay without messing, what's the problem? Commented Aug 3, 2014 at 20:10
  • There can be dozens of lines inside of that... so it can look very messy, the goal is to make it more modular, currently I can take the AngularJS approach and pass context objects but a different approach would be better that is what I am looking for, a way to make more modular. Commented Aug 3, 2014 at 20:14
  • 1
    would you like to split this big file into two separate JS file somethings like main.js and util.js ? Commented Aug 3, 2014 at 20:24
  • Does your editor/IDE support code folding and displays the list of functions? Commented Aug 3, 2014 at 20:29
  • 1
    Better posted to codereview.stackexchange.com Commented Aug 5, 2014 at 2:38

5 Answers 5

1

Your question can't be answered that easy, because some information is missing. Which methods use which of the attributes? It's a problem which should be solved when analyzing the problem (not in the design, and definitely not in the code). So which part of your code do you whant to decouple from which other part?

To answer the question on a more general level:

It's regarded good Object Oriented Design (OOD), if you reach a high cohesion inside one module and low coupling between modules. In your case this means: if all your methods reference all your attributes, then keeping it all in one big file is regarded good OOD. But normally the real world problems aren't wired in such a monolithic way.

If you want to decouple something, there is the Single responsibility principle stating, that you should decouple the parts that don't interact with each other. In your case you could (maybe) put everything regarding attribA into one module and anything regarding attribB into another module. Thus it won't get messed up, regardless of the concrete module implementation you use.

Sign up to request clarification or add additional context in comments.

3 Comments

The attributes are essentially selectors to parts of a screen and utility methods that manipulate those methods. Everything that is inside the screen fragment function is well a screen fragment and its parts it can be a table, it can be link, images, etc...
The anwser is still the same: devide the module by it's attributes. for exmaple use a titleFragement and a statusFragment, each containing only one part of the screenFragment code. Then call all of them from a single point.
A screen fragment is already a division of the screen, hence the name screen fragment. But I will try to see if its possible to create a smaller division.
1

Well, the approach I take at least in terms of readability is to pull the function declarations out of the return block:

Sm.screenfragment(function (screen, viewModel) {
    //Still can grow into a mess
    var util = {
            attribA : screen.get('title'),
            attribB : screen.get('status'),
            setAttribA : function (container) {
            //Do Stuff
            },
            setAttribB : function(event, viewName) {
            //Do Stuff
            }   
    };

    var create = function() {
        //Do Stuff
        util.setAttribA...
    };

    var prepare = function(callback, config) {
        //Do Stuff
    };

    var enter = function() {
        //Do Stuff
    };

    var exit = function(callback) {
        //Do Stuff
    };

    return {
        model: {
            //Do Stuff
        },
        create: create,
        prepare: prepare,
        enter: enter,
        exit: exit
    };
});

And then if the code within those functions is generic/modular, pull that code out into a utility file and call them from there

Comments

1

I agree with @Waog that good OOD is probably the solution to your problem. Given that you might have a lot of properties on the object that you're writing setAttribA functions for, why not use a map to clean up the code a bit? There are probably dozens of map implementations for JavaScript, a simple google search for "map polyfill" lead me to this one, which looks nice: http://eriwen.github.io/smap.js/

Comments

1

Seems like there's opportunity to use an OO approach to encapsulate similar functionality. Each class can live in its own file... This seems nice:

// In file attrs.js
function ScreenAttrs(screen) {
    this.screen = screen;
    this.attribA = screen.get('title');
    // ...
    // ... could even categorize attributes into separate methods
    // this.initFooAttrs();
    // this.initBarAttrs();
};

// In file modifier.js
var attrs = new ScreenAttrs(screen);

function AttribModifier(attributes) {
    this.attrs = attributes;
};

AttribModifier.prototype.setAttribA = function() {
    // .. do stuff w/ this.attrs.attribA
};
// ... etc.

// in api.js
var modifer = new AttribModifier(attrs);

function ScreenAPIImpl (modifier) {
    this.modifier = modifier;
};

ScreenAPIImpl.proxy = function(context, method) {
    return function() {
        return method.apply( context, args.concat( slice.call( arguments ) ) );
    };
};

ScreenAPIImpl.prototype.model = function (foo) {
    // operation with this.modifier.setAttribA
    // operation with this.modifier.attrs.attribA
};

ScreenAPIImpl.prototype.fetchAPI = function (screen, viewModel) {
    return {
        // Bind context of method to this object instance
        model: this.proxy(this, this.model),
        // ...
    };
};

// .. etc

var api = new ScreenAPIImpl(modifier);

Sm.screenfragment(api.fetchAPI(screen, viewModel));

This also opens itself up to creating a helper builder class that constructs everything and returns the final API object:

var api = CreateAPI(screen);

Comments

1

Let's consider the following excerpt from your code, where I understand the issue lies:

Sm.screenfragment(function (screen, viewModel) {
    //This can grow very quickly and turn into a mess
    var attribA = screen.get('title'),
    ... 
    ... 
    attribZ = screen.get('status');

    var setAttribA = function (container) {
        //Do Stuff
    };
    ...
    ...
    var setAttribZ = function(event, viewName) {
        //Do Stuff
    };

    ...

As far as I can see what you're doing, in my opinion, it's unnecessary to define properties attribA up to attribZ and then setting them, and then again define setter functions for them. You could simply return or access screen.get('x') where and when it is needed.

But if it's absolutely necessary, for some reasons, then the following strategy popularised by jQuery may suffice:

attributeX = function(x, container){
    // if x and container are undefined, then we can assume API caller intends to
    // read value of property 'x' otherwise, caller intends to write to the property.
    if(container){
        // don't forget to do stuff before setting!
        container.setProp(x); // you get the idea
    } else {
        // well we must have a 'container' to set a prop,
        // if we don't then caller must want to read.
        return screen.get(x);
    }
}

If this strategy doesn't mitigate the issue or if you think I didn't understand question properly, then please do attempt to make the case clearer and get us closer to the aim.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.