I'm working on a new command line application in GO and was hoping I could get feedback on my design pattern, or suggestions on a better one to use. I'm still new to GO, only having used it for a handful of projects, and would value any suggestions to take better advantage of the "GO" way of doing things.
The intent of the application will be to either scan a single domain, or a list of domains and then output results in either a normal format (depending on flags) containing information about vulnerable domains and suggestions, or a file only showing the domains in the initial input that are vulnerable.
main.go
package main
import (
"flag"
"fmt"
"./libprojectstart"
"os"
)
func Banner() {
fmt.Printf("%s%sprojectstart - SSL Mass Renegotiation Tester\n%s",
libprojectstart.OKBLUE, libprojectstart.BOLD, libprojectstart.RESET)
}
func ParseCmdLine() (state *libprojectstart.State) {
s := libprojectstart.InitState()
flag.StringVar(&s.Domain, "d", "", "Domain to scan")
flag.StringVar(&s.DomainList, "dL", "", "List of domains to scan")
flag.StringVar(&s.OutputNormal, "oN", "", "File to write normal output to")
flag.StringVar(&s.OutputDomains, "oD", "", "File to write successful domains to")
flag.IntVar(&s.Threads, "t", 20, "Number of threads (Default: 20)")
flag.BoolVar(&s.NoColour, "--no-colour", true, "Don't Use colors in output")
flag.BoolVar(&s.Silent, "--silent", false, "Output successful scans only")
flag.BoolVar(&s.Verbose, "v", false, "Verbose output")
flag.BoolVar(&s.Usage, "h", false, "Display this message")
flag.Parse()
return &s
}
func main() {
state := ParseCmdLine()
if state.Silent != true {
Banner()
}
if libprojectstart.IsUnchanged(*state) || state.Usage {
Banner()
flag.PrintDefaults()
os.Exit(1)
}
if validated, error := libprojectstart.ValidateState(state); validated != true {
fmt.Printf("%s[!] %s%s", libprojectstart.FAIL, libprojectstart.RESET, error)
flag.PrintDefaults()
os.Exit(1)
}
}
state.go
package libprojectstart
// Contains state read in from the command line
type State struct {
Domain string // Domain to check for
DomainList string // File location for a list of domains
OutputNormal string // File to output in normal format
OutputDomains string // File to output domains only to
Verbose bool // Verbose prints, incl. Debug information
Threads int // Number of threads to use
NoColour bool // Strip colour from output
Silent bool // Output domains only
Usage bool // Print usage information
}
func InitState() (s State) {
return State { "", "", "", "", false, 20, false, false, false }
}
func ValidateState(s *State) (result bool, error string) {
if s.Domain == "" && s.DomainList == "" {
return false, "You must specify either a domain or list of domains to test"
}
return true, ""
}
func IsUnchanged(s State) bool {
return s == InitState()
}
colour.go
package libprojectstart
var (
HEADER = "033円[95m"
OKBLUE = "033円[94m"
OKGREEN = "033円[92m"
WARNING = "033円[93m"
FAIL = "033円[91m"
BOLD = "033円[1m"
UNDERLINE = "033円[4m"
RESET = "033円[0m"
)
The intent of this application is that the command line options, as well as the feedback being printed will grow significantly over time. I've aimed to demonstrate some of that error checking in ValidateState()
as that function will grow with the project, as will the printing of colours. Open to any and all suggestions for improvements!
1 Answer 1
Validation logic
When checking whether the program should print the usage message or not, I find it an unusual approach to say "is the state (of initial parameter values) unchanged?" It would be more natural to check facts more directly. For example in this program the only input you really require is one or more domain names. Then that should be the determining factor for displaying the usage message: "did the user specify a domain name?" Direct, straightforward, to the point.
Naming
The current names are not very intuitive:
State
is state of what? It's the command line parameters. Something likeCmdParams
would be better.Banner
is a noun, so it sounds like an object, not something that takes some action.PrintBanner
would be better.- Names starting with capital letters are exported (~ public API) in go. Many if not all names in the posted could be private, so their names should start with lowercase.
- Instead of
InitX
, when creating a new instance ofX
, the nameNewX
is more common.
Performance
Every time you evaluate s == InitState()
, InitState()
creates a new instance to perform the comparison. It would be enough to have just one such instance as a frame of reference, created as a var
at top-level scope, no need to create a new instance every time.
Style
Many of the functions specify a name for the returned value, but don't actually use it in the function body. So those optional names can be dropped.
Instead of this:
return State { "", "", "", "", false, 20, false, false, false }
You could take advantage of the default zero-values of strings and booleans, and write simpler like this:
return State{Threads: 20}
In my experience, negative boolean variables such as NoColour
are often confusing in practice.
Inverting the meaning and using !Colour
tends to work better.
Use short boolean conditions
Instead of if state.Silent != true
, you can write shorter as if !state.Silent
.
Banner printed twice
When the script is called without parameters, the banner will be printed twice. Swap these two statements to avoid that.
if state.Silent != true { Banner() } if libprojectstart.IsUnchanged(*state) || state.Usage { Banner() flag.PrintDefaults() os.Exit(1) }
-
\$\begingroup\$ Exactly what I was looking for, thank-you! Off to do some refactoring. \$\endgroup\$Michael A– Michael A2018年05月19日 11:32:03 +00:00Commented May 19, 2018 at 11:32