3
\$\begingroup\$

Problem statement

After 10 sec of execution of a program, start reading the property descriptors of each property of an object and continue reading all remaining properties each, every 3 seconds until done.

Solution

function Timer(obj, startTime, interval) {
    var PDO ={
        obj:{value: obj, enumerable: true, writable:false, configurable:false},
        startTime: {value:startTime, enumerable:true, writable:false, configurable:false},
        interval: {value:interval, enumerable:true, writable:false, configurable:false}
    };
    Object.defineProperties(this, PDO);
}

Timer.prototype.readProperties = function(){
    var props = Object.getOwnPropertyNames(this.obj);
    var self = this;
    function readPDO(){
        console.log(Object.getOwnPropertyDescriptor(self.obj, props[0]));
        props.shift();
        if (props.length === 0) {
            clearInterval(self.clearID);
        }
    }
    setTimeout(function(){
        self.clearID = setInterval(function(){
                                        readPDO();
                                    },
                                    self.interval
                                  );
                        },
                        self.startTime
              );
}

var t = new Timer({x:1, y:2}, 10000, 3000);
t.readProperties();

Can we improve the solution?

Are the naming conventions looking fine?

\$\endgroup\$
  • \$\begingroup\$ @BrunoCosta Do I need to override Object.prototype.watch? \$\endgroup\$ – overexchange Jan 15 '16 at 0:25
  • \$\begingroup\$ But watch conceptually work on a property state change. Where is the property state change in my code? \$\endgroup\$ – overexchange Jan 15 '16 at 0:41
  • \$\begingroup\$ @BrunoCosta This is another point. Can you accumulate all your comments as an answer? \$\endgroup\$ – overexchange Jan 15 '16 at 0:43
  • \$\begingroup\$ I now understand a little bit better what you are trying to do. While all of what I said may still apply I will see if I can give other kind of answer tomorrow ( if no one gets to it first that is) \$\endgroup\$ – Bruno Costa Jan 15 '16 at 1:18
3
\$\begingroup\$

Describe your problem better

While this may seem not be a thing to be pointing out in a code review it is important to describe your problem in a nice way. I would describe it as follows:

I want to yield each property of an object every three seconds. However the first one should be delayed by 10 seconds.

Separation of concerns

You are mixing the time repeating problem with the property yielding. You also don't allow a way to specify a different strategy for what to do whenever another property comes (in this case that would be the console.log).

function setIntervalDelayed(delay, interval, cb){
    setTimeout(function(){
        if(cb()){
            setIntervalDelayed(interval, interval, cb);
        }
    }, delay);
}

function yieldProperties(obj, delay, interval, cb){
    var props = Object.getOwnPropertyNames(obj);
    setIntervalDelayed(delay, interval, function(){
        if(props.length == 0) return false;
        cb(obj[props[0]]);
        props.shift();
        return props.length > 0;
    })  
}

yieldProperties({x:0, y:1, z:2}, 1000 * 10, 1000 * 3, console.log);

EDIT

While my approach is sligthly better then yours there is still mixed code. Again yieldProperties is doing more then it should, it still has that depedency of the delay and interval arguments, that don't really fit there. Solution: implement iterator pattern.

    function setIntervalDelayed(delay, interval, cb){
        setTimeout(function(){
            if(cb()){
                setIntervalDelayed(interval, interval, cb);
            }
        }, delay);
    }

    function iterate(arr, cb){
        var i = 0;
        return function(){
            if(i >= arr.length) return false;
            cb.apply(arr[i]);
            ++i;
            return true;
        };
    }

    function yieldProperties(obj, cb){
        return iterate(Object.getOwnPropertyNames(obj), function(){
            cb({
                name: this.toString(),
                value: obj[this]
            });
        });
    }

    setIntervalDelayed(10 * 1000, 3 * 1000, yieldProperties({x:0, y:1, z:2}, console.log.bind(console)))
\$\endgroup\$
  • \$\begingroup\$ From readability aspect, which code is better? \$\endgroup\$ – overexchange Jan 15 '16 at 13:10
1
\$\begingroup\$

I agree with Bruno's tips, here are some of my tips since you've shown interest in readability.

  1. Spacing Consistency. It's a nit-picky kind of tip, but it does help readability.

You have a lot of places where you have something like function() { and then another location with function(){. Again things like obj:{ then value: obj.

It takes a while to get used to it, but readability is greatly improved with consistency.

  1. Naming. Names should do more than be a unique identifier, they should tell you the purpose of the function/variable.

The name Timer tells me absolutely nothing about why it is taking an object and what it is doing. Yes, it tells me it is doing something with time, but what?

Try narrowing your names to something more specific (Yes, this can be very difficult at times, but 2+ weeks from now if you look at this code again it will be worth it). A suitable replacement for Timer might be DelayedPeriodicReader perhaps?

I don't claim to be the best at naming; the point is to describe your functions/variables as best as possible. If you're using generic names and the code isn't generic, you should look for a better name.

  1. Formatting. This could have been just because of copy/paste transition, but in case it wasn't, your setInterval area of your code is a painful eye-sore. Try something more attuned to this:

Note: Don't use this exact code, as it's been a while since I've done any javascript so I probably missed something. I just wanted you to get the idea, anonymous/inline functions can be great, but if over-used they quickly become hard to read.

var periodicPDORead = function() {
    self.clearID = setInterval(readPDO, self.interval);
};
setTimeout(periodicPDORead, self.startTime);

Hopefully these tips help you write more readable code in the future.

\$\endgroup\$
  • \$\begingroup\$ I have to say that I am amazed by myself it seems that I am so used to follow a certain space formatting for my code that, apparently, I am doing it subconsciously, at least to a certain extent of course I probably won't nail it over hundreds of lines of code. \$\endgroup\$ – Bruno Costa Jan 15 '16 at 14:38

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.