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")
}
go philos()isforks[(i+numPhilos)%numPhilos], which is the exact same thing as writingforks[i]anyway \$\endgroup\$