Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mci 0 will result a out-of-memory by div 10 always true #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dindinw
Copy link

@dindinw dindinw commented Aug 20, 2017

No description provided.

@tonyofbyteball
Copy link
Member

Can it be called with mci=0 at all?

@dindinw
Copy link
Author

dindinw commented Oct 4, 2017

not at the current stage, because mc=0 is no sense, but I think it maybe be better & safer for the method to check the input argument. anyway I result a Out-of-memory when I was doing my test.

@@ -916,6 +916,7 @@ function markMcIndexStable(conn, mci, onDone){
function getSimilarMcis(mci){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dindinw what do you think of rewriting the whole function as

 function getSimilarMcis(mci){
  	var arrSimilarMcis = [];
  	if (mci <= 0)
  	  	return arrSimilarMcis;
  	var divisor = 10;
  	while (mci % divisor === 0) {
  		arrSimilarMcis.push(mci - divisor);
 		divisor *= 10;
 	}
 	return arrSimilarMcis;
 }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your code is better 👍

Copy link
Author

@dindinw dindinw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eaglo changed code according to your suggestion.

@@ -916,6 +916,7 @@ function markMcIndexStable(conn, mci, onDone){
function getSimilarMcis(mci){
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your code is better 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants