Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Honestly, there's nothing wrong with this code. It's quite good how it is. There are a few minor things you could add, but if this came by me in a code review at work, I probably wouldn't have much to say. So here's what I can say:

Use const for Things that Won't Change

#Use const for Things that Won't Change You'reYou're already using constexpr for the constant in binarySearch(), but you also aren't going to modify either the needle or values. I would make them const to show the intent of their use in the function. I would also make values a const reference because it could potentially be copied and if it's large, that will be slow.

Do All the Calculation in a Function

#Do All the Calculation in a Function YourYour middleOf() function is named as if it should calculate the middle of the range that's passed to it. But it doesn't. You need to add position to the result returned from middleOf(). You should simply return the right value so the called doesn't need to add anything to it. I would define it like this:

unsigned int middleOf(const position, const int leftBound, const int rightBound) {
 return position + (rightBound - leftBound + 1) / 2;
}

Then it can be called like this:

position = middleOf(position, leftBound, rightBound);

You could also declare it constexpr.

Naming

#Naming OverallOverall your naming is pretty good. However, the meaning of the name needle is unclear. Is it a "needle in a haystack"? If so that's a bit of a stretch. I'd just name it something clear like toFind or searchValue.

Honestly, there's nothing wrong with this code. It's quite good how it is. There are a few minor things you could add, but if this came by me in a code review at work, I probably wouldn't have much to say. So here's what I can say:

#Use const for Things that Won't Change You're already using constexpr for the constant in binarySearch(), but you also aren't going to modify either the needle or values. I would make them const to show the intent of their use in the function. I would also make values a const reference because it could potentially be copied and if it's large, that will be slow.

#Do All the Calculation in a Function Your middleOf() function is named as if it should calculate the middle of the range that's passed to it. But it doesn't. You need to add position to the result returned from middleOf(). You should simply return the right value so the called doesn't need to add anything to it. I would define it like this:

unsigned int middleOf(const position, const int leftBound, const int rightBound) {
 return position + (rightBound - leftBound + 1) / 2;
}

Then it can be called like this:

position = middleOf(position, leftBound, rightBound);

You could also declare it constexpr.

#Naming Overall your naming is pretty good. However, the meaning of the name needle is unclear. Is it a "needle in a haystack"? If so that's a bit of a stretch. I'd just name it something clear like toFind or searchValue.

Honestly, there's nothing wrong with this code. It's quite good how it is. There are a few minor things you could add, but if this came by me in a code review at work, I probably wouldn't have much to say. So here's what I can say:

Use const for Things that Won't Change

You're already using constexpr for the constant in binarySearch(), but you also aren't going to modify either the needle or values. I would make them const to show the intent of their use in the function. I would also make values a const reference because it could potentially be copied and if it's large, that will be slow.

Do All the Calculation in a Function

Your middleOf() function is named as if it should calculate the middle of the range that's passed to it. But it doesn't. You need to add position to the result returned from middleOf(). You should simply return the right value so the called doesn't need to add anything to it. I would define it like this:

unsigned int middleOf(const position, const int leftBound, const int rightBound) {
 return position + (rightBound - leftBound + 1) / 2;
}

Then it can be called like this:

position = middleOf(position, leftBound, rightBound);

You could also declare it constexpr.

Naming

Overall your naming is pretty good. However, the meaning of the name needle is unclear. Is it a "needle in a haystack"? If so that's a bit of a stretch. I'd just name it something clear like toFind or searchValue.

Fixed middleOf() function to not overflow
Source Link
user1118321
  • 11.9k
  • 1
  • 20
  • 46

Honestly, there's nothing wrong with this code. It's quite good how it is. There are a few minor things you could add, but if this came by me in a code review at work, I probably wouldn't have much to say. So here's what I can say:

#Use const for Things that Won't Change You're already using constexpr for the constant in binarySearch(), but you also aren't going to modify either the needle or values. I would make them const to show the intent of their use in the function. I would also make values a const reference because it could potentially be copied and if it's large, that will be slow.

