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
.
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
.
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
.