<p>Hello,</p>
<p>i started on a little project which is supposed to help my workflow in the future. As a developer i'm anything but experienced, hence i thought it might be smart to get some feedback before i continue.</p>
<p>First a brief overview of what i want to achieve on the long run: </p>
<p>i want to write a small CLI tool that automatically syncs my postgres database with my projects models and also auto-generates getters/setters.</p>
<p>What it does right now: reads a model file, f.e. User.go and let's me generate functions for every struct field. So i get fieldnames, types and tags.</p>
<p><a href="https://gist.github.com/anonymous/8e09e4715e08edbbb8d288b1ebf82f75" rel="nofollow">Code</a></p>
<p>Any feedback or suggestions?</p>
<p>Regards</p>
<hr/>**评论：**<br/><br/>NeedsMoreTests: <pre><p>It's hard to do code review on Reddit so the very first thing I'd suggest is to host your code somewhere or use a service list pastebin/gist/etc. That will give people line number and properly format your code too.</p>
<p>Aside from that, here are some suggestions (sorry for the bold, I don't mean to yell it just makes formatting easier):</p>
<p><strong>Structs should go outside of the function body</strong></p>
<p><strong>Don't use else in an if statement unless you really need to. This is cleaner:</strong></p>
<pre><code>dat, err := ioutil.ReadFile("./models/" + modelfile + ".go")
if err != nil {
panic(err)
}
[ ... ]
</code></pre>
<p><strong>Use <code>filepath.Join</code> to join paths, not <code>+</code></strong></p>
<p><strong>This section</strong></p>
<pre><code>if x.Name == "models" || x.Name == modelfile {
} else {
if property == 0 {
newfield.name = x.Name
property++
} else if property == 1 {
newfield.fieldtype = x.Name
source = source + "\n//Get" + newfield.name + " function\nfunc Get" + newfield.name + "() " + newfield.fieldtype + " {\n fmt.Println(\"test\") \nreturn 1\n}\n"
property++
}
}
</code></pre>
<p><strong>...would be cleaner if it looked something like this</strong>:</p>
<pre><code>if x.Name == "models" || x.Name == modelfile {
break
}
switch property {
case 0:
[ ... ]
case 1:
[ ... ]
}
</code></pre>
<p>Making code readable is nearly as important as it being functional. One because you read code more often than you write it and it also makes it harder to make silly mistakes. </p>
<p>Based on your description and what you've written you might consider a different approach. It would seem you're trying to automate the process of writing code which more times than not is the wrong approach to solving the problem. For example you could write an interface that would take field name and then use that in the model to return the information you need. You also write something that takes a struct and then does something based on how you've annotated a field. Like <code>json.[Marshall|Unmarshall]</code> would do with this:</p>
<pre><code>// instead of 'json' you could have something else here.
type Foo struct {
Bar string `json:"bar"`
}
// Google's protobuf library for example makes extensive use of this approach.
// 0 - field number
// opt - means the field is not required
// name - the name of the field in the protobuf
type Foo struct {
Bar string `protobuf:"string,0,opt,name=bar" json:"bar,omitempty"`
}
</code></pre>
<p>Parsing syntax trees is certainly a useful skill to learn but I don't think it's the appropriate solution in this case. It's kind of like handling user input, don't trust it and expect that at some point it will break your code. While this might work for you short term you'll eventually have to refactor it as you want to add more which overall will probably take you more effort than figuring out a better way of handling this.</p></pre>FPSports: <pre><p>Hey, thanks a lot!</p>
<p>Could you elaborate a bit further on why to use filepath.Join?</p>
<p>I've updated the code with your suggestions.</p>
<p>Regarding the general approach: i figured writing getteres and setters automatically might not be the best use case, since creating an interface for this task that applies i can use for all models would be better. I kinda struggle with interfaces in general... </p>
<p>The next bigger step would be to check if the according database table is still up-to-date based on the model or i need to alter it. So f.e. i want to do:</p>
<p>iterate over the "newfield" my current code generated -> check if table with name of the model exists -> check if columns are still up to date based on newfield.name/type/tag.</p>
<p>I don't think i can do this with interfaces, can i? </p>
<p>Regards</p></pre>NeedsMoreTests: <pre><blockquote>
<p>Could you elaborate a bit further on why to use filepath.Join?</p>
</blockquote>
<p>Sure, there's a few reasons</p>
<ul>
<li>It's cleaner. <code>filepath.Join(a, b, c)</code> vs <code>a + "/" + b + "/" + c</code></li>
<li>It's more consistent. In the example above, you <code>a</code> could be something like <code>foo/</code> in which case <code>+ "/"</code> is unnecessary. There are other things it does too, I'd read the docs for more information: <a href="https://golang.org/pkg/path/filepath/#Join" rel="nofollow">https://golang.org/pkg/path/filepath/#Join</a></li>
<li>It's cross platform</li>
</ul>
<blockquote>
<p>I kinda struggle with interfaces in general...</p>
</blockquote>
<p>To be honest, I did too until recently. Coming from a Python/C++/Java background interfaces were a bit odd. Here's a minimal example which might help with the understanding: <a href="https://play.golang.org/p/sXkiAW9ReY" rel="nofollow">https://play.golang.org/p/sXkiAW9ReY</a></p>
<p>In the example code the <code>Bob</code> and <code>John</code> structs implement the <code>Name()</code> function. The <code>Person</code> interface stats that any struct which implements <code>Name()</code> is also a person. This is a simplified view of what's actually going on under the hood of course but hopefully it's enough to give you something to test/play around with.</p>
<blockquote>
<p>I don't think i can do this with interfaces, can i?</p>
</blockquote>
<p>I'm sorry, I don't know. It's still a little difficult to tell what you're trying to implement without seeing the code you're thinking off. Interfaces may not be the right solution but there are probably better solutions than parsing syntax trees too. Here's my suggestion, write some tests for your existing code especially around edge cases which could break your existing design. If it's easy to test and your tests don't need to be modified or rewritten for every model you have then maybe the current approach is an ok solution. If you can't write tests then your current design may be incomplete or not be sufficiently testable in which case you might need another solution.</p></pre>FPSports: <pre><p>What's the benefit of using an interface there instead of just <a href="https://play.golang.org/p/Qn-kBK0bhT" rel="nofollow">this</a>?</p></pre>acdenisSK: <pre><p>edit: threw everything away and started from scratch because i started to get confused whether my comment actually answered your question.</p>
<p>Because this approach allows for virtually anything to be a <code>Person</code> without the hassle of maintaining structs that are <em>supposed</em> to be people.</p></pre>NeedsMoreTests: <pre><p>It's a simple example, there's basically no advantage in this case. </p>
<p>The main point behind the example was to show that you can use an interface to pass around a common type instead of a specific type in a function call. Basically interfaces allow you to provide abstractions: <a href="https://play.golang.org/p/GzNmf1M31C" rel="nofollow">https://play.golang.org/p/GzNmf1M31C</a>. So in your code you could have something like:</p>
<pre><code>type Model interface {
Set(name string, value interface{}) error
Get(name string) (interface{}, error)
}
</code></pre>
<p>So any struct which implements the above interface could figure out how to handle Set/Get internally. <code>interface{}</code> allows you to handle 'any type' instead of having to be explicit. So you will have to do the type conversion yourself but I feel like this approach is cleaner than parsing the syntax tree and generating functions. On a side note about code generation, I neglected to mention that <code>go generate</code> is pretty standard. So if you do still want to go that route, that's probably how you should be generating the code.</p></pre>FPSports: <pre><p>Thanks for your answers man. Appreciated a lot. What you wrote above actually came to my head few hours later as well. So i basically wouldn't need to write getters and setters for every field in a struct ... </p>
<p>Regarding the code generation .... i'd rather avoid it in general for my purposes tbh. And for this little tool i won't need code generation if i implement getters/setters through an interface.</p></pre>NeedsMoreTests: <pre><p>Anytime, glad I could help! </p>
<p>Code generation by itself can be a powerful tool and is sometimes necessary too, like when using Protobufs, but it certainly has some drawbacks to it too. Picking the right tool for the job just takes time and experience...especially when learning a new language.</p>
<p>Best of luck!</p></pre>