#Do All the Calculation in a Function Your middleOf() function is named as if it should calculate the middle of the range that's passed to it. But it doesn't. You need to add position to the result returned from middleOf(). You should simply return the right value so the called doesn't need to add anything to it. I would define it like this:

unsigned int middleOf(const position, const int leftBound, const int rightBound) {
 return position + (rightBound +- leftBound + 1) / 2;
}

Then it can be called like this:

position = middleOf(position, leftBound, rightBound);

You could also declare it constexpr.

#Naming Overall your naming is pretty good. However, the meaning of the name needle is unclear. Is it a "needle in a haystack"? If so that's a bit of a stretch. I'd just name it something clear like toFind or searchValue.

Honestly, there's nothing wrong with this code. It's quite good how it is. There are a few minor things you could add, but if this came by me in a code review at work, I probably wouldn't have much to say. So here's what I can say:

#Use const for Things that Won't Change You're already using constexpr for the constant in binarySearch(), but you also aren't going to modify either the needle or values. I would make them const to show the intent of their use in the function. I would also make values a const reference because it could potentially be copied and if it's large, that will be slow.

#Do All the Calculation in a Function Your middleOf() function is named as if it should calculate the middle of the range that's passed to it. But it doesn't. You need to add position to the result returned from middleOf(). You should simply return the right value so the called doesn't need to add anything to it. I would define it like this:

unsigned int middleOf(const int leftBound, const int rightBound) {
 return (rightBound + leftBound + 1) / 2;
}

Then it can be called like this:

position = middleOf(leftBound, rightBound);

You could also declare it constexpr.

#Naming Overall your naming is pretty good. However, the meaning of the name needle is unclear. Is it a "needle in a haystack"? If so that's a bit of a stretch. I'd just name it something clear like toFind or searchValue.

Honestly, there's nothing wrong with this code. It's quite good how it is. There are a few minor things you could add, but if this came by me in a code review at work, I probably wouldn't have much to say. So here's what I can say:

#Use const for Things that Won't Change You're already using constexpr for the constant in binarySearch(), but you also aren't going to modify either the needle or values. I would make them const to show the intent of their use in the function. I would also make values a const reference because it could potentially be copied and if it's large, that will be slow.

#Do All the Calculation in a Function Your middleOf() function is named as if it should calculate the middle of the range that's passed to it. But it doesn't. You need to add position to the result returned from middleOf(). You should simply return the right value so the called doesn't need to add anything to it. I would define it like this:

unsigned int middleOf(const position, const int leftBound, const int rightBound) {
 return position + (rightBound - leftBound + 1) / 2;
}

Then it can be called like this:

position = middleOf(position, leftBound, rightBound);

You could also declare it constexpr.

#Naming Overall your naming is pretty good. However, the meaning of the name needle is unclear. Is it a "needle in a haystack"? If so that's a bit of a stretch. I'd just name it something clear like toFind or searchValue.

Source Link
user1118321
  • 11.9k
  • 1
  • 20
  • 46

Honestly, there's nothing wrong with this code. It's quite good how it is. There are a few minor things you could add, but if this came by me in a code review at work, I probably wouldn't have much to say. So here's what I can say:

#Use const for Things that Won't Change You're already using constexpr for the constant in binarySearch(), but you also aren't going to modify either the needle or values. I would make them const to show the intent of their use in the function. I would also make values a const reference because it could potentially be copied and if it's large, that will be slow.

#Do All the Calculation in a Function Your middleOf() function is named as if it should calculate the middle of the range that's passed to it. But it doesn't. You need to add position to the result returned from middleOf(). You should simply return the right value so the called doesn't need to add anything to it. I would define it like this:

unsigned int middleOf(const int leftBound, const int rightBound) {
 return (rightBound + leftBound + 1) / 2;
}

Then it can be called like this:

position = middleOf(leftBound, rightBound);

You could also declare it constexpr.

#Naming Overall your naming is pretty good. However, the meaning of the name needle is unclear. Is it a "needle in a haystack"? If so that's a bit of a stretch. I'd just name it something clear like toFind or searchValue.

lang-cpp

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