0

I'm a little new to Javascript. I'm doing on a project for work and I'm having some trouble getting a method to return a percentage.

function campaignProgress(goal, current){
    this.goal = goal;
    this.current = current; 
    this.percent =  Math.floor(current / goal  * 100);

    this.getGoal = function(){
        return goal;
    }
    this.getCurrent = function(){
        return current;
    }
    this.getPercent = function(){   
        return percent;
    }
}


var totalProgress = new campaignProgress(1.70, 1.064);

When I call it in the html file I reference the .js file in my header and in the body I use;

<script type="text/javascript">

        document.write(totalProgress.getGoal());

        document.write(totalProgress.getPercent());

</script>

The getGoal() method works fine but getPercent() returns nothing. I can reference the percent variable itself like this;

totalProgress.percent

and it'll print fine. Any advice on why this is not working would be appreciated, thanks.

Improve this question
2
  • any console messages you are getting? Commented Jul 22, 2013 at 20:50
  • 1
    percent is undefined, you want this.percent Commented Jul 22, 2013 at 20:50

5 Answers 5

Reset to default
4

You need to return through the scope of the instance this:

this.getGoal = function(){
    return this.goal;
}
this.getCurrent = function(){
    return this.current;
}
this.getPercent = function(){   
    return this.percent;
}

goal and current are returned because of closure with the constructor parameters. But if they are changed after the constructor runs then they would return wrong values. This is apparent with the percent variable.

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

6 Comments

@dandavis No, they are properties of the instance (totalProgress), not local variables.
@dandavis But then what if we want to access them through this?
@KonstantinD-Infragistics That worked, thanks. Your explanation makes perfect sense looking at it now.
@KonstantinD-Infragistics I voted you down, not dandavis. Because this is unnecessary to do. And the value of this isn't guaranteed to be what you expect inside those functions anyways. I doubt this is what the OP (or anyone) wants. Why would you declare a "public" property, and then "public" methods just to get them?
@KonstantinD-Infragistics: i've never voted anybody anything. look at my badges. i was talking about the OP's code where this.current is not needed...
|
2

It looks like you're attempting to mimic a class, where, those should be "private". I think you'd want:

function campaignProgress(goal, current){
    var privateGoal = goal,
        privateCurrent = current;

    this.getGoal = function(){
        return privateGoal;
    };
    this.setGoal = function(g){
        privateGoal = g;
    };
    this.getCurrent = function(){
        return privateCurrent;
    };
    this.setCurrent = function(c){
        privateCurrent = c;
    };
    this.getPercent = function(){   
        return Math.floor(privateCurrent / privateGoal  * 100);
    };
}

var tp = new campaignProgress(1.70, 1.064);
console.log(tp.getGoal(), tp.getCurrent(), tp.getPercent());
tp.setCurrent(1.111);
console.log(tp.getGoal(), tp.getCurrent(), tp.getPercent());

DEMO: http://jsfiddle.net/twVNN/2/

This causes privateGoal and privateCurrent to be "private", meaning they can't be accessed outside their scope. The methods provided allow that access by calling them instead. Using this.goal = goal; is unnecessary if you're going to use something like getGoal. The privatePercent dynamically calculates the percent based on the values of privateCurrent and privateGoal.

Improve this answer

12 Comments

that breaks the named arguments.
@dandavis What are you talking about?
you should not re-declare (var ) a previously declared variable, in this case goal and current.
@dandavis Instead of arguing, I'll just change and accept your suggestion. It didn't hurt to have before, but it was unnecessary
it's just poor form, even if it worked, and there are some old browser fringe-case instances where the hoisting pattern can cause a problem when formals and lexical overlap.
|
2

You are assigning your variables as properties of the function (this.goal), but when you retrieve them you are trying to get local variables. This should fix it:

function campaignProgress(goal, current){
    this.goal = goal;
    this.current = current; 
    this.percent =  Math.floor(current / goal  * 100);

    this.getGoal = function(){
        return this.goal;
    }
    this.getCurrent = function(){
        return this.current;
    }
    this.getPercent = function(){   
        return this.percent;
    }
}

The other question here is are you using new to create an instance of this "function class"? Otherwise assigning this.goal will be assigning those variables to global scope.

var c = campaignProgress(1, 3);
console.log(c);//undefined
console.log(window.goal);//1

vs

var c = new campaignProgress(1, 3)
console.log(c);//instance
console.log(window.goal);//undefined
Improve this answer

2 Comments

Using this is preferable since they are being set as properties. This means they may be altered elsewhere. By just accessing the local vars they will remain unaware of changes to the properties.
since OP is using getters, it doesn't much matter.
2

You nee to use closures var that = this;

function campaignProgress(goal, current) {
    this.goal = goal;
    this.current = current; 
    this.percent =  Math.floor(current / goal  * 100);

    var that = this;
    this.getGoal = function() {
        return that.goal;
    }
    this.getCurrent = function(){
        return that.current;
    }
    this.getPercent = function(){   
        return that.percent;
    }
}


var totalProgress = new campaignProgress(1.70, 1.064);
console.log(totalProgress.getGoal());
console.log(totalProgress.getPercent());

this always is the value inside a function(), if you where to call this.goal (as some mentioned above), that's the same as simply calling goal

Improve this answer

4 Comments

@Teemu You don't need to use that here, but this isn't guaranteed to be what you expect in those methods. What if you did var cp = new campaignProgress(1, 2), method = cp.getGoal; alert(method());? This answer fixes that
@Teemu what i mean by closures here (maybe not correct term) is that this inside a function references what comes between the opening and closing bracket of that particular function.
@Teemu Ahh I see, I'm not sure if I missed that or what :)
Well, return this.XXXX; works only inside the constructor. I just realized what @Ian just has commented. When taken out of the context, we'll get undefined. Hence, it's always safest to set var that = this and use that in inner functions.
0

Goal works because goal is defined within each function's closure (the goal argument that's passed into the function). The others don't work because they're not referenced with they keyword this (which is not nearly as optional as some languages). Unfortunately, due to the way this works, we can't simply add this to the return statement in each of the sub-functions (since we're not working on a prototype, the value of this could change depending on where we're calling these functions from). Thus, use the changes in bold.

function campaignProgress(goal, current){
    var self = this;
    this.goal = goal;
    this.current = current; 
    this.percent =  Math.floor(current / goal  * 100);

    this.getGoal = function(){
        return self.goal;
    }
    this.getCurrent = function(){
        return self.current;
    }
    this.getPercent = function(){   
        return self.percent;
    }
}

By using the self variable, we capture the value we want this to be when we first call the function, and then we operate on that.

Improve this answer

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.