7
\$\begingroup\$

Whenever I try to learn a new language, I write a subnetting calculator. Here's my attempt with Go.

In particular, I think I'm not writing idiomatic Go, as well as abusing float64(), uint64(), etc.

My math should be correct, although I might have goofed when trying to translate JavaScript's number types into Go's C-esque versions.

Also, I know I should do some error-checking on the inputs (e.g. seeing if I get a logical IP instead of a string like "cat", but that'll come once I learn how to write correct Go ;))

package main

import (
    "flag"
    "fmt"
    "math"
    "os"
    "strconv"
    "strings"
)

const SIXTY_FOUR_BITS uint64 = 18446744073709551615
const MAX_BIT_VALUE int = 32
const MAX_BIT_BIN uint64 = 255

func main() {
    var pf uint64
    var unpack_pf uint64

    flag.Parse()

    if len(flag.Args()) == 0 {
        flag.Usage()
        os.Exit(1)
    }

    var ip_address string = flag.Args()[0]
    var netmask = flag.Args()[1]

    var ip_arr []string = strings.Split(ip_address, ".")
    var sm_arr []string = strings.Split(netmask, ".")

    pf, _ = strconv.ParseUint(netmask, 10, 32)

    if len(sm_arr) == 1 {
        if pf <= 32 {
            unpack_pf = unpack_int(pf)
            sm_arr = strings.Split(int_qdot(unpack_pf), ".")
            netmask = strings.Join(sm_arr, ".")
        } else if pf > 32 {
            pf = host_pf(pf)
            unpack_pf = unpack_int(pf)
            sm_arr = strings.Split(int_qdot(unpack_pf), ".")
            netmask = strings.Join(sm_arr, ".")
        }
        if pf == 0 {
            panic("Cannot have netmask < 0 or 0.0.0.0")
        }
    }

    var ip_dec uint64 = qdot_int(ip_arr)
    var nm_dec uint64 = qdot_int(sm_arr)

    var network string = network_address(ip_dec, nm_dec)
    var broadcast string = broadcast_address(ip_dec, nm_dec)
    var wildcard string = int_qdot(^nm_dec)

    var prefix uint64 = qdot_pf(nm_dec)
    var ip_class string = class(ip_arr[0])
    var num_hosts float64 = hosts(prefix)
    var num_subnets float64 = subnets(nm_dec)
    var net_min string = int_qdot(qdot_int(strings.Split(network, ".")) + 1)
    var net_max string = int_qdot(qdot_int(strings.Split(broadcast, ".")) - 1)
    var ip_range string = net_min + " - " + net_max
    var addr_type string = "IPv4"

    write_results(addr_type, ip_address, ip_dec, netmask, nm_dec, wildcard, ip_class, network, prefix, net_min, net_max, broadcast, ip_range, num_subnets, num_hosts)

}

func write_results(addr_type, ip_address string, ip_dec uint64, netmask string, nm_dec uint64, wildcard string, ip_class string, network string, prefix uint64, net_min string, net_max string, broadcast string, ip_range string, num_subnets float64, num_hosts float64) int {

    fmt.Printf("====================\n    %s RESULTS    \n====================\n\nAddress:        - %s\n  (hex):        - %#X\n  (dec):        - %d\n  (bin):        - %b\nNetmask:        - %s = %d\n  (hex):        - %#X\n  (dec):        - %d\n  (bin):        - %b\nWildcard:       - %s\nClass:          - %s\nNetwork:        - %s\nNet Min:        - %s\nNet Max:        - %s\nBroadcast:      - %s\nRange:          - %s\nSubnets:        - %.0f\nHosts/net:      - %.0f\n\n-\n", addr_type, ip_address, ip_dec, ip_dec, ip_dec, netmask, prefix, nm_dec, nm_dec, nm_dec, wildcard, ip_class, network, net_min, net_max, broadcast, ip_range, num_subnets, num_hosts)

    return 0

}

func unpack_int(sm_int uint64) uint64 {
    return SIXTY_FOUR_BITS << (uint64(MAX_BIT_VALUE) - sm_int)
}

func qdot_int(qdot []string) uint64 {
    var a uint64 = 0
    var w uint64
    var x uint64
    var y uint64
    var z uint64

    w, _ = strconv.ParseUint(qdot[0], 10, MAX_BIT_VALUE)
    x, _ = strconv.ParseUint(qdot[1], 10, MAX_BIT_VALUE)
    y, _ = strconv.ParseUint(qdot[2], 10, MAX_BIT_VALUE)
    z, _ = strconv.ParseUint(qdot[3], 10, MAX_BIT_VALUE)

    a += w << 24
    a += x << 16
    a += y << 8
    a += z << 0

    return a
}

func int_qdot(integer uint64) string {
    var w string = strconv.FormatUint(integer>>24&MAX_BIT_BIN, 10)
    var x string = strconv.FormatUint(integer>>16&MAX_BIT_BIN, 10)
    var y string = strconv.FormatUint(integer>>8&MAX_BIT_BIN, 10)
    var z string = strconv.FormatUint(integer&MAX_BIT_BIN, 10)

    var a []string = []string{w, x, y, z}

    return strings.Join(a, ".")
}

func network_address(ip uint64, sm uint64) string {
    return int_qdot(ip & sm)
}

func broadcast_address(ip uint64, sm uint64) string {
    return int_qdot(ip | ^sm)
}

func host_pf(hn uint64) uint64 {
    var x float64

    if 0 < hn {
        x = float64(MAX_BIT_VALUE) - math.Ceil(math.Log2(float64(hn)))
    } else {
        x = 0
    }
    return uint64(x)
}

func qdot_pf(qdot uint64) uint64 {
    var x uint64 = qdot

    var mask []uint64 = []uint64{
        0x55555555,
        0x33333333,
        0x0F0F0F0F,
        0x00FF00FF,
        0x0000FFFF,
    }

    var shift uint64
    var i uint64
    for i, shift = 0, 1; i < 5; i, shift = i+1, shift*2 {
        x = x&mask[i] + x>>shift&mask[i]
    }

    return x
}

func class(bit string) string {
    var ip uint64
    var x string

    ip, _ = strconv.ParseUint(bit, 10, 32)

    if ip < 128 {
        x = "Class A"
    } else if ip < 192 {
        x = "Class B"
    } else if ip < 224 {
        x = "Class C"
    } else if ip < 240 {
        x = "Class D"
    } else if ip < 256 {
        x = "Class E"
    } else if ip < 0 {
        panic("Invalid IP")
    } else {
        panic("Super invalid IP")
    }

    return x
}

func subnets(pf uint64) float64 {
    var mod_base float64 = math.Mod(float64(pf), 8)
    var x float64

    if mod_base > 0 {
        x = math.Pow(2, mod_base)
    } else {
        x = 256
    }

    return x
}

func hosts(bits uint64) float64 {
    var x float64 = float64(bits)
    if x >= 2 {
        x = math.Pow(2, (float64(MAX_BIT_VALUE)-x)) - 2
    }

    return x
}
\$\endgroup\$
6
\$\begingroup\$

You have correctly noticed that this isn't extremely idiomatic. However, this code also has some issues that are completely language independent:

  • You have a large amount of magic numbers. Either explain them with a short comment, or (preferably) calculate them. Let the compiler do constant folding; don't do it yourself.

  • Your code does not have a single comment. Not having comments does not mean that your code would be self-documenting.

  • You have the tendency to declare your variables up front. Try to put the declaration as close to its first usage as possible, and try to restrict variables to the narrowest possible scope. Except in dated languages such Pascal or C 89, declaration at the top of a scope is not generally required. In fact, it is often beneficial to never declare variables without also defining them in the same statement.

  • Your variable names are quite arbitrary. Names such as w, x, y, z are unacceptable. Instead, byte1, byte2, … might be preferable, but that means you actually want a loop. Let the compiler unroll loops if it sees fit, don't do so yourself (at least without benchmarking).

  • Your main function does both general startup such as parsing arguments, and contains the main algorithm. Separate these two concerns. When you split out the algorithm in its own function, consider returning a struct instead of a large tuple of return values.

  • If you have some kind of template in your code, try to make this template look similar to the result. In the case of your write_results format string, this means splitting it up into several lines, and concatenating the string pieces to the final format. Try to limit your line length to avoid line wrapping or horizontal scrolling in your code.

A few Go-specific remarks beyond those issues mentioned by Yuushi:

  • If the exact size of a numeric type is relevant in your program (and this is the case here), don't use int or uint, but int32 or something like that. The size of int is implementation-specific and may either be 32 bit or 64 bit. (See the language spec)

  • If a built-in type has a special semantic meaning in your program, declare a type alias, e.g. type Ip4Address uint32.

  • Once you have a type for your IP adresses, you can use Go's OOP features for a more elegant interface. Instead of func hosts(bits uint64) float64 consider func (this Ip4Adress) Hosts() int.

  • If we're already discussing that hosts function: Why on earth are you working with floats when you are actually calculating a discrete value? Note that \$2^n\$ can be calculated as 1 << n for integers, so you don't have to use math.Pow.

\$\endgroup\$
  • \$\begingroup\$ Regarding magic numbers, I am missing some, thanks! I think the hex values would be weird as constants -- am I incorrect in thinking that? Also, I will comment my code, but I wrote this with vim split screened and my other code is java-doc commented... I just haven't added the comments yet is all. I shouldn't have left that out, sorry. I'll loop my unrolled loops -- I assumed since there are only n values, unrolling wouldn't harm anything. Regarding the Go remarks, these are awesome and I'll definitely implement them. Thanks so much! \$\endgroup\$ – Eric Lagergren Oct 5 '14 at 16:10
  • 2
    \$\begingroup\$ I don't think using this as a receiver parameter for a newcomer is a good thing to do. Especially when the Style Guide explicitly discourages that. Good review otherwise. \$\endgroup\$ – Ainar-G Oct 5 '14 at 17:48
5
\$\begingroup\$

The first thing that jumps out is the use of var everywhere. Most of the time this is unnecessary:

var prefix uint64 = qdot_pf(nm_dec)
var ip_class string = class(ip_arr[0])
var num_hosts float64 = hosts(prefix)
var num_subnets float64 = subnets(nm_dec)
var net_min string = int_qdot(qdot_int(strings.Split(network, ".")) + 1)
var net_max string = int_qdot(qdot_int(strings.Split(broadcast, ".")) - 1)
var ip_range string = net_min + " - " + net_max
var addr_type string = "IPv4"

This whole block could be declared using := syntax, which will also infer the types for you:

prefix := qdot_pf(nm_dec)
ip_class := class(ip_arr[0])
num_hosts := hosts(prefix)
// And so on...

Almost everywhere you have a var could be replaced simply by using a :=. The only time you really need to use var is when you aren't initialising it with a value. This also works for arrays and slices:

var a []string = []string{w, x, y, z}

can just be:

a = []string{w, x, y, z}

Function names in Go should either be PascalCase if they are public, or camelCase if they are meant to be package private.

You can write your qdot_int function in a much shorter way:

func qdot_int(qdot []string) uint64 {
    a := uint64(0)
    var b uint64
    shifts := [4]uint{24, 16, 8, 0}

    for i, v in range qdot {
        b, _ := strconv.ParseUint(v, 10, MAX_BIT_VALUE)
        a += (b << shifts[i])
    }

    return a
}

Generally, switch is more idiomatic than if/else chains in Go.

func class(bit string) string {
    ip, _ = strconv.ParseUint(bit, 10, 32)

    switch(ip) {
        case ip < 128:
            return "Class A"
        case ip < 192:
            return "Class B"
        case ip < 224:
            return "Class C" 
        case ip < 240:
            return "Class D"
        case ip < 256:
            return "Class E"
        default:
            panic("Invalid IP")
    }
}

Finally, you can use a const block to avoid having to rewrite const for each variable:

const (
    SIXTY_FOUR_BITS uint64 = 18446744073709551615
    MAX_BIT_VALUE int = 32
    MAX_BIT_BIN uint64 = 255
)

You can also do this for var as well, when it is required:

var (
    // Variable declarations here
) 
\$\endgroup\$
  • \$\begingroup\$ Thank you! A couple of questions -- why is var looked down upon? Wouldn't it be more helpful for the compiler to know the type of variable instead of having to infer it? Also, the case/switch is new to me -- in the languages that I currently know, if/else chains are preferred for readability. Thanks again :-) \$\endgroup\$ – Eric Lagergren Oct 8 '14 at 2:56
  • \$\begingroup\$ @eric_lagergren Nope. In most cases it's just noise, and it will make literally no difference to the compiler. The big difference with switch in Go is that it does not fallthrough by default (so you don't need to break after each case). This is likely a large reason as to why it is more idiomatic compared to other languages. \$\endgroup\$ – Yuushi Oct 9 '14 at 12:21

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.