3
\$\begingroup\$

Follow-up of this question.

Questions:

package main

import (
    "fmt"
    "sync"
)

func philos(id int, left, right chan bool, wg *sync.WaitGroup) {
    fmt.Printf("Philosopher # %d wants to eat\n", id) 
    <-left
    <-right
    left <- true
    right <- true
    fmt.Printf("Philosopher # %d finished eating\n", id) 
    wg.Done()
}

func main() {
    const numPhilos = 5 
    var forks [numPhilos]chan bool
    for i := 0; i < numPhilos; i++ {
        forks[i] = make(chan bool, 1)
        forks[i] <- true
    }   
    var wg sync.WaitGroup
    for i := 0; i < numPhilos; i++ {
        wg.Add(1)
        go philos(i, forks[(i-1+numPhilos)%numPhilos], forks[(i+numPhilos)%numPhilos], &wg)
    }   
    wg.Wait()
    fmt.Println("Everybody finished eating")
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Just an observation, but the second argument to your routine in go philos() is forks[(i+numPhilos)%numPhilos], which is the exact same thing as writing forks[i] anyway \$\endgroup\$ – Elias Van Ootegem May 8 at 9:16
  • \$\begingroup\$ @EliasVanOotegem: good point! \$\endgroup\$ – Attilio May 8 at 12:41
5
\$\begingroup\$

All in all, it's not a bad implementation at all. The bulk of my comments will focus on idiomatic golang stuff, and some small tweaks you can make to the main function. As I usually do here, I'll go through it line by line

func main() {
    const numPhilos = 5

OK, so you're starting out defining an untyped constant in your main. That's perfectly valid, and it doesn't make much of a difference, but generally speaking, I'd define my constants outside of functions. This makes it easier to centralise your constants, see what constants are used in the file/package (if you're exporting them), and makes it easier to break up your code into smaller functions further down the line. Moving on:

var forks [numPhilos]chan bool

OK, so arrays can be used in go, but it's generally recommended you don't. The rule of thumb is: use slices if you can. Next:

for i := 0; i < numPhilos; i++ {
    forks[i] = make(chan bool, 1)
    forks[i] <- true
}

Again, no real issues here, only, you're assigning a channel to an index in an array, and then writing to it, accessing the array again. I'd use a scoped variable instead. Next:

var wg sync.WaitGroup
for i := 0; i < numPhilos; i++ {
    wg.Add(1)
    go philos(i, forks[(i-1+numPhilos)%numPhilos], forks[(i+numPhilos)%numPhilos], &wg)
}   
wg.Wait()

Right, short of what I pointed out in the comment about forks[(i+numPhilos)%numPhilos] being the same as forks[i], this all works, but there's quite a few things we can improve:

  • you're creating a var wg sync.WaitGroup, and passing pointers to it. Good, but why not create a pointer literal. It's safer (less likely to pass by value accidentally), and code is easier to read IMO
  • You're incrementing i, and accessing forks, knowing full well that the len(forks) won't be exceeded. After all, your loop is the same as the one you used to initialise forks. So why not loop over forks to begin with?
  • wg.Add(1) is incrementing the waitgroup for each routine, but you clearly know beforehand how many routines you're going to spin up. You can add that total number of routines to your waitgroup outside of the loop.
  • I don't like the names numPhilos and philos for a func.
  • You're passing the waitgroup as a last argument. It's more common to see context.Context as the first argument, and things like a waitgroup (controlling the runtime and routines) as first arguments, rather than last

Last line:

fmt.Println("Everybody finished eating")

This should not be the end of your program. You should close the channels!

Now, let's put all of this together:

const numPhilos = 5

func main() {
    // create slice, not an array - set capacity to numPhilos
    forks := make([]chan bool, 0, numPhilos)
    for i := 0; i < numPhilos; i++ {
        // create channel in local scope
        ch := make(chan bool, 1)
        ch <- true // write to channel directly
        forks = append(forks, ch) // append to forks slice
    }
    // I prefer literals, because I can create a pointer type directly
    wg := &sync.WaitGroup{}
    // add 1 for each channel in forks
    wg.Add(len(forks))
    for i, ch := range forks {
        // forks[i] is now ch, get the left one using the method you are using already
        go philos(wg, i, forks[(i+numPhilos-1)%numPhilos], ch)
    }
    wg.Wait()
    // close channels
    for _, ch := range forks {
        close(ch)
    }
    // done
    fmt.Println("Everybody finished eating")
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.