<p>Hi Guys, </p>
<p>I'm learning go and am building a sample console app.</p>
<p>It's still a work in progress but I was hoping you could take a look when you get chance and give me your feedback. </p>
<p>You can find it here: <a href="https://github.com/donpisci/BusRoute" rel="nofollow">https://github.com/donpisci/BusRoute</a></p>
<p>Thanks in advance!</p>
<hr/>**评论：**<br/><br/>bketelsen: <pre><p>In no particular order:
1) generally Go apps don't have capital letters in the import path. BusRoute might do strange things for people with case sensitive file systems Same with all file names. No caps please.
2) Use go style comments and your godoc generated documentation will look good. ( <a href="http://blog.golang.org/godoc-documenting-go-code" rel="nofollow">http://blog.golang.org/godoc-documenting-go-code</a> )
3) No need to export the Print type, it's a self contained
4) Love that you used busroute.CommandError. Named errors are great.
5) You declared interfaces (like traveller) but I don't see them used in code anywhere. I would expect you to use the Traveller interface on the bus. Then you could have Passengers and Dogs, for example, both implementing the Traveller interface.</p>
<p>Overall great start to Go.</p></pre>izuriel: <pre><p>Right off the bat, in the main.go file I feel you missed an opportunity to fully
leverage yoru interface. I would simplify your command generate process to
something along the lines of creating a map with your interface and assigning
your commands to it.</p>
<pre><code>var busrouteCommands = map[string]busroute.Commander{
"print": NewPrint(),
"exit": new(Exit),
"passenger": busroute.NewPassengerCommand(),
"bus": busroute.NewBusCommand(),
"create-database": busroute.NewCreateDatabaseCommand(),
}
var helpCommand = new(Help)
</code></pre>
<p>And the benefit of this, of course, is that parse becomes:</p>
<pre><code>func parse(cmd string) busroute.Commander {
key := strings.TrimRight(strings.ToLower(cmd), "\n")
if commander, ok := busrouteCommands[key]; ok {
return commander
} else {
return helpCommand
}
}
</code></pre>
<p>(Also notice that I changed <code>string('\n')</code> to <code>"\n"</code>, I'm sure the Go compiler
will optomize this for you but if you want a string, just make one)</p>
<p>New additions of command require little more than adding a new entry to the map.
If you do need the new addition of commands when you run parse, you can take
a similar yet slightly different approach:</p>
<pre><code>type CommandDefiner func() busroute.Commander
var busrouteCommands = map[string]CommandDefiner{
"print": func() busroute.Commander {
return NewPrint()
},
// omitted for brevity
}
</code></pre>
<p>And then your adjustment to parse is to return <code>commander()</code> instead of <code>commander</code>.
These are mostly small bits but I thought I would point them out so you were aware
of alternative approaches. </p></pre>augorak: <pre><p>I highly recommend the tools "go vet", "golint" and "goimports". Lots of the feedback you've already gotten would be mentioned/corrected by those tools.</p>
<p>I also recommend using a text editor like Sublime (with GoSublime) or Vim (with vim-go) instead of an IDE. Its definitely more common to not use an IDE and you'll want to see your project the way others will if they fork it.</p>
<p>You've split your main.go into sections with comments when what I think you really want to do is split those blocks into files.</p>
<p>Your "main_test.go" file doesn't contain any functions that start with "Test..." so you are not leveraging the included test runner ( <a href="http://golang.org/pkg/testing/" rel="nofollow">http://golang.org/pkg/testing/</a> )</p>
<p>It is more idiomatic to return an error and let the caller decide what to do than to print it. For example:</p>
<pre><code>func loadConnectionString() (Config, error) {
content, err := ioutil.ReadFile("config.json")
if err != nil {
fmt.Print("Error:", err)
}
var conf Config
err = json.Unmarshal(content, &conf)
if err != nil {
fmt.Print("Error:", err)
}
return conf, err
}
</code></pre>
<p>Would be written as:</p>
<pre><code>func loadConnectionString() (*Config, error) {
content, err := ioutil.ReadFile("config.json")
if err != nil {
return nil, err
}
conf := new(Config)
err = json.Unmarshal(content, conf)
if err != nil {
return nil, err
}
return conf, err
}
</code></pre></pre>donpisci: <pre><p>Thank you all for the feedback, I really appreciate it. I've incorporated the changes mentioned (not committed yet) and think I am starting to think in more of a 'go' way. Although, I have tried declaring variables in C# using ':=' !</p>
<p>I think the hardest bit so far has been trying to stay idiomatic as, coming from a C# background, there are number of differences. </p></pre>
