3
\$\begingroup\$

I've started Go for the last couple of months and I decided to challenging small projects, so I created this proxy requests script.

It works well (in my opinion at least) but I'm looking for different ways of improving this further like being able to target all servers, or making this more concise and compact. To be honest I think it is readable but again that's my opinion. I'm looking for it to be peer reviewed with no holds barred critism of my code. Why? to learn new ways of improving my coding techniques and making it much more efficient.

If anyone wants to view the code feel free:

package main

import (
    "context"
    "fmt"
    "math/rand"
    "time"
)

type frame struct {
    context.Context
    resp chan<- string
}

type frames chan frame

func main() {
    var serverConns []frames
    const totalServers = 10
    for i := 0; i < 10; i++ {
        serverID := i
        conn := make(frames)
        defer close(conn)
        serverConns = append(serverConns, conn)
        go func() {
            // the server
            for frame := range conn {
                resps := []string{"banana", "apple", "guava"}
                time.Sleep(50*time.Millisecond + time.Duration(rand.Intn(500))*time.Millisecond)
                r := fmt.Sprint("server ", serverID, ": ", resps[rand.Intn(len(resps))])
                select {
                case <-frame.Done():
                case frame.resp <- r:
                }
            }
        }()
    }
    // the proxy
    const maxFanout = 3
    var conns []frames
    connIDs := rand.Perm(len(serverConns))[:totalServers/2]
    for _, connID := range connIDs {
        fmt.Println("selected server:", connID)
        conns = append(conns, serverConns[connID])
    }
    fanoutConn := make(frames)
    go func() {
        for p := range fanoutConn {
            ctx, cancel := context.WithCancel(p.Context)
            firstResp := make(chan string)
            for _, serverConn := range conns {
                serverConn := serverConn
                fwdframe := frame{
                    Context: ctx,
                    resp:    firstResp,
                }
                go func() {
                    serverConn <- fwdframe
                }()
            }
            response := <-firstResp
            cancel()
            p.resp <- response
        }
    }()
    // the client
    for {
        r := make(chan string)
        p := frame{
            Context: context.Background(),
            resp:    r,
        }
        fanoutConn <- p
        fmt.Println(<-r)
    }
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I'm looking for it to be peer reviewed with no holds barred critism of my code. Why? to learn new ways of improving my coding techniques and making it much more efficient. Judas Is Back


The evaluation criteria for a code review:

Code should be correct, maintainable, robust, reasonably efficient, and, most importantly, readable. Code should be useful and have real value.


I tried to read your code. I found it to be an unreadable, sprawling, blob of code. Sorry!

The first step is to give the code some structure. For example,

On the Criteria To Be Used in Decomposing Systems into Modules, D.L. Parnas, Carnegie-Mellon University, Communications of the ACM, December 1972, Volume 15, Number 12.

Parnas quotes Gouthier and Pont:

A well-defined segmentation of the project effort ensures system modularity. Each task forms a separate, distinct program module. At implementation time each module and its inputs and outputs are well-defined, there is no confusion in the intended interface with other system modules. At checkout time the in- tegrity of the module is tested independently; there are few sche- duling problems in synchronizing the completion of several tasks before checkout can begin. Finally, the system is maintained in modular fashion; system errors and deficiencies can be traced to specific system modules, thus limiting the scope of detailed error searching.


After spending ten minutes on basic cleanup, this is what I have:

Playground: https://play.golang.org/p/tsPqSe0urdv

The main function summarizes the program.

func main() {
    // the servers
    serverConns := servers()
    // the proxies
    fanoutConn := proxies(serverConns)
    // the client
    client(fanoutConn)
}

The server and servers functions encapsulate the server related processing, promoting information hiding. I did a lot of basic cleanup.

// the server
func server(id int, conn frames) {
    defer close(conn)

    server := fmt.Sprint("server ", id, ": ")
    resps := []string{"banana", "apple", "guava"}

    for frame := range conn {
        time.Sleep(time.Duration(50+rand.Intn(500)) * time.Millisecond)

        resp := server + resps[rand.Intn(len(resps))]

        select {
        case <-frame.Done():
        case frame.resp <- resp:
        }
    }
}

// the servers
func servers() []frames {
    const totalServers = 10
    serverConns := make([]frames, totalServers)
    for id := range serverConns {
        conn := make(frames)
        serverConns[id] = conn
        go server(id, conn)
    }
    return serverConns
}

There is more cleanup to be done on these and other functions.

As the code gets more readable, we move closer to other code review goals like correctness and maintainability.

proxy.go:

package main

import (
    "context"
    "fmt"
    "math/rand"
    "time"
)

type frame struct {
    context.Context
    resp chan<- string
}

type frames chan frame

// the server
func server(id int, conn frames) {
    defer close(conn)

    server := fmt.Sprint("server ", id, ": ")
    resps := []string{"banana", "apple", "guava"}

    for frame := range conn {
        time.Sleep(time.Duration(50+rand.Intn(500)) * time.Millisecond)

        resp := server + resps[rand.Intn(len(resps))]

        select {
        case <-frame.Done():
        case frame.resp <- resp:
        }
    }
}

// the servers
func servers() []frames {
    const totalServers = 10
    serverConns := make([]frames, totalServers)
    for id := range serverConns {
        conn := make(frames)
        serverConns[id] = conn
        go server(id, conn)
    }
    return serverConns
}

// the proxy
func proxy(conns []frames, fanoutConn frames) {
    for p := range fanoutConn {
        ctx, cancel := context.WithCancel(p.Context)
        firstResp := make(chan string)
        for _, serverConn := range conns {
            serverConn := serverConn
            fwdframe := frame{
                Context: ctx,
                resp:    firstResp,
            }
            go func() {
                serverConn <- fwdframe
            }()
        }
        response := <-firstResp
        cancel()
        p.resp <- response
    }
}

// the proxies
func proxies(serverConns []frames) frames {
    connIDs := rand.Perm(len(serverConns))[:len(serverConns)/2]
    var conns []frames = make([]frames, 0, len(connIDs))
    for _, connID := range connIDs {
        fmt.Println("selected server:", connID)
        conns = append(conns, serverConns[connID])
    }
    fanoutConn := make(frames)
    go proxy(conns, fanoutConn)
    return fanoutConn
}

// the client
func client(fanoutConn frames) {
    for {
        resp := make(chan string)
        p := frame{
            Context: context.Background(),
            resp:    resp,
        }
        fanoutConn <- p
        fmt.Println(<-resp)
    }
}

func main() {
    // the servers
    serverConns := servers()
    // the proxies
    fanoutConn := proxies(serverConns)
    // the client
    client(fanoutConn)
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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