Changes #1
Changes #1
Conversation
jrivany
left a comment
jrivany
left a comment
There was a problem hiding this comment.
PR title and description need some work, not up to snuff.
cmd/mlbviolations/main.go
Outdated
| godotenv.Load() | ||
|
|
||
| db, err := sql.Open("mysql", | ||
| fmt.Sprintf("%v:%v@tcp(%v:%v)/%v", os.Getenv("DB_USER"), os.Getenv("DB_PASS"), os.Getenv("DB_HOST"), os.Getenv("DB_PORT"), os.Getenv("DB_TABLE"))) |
There was a problem hiding this comment.
| fmt.Sprintf("%v:%v@tcp(%v:%v)/%v", os.Getenv("DB_USER"), os.Getenv("DB_PASS"), os.Getenv("DB_HOST"), os.Getenv("DB_PORT"), os.Getenv("DB_TABLE"))) | |
| fmt.Sprintf("%v:%v@tcp(%v:%v)/%v", os.Getenv("DB_USER"), os.Getenv("DB_PASS"), os.Getenv("DB_HOST"), os.Getenv("DB_PORT"), os.Getenv("DB_DATABASE_NAME"))) |
There was a problem hiding this comment.
Naming wise, your connecting to a specific database hosted on that MySQL server and then running queries that have all the table data.
itsjamie
left a comment
itsjamie
left a comment
There was a problem hiding this comment.
https://go.dev/doc/database/sql-injection
Right now the fmt.Sprintf can be changed to use the positional argument syntax for your database driver, so in this case, you would use ? for the SQL.
cmd/mlbviolations/main.go
Outdated
| log.Fatal(err) | ||
| } | ||
|
|
||
| return seasonTotal |
There was a problem hiding this comment.
So I think in a more idiomatic go world, you would return the err and the total (potentially nil) here and log fatal in the calling code.
Then all the exit points are clear from main
|
If you start to find building your tweets from |
internal/custom-structs.go
Outdated
|
|
||
| // grab the team total for the given code | ||
| var teamTotalType int | ||
| err = db.QueryRow(fmt.Sprintf("SELECT COUNT(*) FROM player_stats WHERE team_name = '%s' AND code = '%s'", team, code)).Scan(&teamTotalType) |
There was a problem hiding this comment.
you can use prepared statements for these too ;)
internal/custom-structs.go
Outdated
|
|
||
| func saveViolation(db *sql.DB, code string, p Player, team string, date string) (int, int, int, int) { | ||
| table := os.Getenv("DB_TABLE") | ||
| stmt, err := db.Prepare(fmt.Sprintf("INSERT INTO %s(player_id, player_name, team_name, code, date) VALUES(?, ?, ?, ?, ?)", table)) |
There was a problem hiding this comment.
It doesn't really matter to the impact or quality of the code, but because you are using placeholders here, Go would automatically prepare the statement. It also doesn't change the performance at all. You could validate this by using Wireshark or something and watching the traffic to the database, it wouldn't change between the prepare + exec, versus an Exec with the statement with placeholders. The same network traffic will end up being sent.
See: https://dev.mysql.com/doc/refman/8.0/en/sql-prepared-statements.html
In particular...
A prepared statement is specific to the session in which it was created. If you terminate a session without deallocating a previously prepared statement, the server deallocates it automatically.
A prepared statement is also global to the session. If you create a prepared statement within a stored routine, it is not deallocated when the stored routine ends.
So that's what the server will do as Go when you are using the db API uses a connection pool, and will establish multiple connections.
Now, if you were to retrieve a connection, and run the same queries, you could use the Prepare API, and reuse the driver.Stmt that is returned to make sure you doesn't get automatic deallocations depending on if the driver you're using deallocates the automatic prepared statements immediately after use because perhaps they don't name the statements, and they are an unnamed prepared statement, which only stays prepared until the next statement. The Postgres driver lib/pq iirc does this.
changes to add additional data to tweets with team and player totals