Recipe: Reversing an Array has incorrect algorithm

[pjbgtnj]

The last example - reversing an array by making a new copy doesn't work if the length of the array is an old number of elements: I'd suggest adding:

if len(numbers) & 1 == 1 {  // if odd
    halfWay := len(numbers) >> 1 // divide by 2 
    newNumbers[halfWay] = numbers[halfWay]
}

or better yet:

func reverse(numbers []int) []int {
    newNumbers := make([]int, 0, len(numbers))
    for i := len(numbers)-1; i >= 0; i-- {
        newNumbers = append(newNumbers, numbers[i])
    }
    return newNumbers
}

[tammersaleh]

Thanks @pjbgtnj! Working on a test for this now.

[tammersaleh]

Hey @pjbgtnj - I think you might be incorrect. The existing tests run this code against an odd number of elements and they pass:

golangcookbook.github.io/chapters/arrays/reverse/test_copy.go

Lines 5 to 18 in c93e83a

	func reverse(numbers []int) []int {
	newNumbers := make([]int, len(numbers))
	for i, j := 0, len(numbers)-1; i < j; i, j = i+1, j-1 {
	newNumbers[i], newNumbers[j] = numbers[j], numbers[i]
	}
	return newNumbers
	}
	func main() {
	array := []int{1, 2, 3, 4, 5}
	fmt.Printf("%v\n", reverse(array))
	fmt.Printf("%v\n", array)
	}

Could you possibly attempt a PR where you show this code failing?

[pjbgtnj]

I’m out but will send a test when I get back. For my case the middle element was nil only in the reversed slice of points of length 7. I would have expected your middle element to be 0.

…

Sent from my iPhone

On Dec 18, 2019, at 5:21 PM, Tammer Saleh ***@***.***> wrote: Hey @pjbgtnj - I think you might be incorrect. The existing tests run this code against an odd number of elements and they pass: https://github.com/superorbital/golangcookbook.github.io/blob/c93e83ac06d037e32126b020980f7a15bd6c58e9/chapters/arrays/reverse/test_copy.go#L5-L18 Could you possibly attempt a PR where you show this code failing? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[pjbgtnj]

I just ran the test you provided above and the output is incorrect:

[5 4 0 2 1] <<< - The 0 should be a 3
[1 2 3 4 5]

When copying in place the middle most element doesn't need to be swapped but when you copy to a new slice, it does.

[tammersaleh]

That was right in front of me, and I still didn't see it. Thank you for being persistent :)