sort.Slice arguments

[Val]

Hello fellows
The following code (playground) :

    //
    // I am in Philadelphia, and I want to visit other cities
    //
    itinerary := []string{"Philadelphia", "Chicago", "Boston", "Austin"}

    //
    // I am in Philadelphia, and I want to visit other cities in alphabetical order
    //
    sort.Slice(itinerary[1:], func(i, j int) bool {
        return itinerary[i] < itinerary[j]
    })

is broken for retrospectively obvious reasons.  Yet, I scratched my head for several minutes before finding the bug in my actual code.
So I wonder if this would be a good case for a new vet rule.  Something like "slicing the first argument is not a good idea, it will lead to something that is either broken or convoluted".
What do you think?

[Jan Mercl]

On Wed, Mar 8, 2017 at 2:10 PM Val <dele...@gmail.com> wrote:

> What do you think?

You should explain what you've expected to get instead of what you've got. Without that, I for one, cannot figure out what you see as broken and I have no idea why do you think it's not a good idea to sort a slice of a slice. After all, it's just a slice as any other.

--

-j

[Valentin Deleplace]

I did explain the expected result : "and I want to visit other cities in alphabetical order"

[C Banning]

https://play.golang.org/p/Pqd0Wk-yqe

[David Peacock]

On Wed, Mar 8, 2017 at 8:32 AM, Valentin Deleplace <dele...@gmail.com> wrote:

I did explain the expected result : "and I want to visit other cities in alphabetical order"

Jan is correct; the characterization of a problem hinges on accurately describing what you expect and what happened instead.  Terms such as "broken" and "does not work" are best avoided because others don't know what your definitions are for these in the given context. :-)

That being said, your alphabetical order is returned as you intend if you adjust line 17 thusly:

sort.Slice(itinerary[:], func(i, j int) bool {

Cheers,

David

 

Le 8 mars 2017 2:16 PM, "Jan Mercl" <0xj...@gmail.com> a écrit :

On Wed, Mar 8, 2017 at 2:10 PM Val <dele...@gmail.com> wrote:

> What do you think?

You should explain what you've expected to get instead of what you've got. Without that, I for one, cannot figure out what you see as broken and I have no idea why do you think it's not a good idea to sort a slice of a slice. After all, it's just a slice as any other.

--

-j

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Val]

Sorry for not elaborating in the first place (I was trying to make the point very concise).

This is the starting city :  Philadelphia

This are the other cities :   Chicago, Boston, Austin

The itinerary is the concatenation of starting city + other cities to be visited :   [Philadelphia, Chicago, Boston, Austin]

In this contrived example the first city is fixed, because this is where I am now, wherever I decide to go next. That's why I decide to reorder all cities of my itinerary except the first one.

This is what my naive code what trying to achieve (expected value of slice itinerary after partial sort) : [Philadelphia Austin Boston Chicago]

This is what the code actually produces (value of slice itinerary after partial sort) : [Philadelphia Boston Chicago Austin] , which is why i say it is broken : the last 3 items don't end up in alphabetical order like I expected them to be.

In a real program, there would be various legit reasons to sort only part of a slice, and the items would not always have a builtin type like string, rather custom struct types.

@CBanning  Thanks, I had not thought of StringSlice!  But it's not applicable in my real programs, which deal with custom struct types (not strings).

@David  Sorting the whole slice is not what I was trying to achieve, because I can't change my starting point.

The bug in my code is that the two arguments I pass to sort.Slice are inconsistent : the portion to be sorted is a reslicing from itinerary, while the "less" closure indexes items of the whole itinerary.

I brought up the "go vet" idea because I feel that whenever someone will do some reslicing directly in the first argument,
- either the result will be "broken" like mine,
- or the code in the less function will have to refer to the same (unnamed) resliced portion, which is imo convoluted : fix 1 or fix 2 .

The most readable alternative I can think of while still using sort.Slice is to reslice in a new variable prior to sorting : fix 3 .

Best regards
Val

To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

[James Bardin]

On Wednesday, March 8, 2017 at 10:01:08 AM UTC-5, Val wrote:

- or the code in the less function will have to refer to the same (unnamed) resliced portion, which is imo convoluted : fix 1 or fix 2 .

Having recently written code that is pretty much the same as fix #1, I wouldn't want vet to flag that as incorrect when it isn't.  That's more a lint issue, but I'm not convinced it's a problem when unit test coverage of the code should pick out the problem fairly quickly. 

[David Peacock]

On Wed, Mar 8, 2017 at 10:01 AM, Val <dele...@gmail.com> wrote:

Sorry for not elaborating in the first place (I was trying to make the point very concise).

Thank you for detailing further! :-)

The bug in my code is that the two arguments I pass to sort.Slice are inconsistent : the portion to be sorted is a reslicing from itinerary, while the "less" closure indexes items of the whole itinerary.

I brought up the "go vet" idea because I feel that whenever someone will do some reslicing directly in the first argument,
- either the result will be "broken" like mine,
- or the code in the less function will have to refer to the same (unnamed) resliced portion, which is imo convoluted : fix 1 or fix 2 .

The most readable alternative I can think of while still using sort.Slice is to reslice in a new variable prior to sorting : fix 3 .

Personally, I would favor fix 3 in this context.

Cheers,

David

[Ain]

Another, OO-ish fix is to use method on custom type (https://play.golang.org/p/a5S5rNog5h):

type itinerary []string

func (t itinerary) sort() {
    sort.Slice(t, func(i, j int) bool {
        return t[i] < t[j]
    })
}

func main() {
    itinerary := itinerary{"Philadelphia", "Chicago", "Boston", "Austin"}
    itinerary[1:].sort()
    fmt.Println("My new itinerary is", itinerary)
}


ain

[Jérôme Champion]

As itinerary and the reslicing operation use the same backing array, you could just offset your indexes:

https://play.golang.org/p/2ZzKST5J1g

I'm not sure if it's a good idea, could the sort.Slice implementation change in the future and make a regression?

[代君]

modify your code like this:

```go

package main

import (

"fmt"

"sort"

)

func main() {

//

// I am in Philadelphia, and I want to visit other cities

//

itinerary := []string{"Philadelphia", "Chicago", "Boston", "Austin", "Def"}

//

// I am in Philadelphia, and I want to visit other cities in alphabetical order

//

sort.Slice(itinerary[1:], func(i, j int) bool {

return itinerary[1:][i] < itinerary[1:][j]

})

fmt.Println("My new itinerary is", itinerary)

}

```

because you use itinerary[1:] to sort, but your sort function choosed itinerary's index to compare, so your result is not ok.