#Code Review

73 messages · Page 1 of 1 (latest)

bold viper
ashen nexus
#

First off, I'd probably say have one entry point that uses a flag for the database location, with an optional flag to overwrite if it exists already

#

I don't see the value in having a separate binary to initialise the database

#

You're using a global for the database interaction, which in a small project isn't neccearily the be all and end all of bad practices, but it is definitely better to have a struct with methods to better control the state and interaction with it

#

if this is an introductory project, it would be better to get a handle on best practices and avoid using a global

bold viper
#

Hmm okay awesome, thank you for your input @ashen nexus

#

Is there any negative to using map, as opposed to using structs for everything?

#

From what I understand, structs are more performant?

ashen nexus
#

they're very different concepts

#

A map is a key/value lookup, a struct is type that can contain fields and methods

#

If you need to store things that you need to reference later by a known key, then you could use a map

bold viper
#

Ahhh okay

#

Would something like this be a decent use for a map:

response := map[string]interface{}{"ok": false, "error": "failed to retrieve addresses"}

ashen nexus
#

any time you use interface{} or its alias any you're essentially bypassing the type system

#

so you'd usually have to ask yourself, is it absolutely necessary that I forego type safety for this

#

is there context for that snippet above?

bold viper
#

ahhh right, thank you I didn't realize it bypassed the type system

ashen nexus
#

by using interface{} your essentially saying "I have no idea what the type of this thing will be"

bold viper
#

I'm coming from Lua and I'm used to just using tables for everything, but I'm not sure if there's a better way to pass through {"ok":false}

#

Sorry if my terminology is poor, I'm not a professional developer, just a hobbyist

ashen nexus
#

It'll help define the arguments and flags programmatically, and also generate help etc

bold viper
#

What do you mean by outside of the reach of the flags package?

ashen nexus
ashen nexus
bold viper
ashen nexus
#

with urfave/cli it becomes quite easy to do commands and subcommands with their own set of flags and switches, and it'll nicely provide --help for each of them

bold viper
#

Is there another way to do it better, besides using urfave/cli?

#

I'm trying to avoid using external modules where possible, and forcing myself to use the base modules/packages

#

Ah right disregard, you mentioned flags already

ashen nexus
#

Switch case would be the way to go really, but it can be cleaned a bit. Simply inverting the condition at the start and for balance would read better

func main() {

    if len(os.Args) < 2 {
        printHelp()
    }

    switch os.Args[1] {
    case "balance":
        if len(os.Args) != 3 {
            printHelp()
        }
        getBalance()
    case "send":
        sendTransaction()
    default:
        printHelp()
    }
}
#

You should try and keep indentations and nesting as minimal as possible. If there is a way to invert the logic of a condition and return early instead of having to nest or use an else block then it's usually a good idea to do it

#

In those cases above, you would probably want to include an os.Exit(1) in printHelp() or after the calls to printHelp() as it's unlikely you'd want to print help and continue exution.

bold viper
#

Awesome, thank you so much for taking the time to help!

ashen nexus
#

For cases like

func getBalance() {
    resp, err := http.Get(syncNode + "address/" + os.Args[2])
...

it would be much better to pass the required values as function parameters

#

generally speaking, the sooner you can capture external input (whether its config from files, environment variables, command line arguments) and convert them into values represented in your code the better

#

so it would likely end up something like

 case "balance":
        if len(os.Args) != 3 {
            printHelp()
        }
        address := os.Args[2]
        //maybe validate address
        getBalance(address)
[...]
bold viper
#

Hmmm, is that for performance reasons?

#

Or just readability?

ashen nexus
#

When you're looking objects in your code that represent something significant in your domain, like this:

    transaction := map[string]interface{}{
        "pkey":    pkey,
        "address": address,
        "amount":  amount,
    }

It might be worth creating a struct for it, especially if it's used in multiple places throughout. It's likely this would be something in a specific package that this main.go could import. You could have something like

type Transaction struct {
    PrivateKey []byte `json:"pkey"` //? not sure the type of "pkey" from above 
    Address string `json:"address"`
    Amount int `json:"amount"`
}

func (t Transaction) Validate() error {
    // logic to validate the transaction fields, is the address correct, is the Amount a positive integer etc
}
ashen nexus
#

the more you indent, the more context you have to keep in your mind as you're reading the code

bold viper
#

Okay cool, that makes sense

#

I find it really hard to read other people's code, even if I know the language, so that seems logical

bold viper
ashen nexus
#

I've found Go tends to be relatively easy to read, especially as certain style choices are generalyl enforced by gofmt

#

I tend to use structs all the time, the first thing I do when approaching a problem is think about what the data looks like and how it can be defined so I can work with it

#

The next step is, how do I want to interact with this or get it to do stuff. Should it be responsible for its functionality, or is it part of something slightly bigger that should accept these things as function parameters to work on

bold viper
#

So with your first step, would you typically also keep in mind the end result too?

#

Then as you said secondly, it's just a matter of interacting with that to get the desired result

#

I was also wondering if Go has some sort of equivalent to Lua's metatables?

ashen nexus
#

Having a quick look at Lua metatables, there isn't a direct equivalent in Go. The approach in Go would be pretty situational as to what you're trying to achieve

bold viper
#

As in, trying to visualize the output of whatever you're doing with the data? Eg. Setting up structs for a response

round wraith
#

you don’t need to add new dependencies like ufav for flags (nor cobra)

limber helm
bold viper
round wraith
#

np. I usually put the import and then go mod tidy (skipping go get)

bold viper
round wraith
#

yes leftmost is best, avoiding unecessary else {} also

bold viper
#

Okay awesome

bold viper
#

im about to work on using databases for structs and getting rid of interface so it's type safe now - by methods of the struct to control the database, would you just do something like this?

# 
type DB struct {
    conn *sql.DB
}

func (db *DB) Create(name string, age int) error {
    query := "INSERT INTO users (name, age) VALUES ($1, $2)"
    _, err := db.conn.Exec(query, name, age)
    if err != nil {
        return fmt.Errorf("failed to insert record: %w", err)
    }
    return nil
}
round wraith
#

you installation instructions

# 
git clone https://github.com/Hypnophobe/go-cash && cd go-cash && go build .

can be changed to much simpler/direct:

go install github.com/hypnophobe/go-cash@latest
bold viper
#

ah okay cool

round wraith
#

and add /wallet/ for the other binary I guess

bold viper
#

yeah ill fix that up eventually, just trying to get this db shit sorted out rn

#

I want to add more features before I bother adding the wallet to the readme as well

#

I still need to write a basic miner