Short code review request

Hi all,

https://play.golang.org/p/LaTzw5nEtO

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

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.

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.

Thanks, Daniel.

---

评论：

fr4nk3n:

you can write the following:

v, ok := indexesMap[table.Name]
if ok == true {
    tables[i].Indexes = v
}

like this:
if v, ok := indexesMap[table.Name]; ok {
    tables[i].Indexes = v
}

I would make sure an index of an array/slice exists before you try to access it.
If you haven't already take a look at Effective Go.
If these are very large csv files, maybe take a look at the bufio package.

dphobson:

Thanks.
That's a nice little tweak, I remember seeing that in the tour now you've pointed it out.
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.

Frakturfreund:

This is just a stylistic nitpick, but instead of this C-style 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)
}

you should write in Go
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)
}

It’s shorter and looks more clear. Notice that the Go switch doesn't need break’s, but has a fallthrough keyword instead.

subtlepseudonym:

Both code blocks compile to the same code (IIRC) so it's definitely worth going for readability.

dphobson:

The switch is definitely clear and I'll use that in future

carsncode:

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.
Hope it helps, good luck with your gophering! Hope you're enjoying it!

dphobson:

Thanks.

The panic() call was just a copy/paste from another file I was looking at, that should definitely be handled better.
What do you mean by the following? 


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.

Frakturfreund:

Instead of instantiating your structs like this:
table := ddTables{
    row[1],        // Tablename
    row[2],        // Tablespace
    row[4],        // Comments
    []ddColumns{}, // Empty Columns
    []ddIndexes{}} // Empty Indexes

Do it like this:
table := ddTables{
    Name:       row[1],
    TableSpace: row[2],
    Comments:   row[4],
    Columns:    []ddColumns{},
    Indexes:    []ddIndexes{},
}

The second one is more typesafe: consider someone refactors the ddTables struct and switches the order of Name and TableSpace for consistency with other structs. This will introduce a silent bug in the first code variant, but not in the second (see also Effective Go).
You can still add comments in the second one, but normally the code should speak for itself.
Also note that the final } got it’s own line, and the additional , at the end of the previous line: That’s for clearer diffs.

subtlepseudonym:

On line 228:
tables[i].Columns[j].Primary = v

Can be replaced with
column.Primary = v

And the i and j variables (defined on 221 and 223) can be thrown away because this is the only instance of their use.

dphobson:

Thanks.
I've just tried this and it doesn't appear to work, I changed to the following:
// 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
        }
    }
}

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?
Changing back to this still works and takes into account some of the other suggestions above:
// 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
        }
    }
}

subtlepseudonym:

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.  
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 (:=).  
https://golang.org/ref/spec#For_statements