Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

I have to disagree with palacsint about using HashMap, if anything, I think that you should use a Set implementation such as HashSet. You're never using the value that is stored in the map, and you have no reason to use it either. Therefore, I would go with a Set. (The fact that a HashSet internally uses a HashMap is an entirely different subject) Palacsint is correct though that HashMap is a better alternative than Hashtable.


Reading lines like this makes me sea-sick:

if( (!myTable.containsKey(minVal-1)) && minVal-1>0){

Use spacing properly and remove unnecessary parameters and it becomes:

if(!myTable.containsKey(minVal - 1) && minVal - 1 > 0) {

Overall, I think that you shouldn't need to use Objects for this (except an array of course, which technically is an object). I would only use primitives and use more self-documenting variable names only use primitives and use more self-documenting variable names.

An unnecessary operation in your code is that on each iteration you check containsKey twice. Once to see if value - 1 is set and once to see if value + 1 is set. Why not just check if value itself is set?

Your code can be rewritten and optimized into this: (This is a fixed version of ChrisW's approach, I had started the approach when he posted his answer and then I did some modifications to make sure it matched your code).

public int firstMissingPositive(int[] array) {
 boolean[] foundValues = new boolean[array.length];
 for (int i : array) {
 if (i > 0 && i <= foundValues.length)
 foundValues[i - 1] = true;
 }
 for (int i = 0; i < foundValues.length; i++) {
 if (!foundValues[i]) {
 return i + 1;
 }
 }
 return foundValues.length + 1;
}

I have to disagree with palacsint about using HashMap, if anything, I think that you should use a Set implementation such as HashSet. You're never using the value that is stored in the map, and you have no reason to use it either. Therefore, I would go with a Set. (The fact that a HashSet internally uses a HashMap is an entirely different subject) Palacsint is correct though that HashMap is a better alternative than Hashtable.


Reading lines like this makes me sea-sick:

if( (!myTable.containsKey(minVal-1)) && minVal-1>0){

Use spacing properly and remove unnecessary parameters and it becomes:

if(!myTable.containsKey(minVal - 1) && minVal - 1 > 0) {

Overall, I think that you shouldn't need to use Objects for this (except an array of course, which technically is an object). I would only use primitives and use more self-documenting variable names.

An unnecessary operation in your code is that on each iteration you check containsKey twice. Once to see if value - 1 is set and once to see if value + 1 is set. Why not just check if value itself is set?

Your code can be rewritten and optimized into this: (This is a fixed version of ChrisW's approach, I had started the approach when he posted his answer and then I did some modifications to make sure it matched your code).

public int firstMissingPositive(int[] array) {
 boolean[] foundValues = new boolean[array.length];
 for (int i : array) {
 if (i > 0 && i <= foundValues.length)
 foundValues[i - 1] = true;
 }
 for (int i = 0; i < foundValues.length; i++) {
 if (!foundValues[i]) {
 return i + 1;
 }
 }
 return foundValues.length + 1;
}

I have to disagree with palacsint about using HashMap, if anything, I think that you should use a Set implementation such as HashSet. You're never using the value that is stored in the map, and you have no reason to use it either. Therefore, I would go with a Set. (The fact that a HashSet internally uses a HashMap is an entirely different subject) Palacsint is correct though that HashMap is a better alternative than Hashtable.


Reading lines like this makes me sea-sick:

if( (!myTable.containsKey(minVal-1)) && minVal-1>0){

Use spacing properly and remove unnecessary parameters and it becomes:

if(!myTable.containsKey(minVal - 1) && minVal - 1 > 0) {

Overall, I think that you shouldn't need to use Objects for this (except an array of course, which technically is an object). I would only use primitives and use more self-documenting variable names.

An unnecessary operation in your code is that on each iteration you check containsKey twice. Once to see if value - 1 is set and once to see if value + 1 is set. Why not just check if value itself is set?

Your code can be rewritten and optimized into this: (This is a fixed version of ChrisW's approach, I had started the approach when he posted his answer and then I did some modifications to make sure it matched your code).

public int firstMissingPositive(int[] array) {
 boolean[] foundValues = new boolean[array.length];
 for (int i : array) {
 if (i > 0 && i <= foundValues.length)
 foundValues[i - 1] = true;
 }
 for (int i = 0; i < foundValues.length; i++) {
 if (!foundValues[i]) {
 return i + 1;
 }
 }
 return foundValues.length + 1;
}
added 525 characters in body
Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311

I have to disagree with palacsint about using HashMap, if anything, I think that you should use a Set implementation such as HashSet. You're never using the value that is stored in the map, and you have no reason to use it either. Therefore, I would go with a Set. (The fact that a HashSet internally uses a HashMap is an entirely different subject) Palacsint is correct though that HashMap is a better alternative than Hashtable.


Reading lines like this makes me sea-sick:

if( (!myTable.containsKey(minVal-1)) && minVal-1>0){

Use spacing properly and remove unnecessary parameters and it becomes:

if(!myTable.containsKey(minVal - 1) && minVal - 1 > 0) {

Overall, I think that you shouldn't need to use Objects for this (except an array of course, which technically is an object). I would only use primitives and use more self-documenting variable names.

An unnecessary operation in your code is that on each iteration you check containsKey twice. Once to see if value - 1 is set and once to see if value + 1 is set. Why not just check if value itself is set?

Your code can be rewritten and optimized into this: (This is a fixed version of ChrisW's approach, I had started the approach when he posted his answer and then I did some modifications to make sure it matched your code).

public int firstMissingPositive(int[] array) {
 boolean[] foundValues = new boolean[array.length];
 for (int i : array) {
 if (i > 0 && i <= foundValues.length)
 foundValues[i - 1] = true;
 }
 for (int i = 0; i < foundValues.length; i++) {
 if (!foundValues[i]) {
 return i + 1;
 }
 }
 return foundValues.length + 1;
}

I have to disagree with palacsint about using HashMap, if anything, I think that you should use a Set implementation such as HashSet. You're never using the value that is stored in the map, and you have no reason to use it either. Therefore, I would go with a Set. (The fact that a HashSet internally uses a HashMap is an entirely different subject) Palacsint is correct though that HashMap is a better alternative than Hashtable.


Reading lines like this makes me sea-sick:

if( (!myTable.containsKey(minVal-1)) && minVal-1>0){

Use spacing properly and remove unnecessary parameters and it becomes:

if(!myTable.containsKey(minVal - 1) && minVal - 1 > 0) {

Overall, I think that you shouldn't need to use Objects for this (except an array of course, which technically is an object). I would only use primitives and use more self-documenting variable names.

I have to disagree with palacsint about using HashMap, if anything, I think that you should use a Set implementation such as HashSet. You're never using the value that is stored in the map, and you have no reason to use it either. Therefore, I would go with a Set. (The fact that a HashSet internally uses a HashMap is an entirely different subject) Palacsint is correct though that HashMap is a better alternative than Hashtable.


Reading lines like this makes me sea-sick:

if( (!myTable.containsKey(minVal-1)) && minVal-1>0){

Use spacing properly and remove unnecessary parameters and it becomes:

if(!myTable.containsKey(minVal - 1) && minVal - 1 > 0) {

Overall, I think that you shouldn't need to use Objects for this (except an array of course, which technically is an object). I would only use primitives and use more self-documenting variable names.

An unnecessary operation in your code is that on each iteration you check containsKey twice. Once to see if value - 1 is set and once to see if value + 1 is set. Why not just check if value itself is set?

Your code can be rewritten and optimized into this: (This is a fixed version of ChrisW's approach, I had started the approach when he posted his answer and then I did some modifications to make sure it matched your code).

public int firstMissingPositive(int[] array) {
 boolean[] foundValues = new boolean[array.length];
 for (int i : array) {
 if (i > 0 && i <= foundValues.length)
 foundValues[i - 1] = true;
 }
 for (int i = 0; i < foundValues.length; i++) {
 if (!foundValues[i]) {
 return i + 1;
 }
 }
 return foundValues.length + 1;
}
deleted 65 characters in body
Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311

I have to disagree with palacsint about using HashMap, if anything, I think that you should use a Set implementation such as HashSet. You're never using the value that is stored in the map, and you have no reason to use it either. Therefore, I would go with a Set. (The fact that a HashSet internally uses a HashMap is an entirely different subject) Palacsint is correct though that HashMap is a better alternative than Hashtable.

(Palacsint's answer seems to have been deleted now though...)


Reading lines like this makes me sea-sick:

if( (!myTable.containsKey(minVal-1)) && minVal-1>0){

Use spacing properly and remove unnecessary parameters and it becomes:

if(!myTable.containsKey(minVal - 1) && minVal - 1 > 0) {

Overall, I think that you shouldn't need to use Objects for this (except an array of course, which technically is an object). I would only use primitives and use more self-documenting variable names.

I have to disagree with palacsint about using HashMap, if anything, I think that you should use a Set implementation such as HashSet. You're never using the value that is stored in the map, and you have no reason to use it either. Therefore, I would go with a Set. (The fact that a HashSet internally uses a HashMap is an entirely different subject) Palacsint is correct though that HashMap is a better alternative than Hashtable.

(Palacsint's answer seems to have been deleted now though...)


Reading lines like this makes me sea-sick:

if( (!myTable.containsKey(minVal-1)) && minVal-1>0){

Use spacing properly and remove unnecessary parameters and it becomes:

if(!myTable.containsKey(minVal - 1) && minVal - 1 > 0) {

Overall, I think that you shouldn't need to use Objects for this (except an array of course, which technically is an object). I would only use primitives and use more self-documenting variable names.

I have to disagree with palacsint about using HashMap, if anything, I think that you should use a Set implementation such as HashSet. You're never using the value that is stored in the map, and you have no reason to use it either. Therefore, I would go with a Set. (The fact that a HashSet internally uses a HashMap is an entirely different subject) Palacsint is correct though that HashMap is a better alternative than Hashtable.


Reading lines like this makes me sea-sick:

if( (!myTable.containsKey(minVal-1)) && minVal-1>0){

Use spacing properly and remove unnecessary parameters and it becomes:

if(!myTable.containsKey(minVal - 1) && minVal - 1 > 0) {

Overall, I think that you shouldn't need to use Objects for this (except an array of course, which technically is an object). I would only use primitives and use more self-documenting variable names.

Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311
Loading
lang-java

AltStyle によって変換されたページ (->オリジナル) /