6 ms·

As this seems to be a beginner project to learn here's some thoughts on the code: 1) You should really not build your json by manually building the string. Tha

by dewey 8y ago

As this seems to be a beginner project to learn here's some thoughts on the code: 1) You should really not build your json by manually building the string. That's very error prone and hard to extend. fmt.Fprintf(f, "{\n\t\"from\": {\n\t\t\"name\": \"%s\", Use https://golang.org/pkg/encoding/json/ https://golang.org/pkg/encoding/json/ (https://gobyexample.com/json https://gobyexample.com/json) instead. Working with json in Go is really one of the nicer features of Go (It's Go, not "GoLang") 2) By setting / typoing a config option you could accidentally delete files from your computer, even your home directory err := os.Remove(jsonfile)

jerf 8y ago
A similar comment for the HTML portions of the message. Use html/template. It's got a couple of quirks (mostly around how it sort of weirdly conflates having one particular template with having a set of templates), but it's one of the safest template languages to just pick up and throw some stuff out; it's quite strong against injection attacks. To assist in using JSON, you'll want to declare a struct holding all your config, at which point it becomes quite surprisingly simple to load it from and save it to a file. Once you have that functionality, I suggest using it for your initial creation as well. I have a program here where I've been using the pattern that I create a default config sample within the program, and if I can't find the config you specify, I print out an error, and the sample JSON file that constitutes a full, legal config for the user's convenience. I wouldn't ship anything that I'd expect to end up in a Linux distro or something that way, but for an internal tool it seems a decent enough pattern. Finally, I'd suggest reddit.com/r/golang is a better sort of place for this sort of thing if you want a review, and there's probably even better places, as I'm not convinced that a karma-based voting site is all that great for reviews for beginners. (It's really easy for a post like that to pick up a couple of early downvotes, and consequently lose all visibility to the people who are willing to help.) But if you're going to use one, /r/golang would be better.
- muzzammildotxyz 8y ago
  Thanks for the suggestion :D I am not shipping it to any distro or something like that. I was using this for myself and just made it open source. Thank you for taking your time and reviewing this. As for /r/golang, I will look into it. :)
abiox 8y ago
i do agree that it makes sense to use the stdlib where possible. however i am a bit wary... my first "real" (but small) project in go was to process some xml. turns out the stdlib parser doesn't handle self-closing tags very well, and would simply not pick up some important content from files i was working with. i would wager the json stuff is probably a much hotter code path so it's probably fine.
- muzzammildotxyz 8y ago
  Thanks for your suggestion. :)
eptcyka 8y ago
It's GoLang, because that's easier to search for.
- dewey 8y ago
  I was just nitpicking, it's not really an important point. People usually prefer "Go" or "Golang", I only mentioned it because I noticed it in the project description.
- muzzammildotxyz 8y ago
  I concur. :D
muzzammildotxyz 8y ago
Thank you for taking your time to review my code. Yes, it is a beginner's project :D 1) How do I format the JSON for readability or should I ignore that? 2) Thanks for that. Didn't know about it. Any ideas on how I can fix that? Maybe I can check if it is a file and not a directory and ask for input again for y/n?
- dewey 8y ago
  You're welcome. 1) What do you mean exactly? The usual way would be to keep your data in a struct like: type configuration struct { Name string `json:"name"` } and then marshall it into a json string like: https://play.golang.org/p/uWvwBY4h03n https://play.golang.org/p/uWvwBY4h03n If you need more details feel free to contact me. Address is in my profile.
  - muzzammildotxyz 8y ago
    No, I meant the readability in .json file. Should I care for that anyway? Also, great website! :D
    - dewey 8y ago
      You don’t have to worry about that and don’t have to manually format it.
      - muzzammildotxyz 8y ago
        https://golang.org/pkg/encoding/json/#MarshalIndent https://golang.org/pkg/encoding/json/#MarshalIndent :D
      - muzzammildotxyz 8y ago
        Please take a look at this: https://github.com/muhammadmuzzammil1998/Log-mailer/commit/fdf8467c4f8e7ea39b58dbd5aad9eff718c6dedb https://github.com/muhammadmuzzammil1998/Log-mailer/commit/f...
    - 8y ago

by dewey 8y ago

jerf 8y ago

A similar comment for the HTML portions of the message. Use html/template. It's got a couple of quirks (mostly around how it sort of weirdly conflates having one particular template with having a set of templates), but it's one of the safest template languages to just pick up and throw some stuff out; it's quite strong against injection attacks. To assist in using JSON, you'll want to declare a struct holding all your config, at which point it becomes quite surprisingly simple to load it from and save it to a file. Once you have that functionality, I suggest using it for your initial creation as well. I have a program here where I've been using the pattern that I create a default config sample within the program, and if I can't find the config you specify, I print out an error, and the sample JSON file that constitutes a full, legal config for the user's convenience. I wouldn't ship anything that I'd expect to end up in a Linux distro or something that way, but for an internal tool it seems a decent enough pattern. Finally, I'd suggest reddit.com/r/golang is a better sort of place for this sort of thing if you want a review, and there's probably even better places, as I'm not convinced that a karma-based voting site is all that great for reviews for beginners. (It's really easy for a post like that to pick up a couple of early downvotes, and consequently lose all visibility to the people who are willing to help.) But if you're going to use one, /r/golang would be better.

muzzammildotxyz 8y ago
Thanks for the suggestion :D I am not shipping it to any distro or something like that. I was using this for myself and just made it open source. Thank you for taking your time and reviewing this. As for /r/golang, I will look into it. :)

abiox 8y ago

i do agree that it makes sense to use the stdlib where possible. however i am a bit wary... my first "real" (but small) project in go was to process some xml. turns out the stdlib parser doesn't handle self-closing tags very well, and would simply not pick up some important content from files i was working with. i would wager the json stuff is probably a much hotter code path so it's probably fine.

muzzammildotxyz 8y ago
Thanks for your suggestion. :)

eptcyka 8y ago

It's GoLang, because that's easier to search for.

dewey 8y ago
I was just nitpicking, it's not really an important point. People usually prefer "Go" or "Golang", I only mentioned it because I noticed it in the project description.
muzzammildotxyz 8y ago
I concur. :D

muzzammildotxyz 8y ago

Thank you for taking your time to review my code. Yes, it is a beginner's project :D 1) How do I format the JSON for readability or should I ignore that? 2) Thanks for that. Didn't know about it. Any ideas on how I can fix that? Maybe I can check if it is a file and not a directory and ask for input again for y/n?

dewey 8y ago
You're welcome. 1) What do you mean exactly? The usual way would be to keep your data in a struct like: type configuration struct { Name string `json:"name"` } and then marshall it into a json string like: https://play.golang.org/p/uWvwBY4h03n https://play.golang.org/p/uWvwBY4h03n If you need more details feel free to contact me. Address is in my profile.
- muzzammildotxyz 8y ago
  No, I meant the readability in .json file. Should I care for that anyway? Also, great website! :D
  - dewey 8y ago
    You don’t have to worry about that and don’t have to manually format it.
    - muzzammildotxyz 8y ago
      https://golang.org/pkg/encoding/json/#MarshalIndent https://golang.org/pkg/encoding/json/#MarshalIndent :D
    - muzzammildotxyz 8y ago
      Please take a look at this: https://github.com/muhammadmuzzammil1998/Log-mailer/commit/fdf8467c4f8e7ea39b58dbd5aad9eff718c6dedb https://github.com/muhammadmuzzammil1998/Log-mailer/commit/f...
  - 8y ago