<p>Hi all,</p>
<p><a href="https://play.golang.org/p/LaTzw5nEtO" rel="nofollow">https://play.golang.org/p/LaTzw5nEtO</a></p>
<p>This is my first foray into Go (I'm a C dev by day) and I've got no one for a code review, if anyone has 2 minutes to look through I'd be very appreciative of some feedback</p>
<p>The code is only short, it reads in 4 CSV files that contain DB format data (tables, columns, primary keys and indexes) and produces a JSON file with it all together.</p>
<p>The goal is to then use this to be able to easily put together a data dictionary in a static site with something like angular eventually.</p>
<p>Thanks, Daniel.</p>
<hr/>**评论：**<br/><br/>fr4nk3n: <pre><p>you can write the following:</p>
<pre><code>v, ok := indexesMap[table.Name]
if ok == true {
tables[i].Indexes = v
}
</code></pre>
<p>like this:</p>
<pre><code>if v, ok := indexesMap[table.Name]; ok {
tables[i].Indexes = v
}
</code></pre>
<p>I would make sure an index of an array/slice exists before you try to access it.</p>
<p>If you haven't already take a look at <a href="https://golang.org/doc/effective_go.html" rel="nofollow">Effective Go</a>.</p>
<p>If these are very large csv files, maybe take a look at the <a href="https://golang.org/pkg/bufio/#example_Scanner_lines" rel="nofollow">bufio package</a>.</p></pre>dphobson: <pre><p>Thanks.</p>
<p>That's a nice little tweak, I remember seeing that in the tour now you've pointed it out.</p>
<p>The files are only a few hundred lines long so I think it's safe to read it all in as one at the moment, I'll have a look at bufio for future though.</p></pre>Frakturfreund: <pre><p>This is just a stylistic nitpick, but instead of this C-style code</p>
<pre><code>if input.filename == "tables.csv" {
fileData = []byte(tables_csv)
} else if input.filename == "columns.csv" {
fileData = []byte(columns_csv)
} else if input.filename == "indexes.csv" {
fileData = []byte(indexes_csv)
} else if input.filename == "pks.csv" {
fileData = []byte(pks_csv)
}
</code></pre>
<p>you should write in Go</p>
<pre><code>switch input.filename {
case "tables.csv":
fileData = []byte(tables_csv)
case "columns.csv":
fileData = []byte(columns_csv)
case "indexes.csv":
fileData = []byte(indexes_csv)
case "pks.csv":
fileData = []byte(pks_csv)
}
</code></pre>
<p>It’s shorter and looks more clear. Notice that the <a href="https://github.com/golang/go/wiki/Switch" rel="nofollow">Go <code>switch</code> doesn't need <code>break</code>’s, but has a <code>fallthrough</code> keyword instead</a>.</p></pre>subtlepseudonym: <pre><p>Both code blocks compile to the same code (IIRC) so it's definitely worth going for readability.</p></pre>dphobson: <pre><p>The switch is definitely clear and I'll use that in future</p></pre>carsncode: <pre><p>Generally looks OK to me at first glance, but I'll call out a couple things. I'd put your quiet setting in a global so you don't have to pass it to your log function every time, since you always pass the same variable in. Unless this is a throwaway app you'll use twice and never look at again, you should handle errors better than just panicking. Lastly in all the places where you're instantiating a struct and comment all the fields, dump the comments and use the field labels instead of the short hand format.</p>
<p>Hope it helps, good luck with your gophering! Hope you're enjoying it!</p></pre>dphobson: <pre><p>Thanks.</p>
<ul>
<li><p>The panic() call was just a copy/paste from another file I was looking at, that should definitely be handled better.</p></li>
<li><p>What do you mean by the following? </p></li>
</ul>
<blockquote>
<p>Lastly in all the places where you're instantiating a struct and comment all the fields, dump the comments and use the field labels instead of the short hand format.</p>
</blockquote></pre>Frakturfreund: <pre><p>Instead of instantiating your structs like this:</p>
<pre><code>table := ddTables{
row[1], // Tablename
row[2], // Tablespace
row[4], // Comments
[]ddColumns{}, // Empty Columns
[]ddIndexes{}} // Empty Indexes
</code></pre>
<p>Do it like this:</p>
<pre><code>table := ddTables{
Name: row[1],
TableSpace: row[2],
Comments: row[4],
Columns: []ddColumns{},
Indexes: []ddIndexes{},
}
</code></pre>
<p>The second one is more typesafe: consider someone refactors the <code>ddTables</code> struct and switches the order of <code>Name</code> and <code>TableSpace</code> for consistency with other structs. This will introduce a silent bug in the first code variant, but not in the second (see also <a href="https://golang.org/doc/effective_go.html#composite_literals" rel="nofollow">Effective Go</a>).</p>
<p>You can still add comments in the second one, but normally the code should speak for itself.</p>
<p>Also note that the final <code>}</code> got it’s own line, and the additional <code>,</code> at the end of the previous line: That’s for clearer diffs.</p></pre>subtlepseudonym: <pre><p>On line 228:</p>
<pre><code>tables[i].Columns[j].Primary = v
</code></pre>
<p>Can be replaced with</p>
<pre><code>column.Primary = v
</code></pre>
<p>And the i and j variables (defined on 221 and 223) can be thrown away because this is the only instance of their use.</p></pre>dphobson: <pre><p>Thanks.</p>
<p>I've just tried this and it doesn't appear to work, I changed to the following:</p>
<pre><code>// Loop through the table data and populate the primary keys
for _, table := range tables {
for _, column := range table.Columns {
if v, ok := pkMap[string(table.Name+"."+column.Name)]; ok {
column.Primary = v
}
}
}
</code></pre>
<p>The code runs fine but the Columns.Primary data in tables isn't populated after the loop, is the table/column data in these ranges a copy of the current tables instance or is it a reference to it?</p>
<p>Changing back to this still works and takes into account some of the other suggestions above:</p>
<pre><code>// Loop through the table data and populate the primary keys
for i, table := range tables {
for j, column := range table.Columns {
if v, ok := pkMap[string(table.Name+"."+column.Name)]; ok {
tables[i].Columns[j].Primary = v
}
}
}
</code></pre></pre>subtlepseudonym: <pre><p>All of the variables on the left side of the range statement (i, j, table, column) are references. My understanding was that 'column.Primary' would serve as shorthand for 'column->Primary' though. </p>
<p>After poking through the spec a bit, it seems that 'column' is only preserved within the scope of the for loop because you used short variable declaration (:=). </p>
<p><a href="https://golang.org/ref/spec#For_statements" rel="nofollow">https://golang.org/ref/spec#For_statements</a></p></pre>
