5
\$\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\$
3
  • 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\$ Commented May 8, 2019 at 9:16
  • \$\begingroup\$ @EliasVanOotegem: good point! \$\endgroup\$ Commented May 8, 2019 at 12:41
  • \$\begingroup\$ IMO this isn't correct as it doesn't demonstrate the goal of the problem. The output suggests that philosophers eat sequentially. time.Sleep()s and "eating" messages should be added to allow one to examine the output to confirm its working. \$\endgroup\$ Commented Sep 22, 2022 at 17:17

1 Answer 1

6
\$\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\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.