3

Is there a way to make this golang code shorter?

func MergeSlices(s1 []float32, s2 []int32) []int {
    var slice []int
    for i := range s1 {
        slice = append(slice, int(s1[i]))
    }
    for i := range s2 {
        slice = append(slice, int(s2[i]))
    }
    return slice
}
Andrey Larin
  • 94
  • 1
  • 6
  • 2
    No there is not. – Volker May 12 '17 at 09:14
  • Avoid using `int32` unless there is a need to use a specific sized integer. Use `int` instead. `int` will be occupy 32 bits in 32 bit systems and 64 bits in 64 bit systems. [https://tour.golang.org/basics/11](https://tour.golang.org/basics/11) – Naveen Ramanathan May 14 '17 at 06:11

4 Answers4

5

You can't eliminate the loops to convert each element to int individually, because you can't convert whole slices of different element types. For explanation, see this question: Type converting slices of interfaces in go

The most you can do is use named result type, and a for range with 2 iteration values, where you can omit the first (the index) by assigning it to the blank identifier, and the 2nd will be the value:

func MergeSlices(s1 []float32, s2 []int32) (s []int) {
    for _, v := range s1 {
        s = append(s, int(v))
    }
    for _, v := range s2 {
        s = append(s, int(v))
    }
    return
}

But know that your code is fine as-is. My code is not something to always follow, it was to answer your question: how to make your code shorter. If you want to improve your code, you could start by looking at its performance, or even refactoring your code to not end up needing to merge slices of different types.

Community
  • 1
  • 1
icza
  • 389,944
  • 63
  • 907
  • 827
  • 4
    I would definitively not recommend using named return to save one line. – William Poussier May 12 '17 at 10:34
  • 1
    @WilliamPoussier Maybe not to save 1 line, but it has its uses (e.g. documenting), and sometimes they are required (e.g. [alter return value before returning in case of panics](http://stackoverflow.com/questions/33167282/how-to-return-a-value-in-a-go-function-that-panics/33167433#33167433)). Anyway, even the initial code is not something that needs shortening... – icza May 12 '17 at 10:43
  • I agree with those specific usecase, this is legit to use a named return value. I wanted to point out that mentionning this to anyone new to go should also come with the good practices that comes with it. – William Poussier May 12 '17 at 11:04
  • I'am new to golang so thank you all, it really helps. – Andrey Larin May 12 '17 at 12:16
  • 1
    @icza: Your use cases don't appear to apply to your example. See [Go Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments), [Named Result Parameters](https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters). – peterSO May 12 '17 at 12:47
  • 1
    @peterSO I agree. What I showed is not something to follow, it was to answer the question: how to make that code shorter. – icza May 12 '17 at 13:50
  • _Technically_, on a 32-bit system, you could use the unsafe package to reinterpret the int32 slice as an int slice. I think. But definitely not the float. – Kaedys May 12 '17 at 14:57
3

Your code should be correct, maintainable, readable, and reasonably efficient. Note that shortness of code is not one of the important goals. For good reason, Stack Exchange has another site for Code Golf questions: Programming Puzzles & Code Golf.

Your code could be improved; it's inefficient. For example, merging two len(256) slices,

BenchmarkMergeSlices      200000      8350 ns/op    8184 B/op     10 allocs/op

Here's a more efficient (and longer) version:

BenchmarkMergeSlices      300000      4420 ns/op    4096 B/op      1 allocs/op

.

func MergeSlices(s1 []float32, s2 []int32) []int {
    slice := make([]int, 0, len(s1)+len(s2))
    for i := range s1 {
        slice = append(slice, int(s1[i]))
    }
    for i := range s2 {
        slice = append(slice, int(s2[i]))
    }
    return slice
}

Use the Go Code Review Comments for Named Result Parameters. For example: "Don't name result parameters just to avoid declaring a var inside the function; that trades off a minor implementation brevity at the cost of unnecessary API verbosity. Clarity of docs is always more important than saving a line or two in your function."

peterSO
  • 158,998
  • 31
  • 281
  • 276
3
var s1 []int
var s2 []int

newSlice = append(s1, s2...)
Vince
  • 31
  • 1
  • 2
    While this may answer the question it's better to add some description on how this answer may help to solve the issue. Please read [_How do I write a good answer_](https://stackoverflow.com/help/how-to-answer) to know more. – Roshana Pitigala Jul 21 '18 at 18:03
0

The code can't get any shorter, but that's a goal of dubious value to begin with; it's not overly verbose as-is. You can, however, likely improve performance by eliminating the intermediate allocations. Every time you call append, if the target slice doesn't have enough space, it expands it, guessing at the necessary size since you haven't told it how much space it will need.

The simplest would just be to presize your target slice (replace var slice []int with slice := make([]int, 0, len(s1) + len(s2)); that way the appends never have to expand it. Setting the second parameter to 0 is important, that sets the length to zero, and the capacity to the total size needed, so that your appends will work as expected.

Once you've presized it though, you can get rid of the appends entirely, and directly set each index:

func MergeSlices(s1 []float32, s2 []int32) []int {
    slice := make([]int, len(s1) + len(s2))
    for i,v := range s1 {
        slice[i] = int(v)
    }
    for i,v := range s2 {
        slice[i+len(s1)] = int(v)
    }
    return slice
}

Playground link

Adrian
  • 42,911
  • 6
  • 107
  • 99
  • He's not filling with the indexes. Filling with the indexes would be `slice = append(slice, int(i))`, not `slice = append(slice, int(s1[i]))`. Note that it's appending the _value_ in `s1` at index `i`, not the index `i` itself. Also, slice allocation is fairly cheap. Go uses exponential slice allocation, so it amortizes to O(1). It's also worth noting that your version changes the output when both slices are 0, returning a 0-length slice instead of a nil slice. Either way, I would still use the `append()` version to avoid the weird `slice[i+len(s1)]` index in the second loop. – Kaedys May 12 '17 at 15:01
  • Right you are, I overlooked that. I'll correct my answer accordingly. And while slice allocation is cheap, doing it once will always be cheaper than doing it repeatedly, and doing it once to the correct size will avoid over-allocating, which append without preallocation is highly likely to do (the chances of exponential resize getting the size of the final allocation spot on are slim). – Adrian May 12 '17 at 15:04
  • Updated my comment with regards to the allocation, since it does introduce one additional semantic: it allocates an underlying array even if both input lists are zero, causing the returned slice to be non-nil but zero length, rather than the nil slice it would be in the OP's version. – Kaedys May 12 '17 at 15:05
  • Also true, but debatable which is preferable. Personally, if I put in two empty slices, I'd expect an empty slice back, not nil; but I can't say either nil or empty slice is objectively "better". – Adrian May 12 '17 at 15:08
  • I don't see any case where an empty slice would be superior. The only mechanical difference between the two is that a nil slice involves no allocation of an underlying array. So using a zero-length slice causes this function to trigger an _additional_ allocation when it otherwise wouldn't need one. – Kaedys May 12 '17 at 15:09
  • An odd complaint given that your original argument was "slice allocation is fairly cheap", but OK. I think the difference between the two is negligible, and I'd prefer the empty slice for consistency, but again, I'd say it's entirely up to personal preference. Given that the case of two empty inputs is probably fairly rare, I'd think the reduction in intermediate allocations would far outweigh the occasional, single, small, unnecessary allocation. You could add an initial empty check & `return nil` if it bothers you that much. – Adrian May 12 '17 at 15:14
  • Oh, for sure, and I usually _do_ put such a check in my code (I just prefer `nil`s as a matter of course). My main point was actually preferring `append` _in this case_ (though I often use your version with indexes as well), because of the double loop and the weird requirement to add the length of the first list to the index in the second loop. Using append in both is less likely to cause issues, for example if you expanded the function to take a third slice between the two or added some initial static values to the start of the slice or something. – Kaedys May 12 '17 at 15:22