Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Coding guideline] Add implementation and grouping section #641

Merged
merged 14 commits into from
Aug 27, 2020
182 changes: 163 additions & 19 deletions docs/contributing/coding-style.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@ Code formatting and naming conventions affect coding readability and maintainabi

But having tools to format source code doesn't mean you do not need to care the formatting of the code, for example:

<table>
<thead><tr><th>Bad</th><th>Good</th></tr></thead>
<tbody><tr><td>

```go
// bad
badStr := "apiVersion: v1\n" +
"kind: Service\n" +
"metadata:\n" +
Expand All @@ -24,8 +27,11 @@ badStr := "apiVersion: v1\n" +
" port: 3000\n" +
" targetPort: 3000\n" +
" protocol: TCP\n"
```

// good
</td><td>

```go
goodStr := `apiVersion: v1
kind: Service
metadata:
Expand All @@ -39,6 +45,8 @@ spec:
`
```

</td></tr></tbody></table>

### Project Layout

The project layout includes the folder and the file structure in the project. We follow the [project-layout](https://github.com/golang-standards/project-layout) example in Vald.
Expand Down Expand Up @@ -90,9 +98,79 @@ All packages should contain `doc.go` file under the package to describe what is
package cache
````

### General style

kevindiu marked this conversation as resolved.
Show resolved Hide resolved
This section describes the general guideline for the Vald programming style, every Vald contributor should keep these general guidelines in mind while working on the implementation of Vald.

#### Order of declaration

Put the higher priority or frequently used declaration on the top of other declaration.
It makes Vald easier to read and search the target source code in Vald.

For example, the interface declaration should have higher priority than struct or function declaration, hence it should be put above other declaration.

<table>
<thead><tr><th>Bad</th><th>Good</th></tr></thead>
<tbody><tr><td>

```go
type S struct {}

func (s *S) fn() {}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved

type I interface {}
```

</td><td>

```go
type I interface {}

type S struct {}

func (s *S) fn() {}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
```

</td></tr></tbody></table>

#### Group similar definition

Group similar definitions such as struct or interface declaration.
We should not group interface and struct declaration in the same block, for example:

<table>
<thead><tr><th>Bad</th><th>Good</th></tr></thead>
<tbody><tr><td>

```go
type (
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
I interface {}
I2 interface {}

kevindiu marked this conversation as resolved.
Show resolved Hide resolved
s struct {}
s2 struct {}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
)
```

</td><td>

```go
type (
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
I interface {}
I2 interface {}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
)

type (
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
s struct {}
s2 struct {}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
)
```

</td></tr></tbody></table>

### Interfaces

Interface defines the program interface for usability and future extendability.
The interface defines the program interface for usability and future extendability.
Unlike other languages like Java, Go supports implicit interface implementation. The type implements do not need to specify the interface name; to "implements" the interface the structs only need to defined the methods the same as the interface, so please be careful to define the method name inside the interface.

The interface should be named as:
Expand Down Expand Up @@ -303,46 +381,53 @@ Please use [internal/errgroup](https://github.com/vdaas/vald/blob/master/interna
All functions return `error` if the function can fail. It is very important to ensure the error checking is performed.
To reduce human mistake that missing the error checking, please check the error using the following style:

<table>
<thead><tr><th>Bad</th><th>Good</th></tr></thead>
<tbody><tr><td>

```go
// good
if err := fn(); err != nil {
err := fn()
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

// handle error
}
```

Instead of this style.
</td><td>

```go
// bad
err := fn()
if err != nil {
if err := fn(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

// handle error
}
```

</td></tr></tbody></table>

If you need the value outside the if statement, please use the following style:

<table>
<thead><tr><th>Bad</th><th>Good</th></tr></thead>
<tbody><tr><td>

```go
// good
conn, err := net.Dial("tcp", "localhost:80")
if err != nil {
if conn, err := net.Dial("tcp", "localhost:80"); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
Add a space between sentences (SENTENCE_WHITESPACE)
Suggestions: Dial
Rule: https://community.languagetool.org/rule/show/SENTENCE_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
Possible typo: you repeated a whitespace (WHITESPACE_RULE)
Suggestions:
Rule: https://community.languagetool.org/rule/show/WHITESPACE_RULE?lang=en-US
Category: TYPOGRAPHY

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

// handle error
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

// use the conn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
After 'the', do not use a verb. Make sure that the spelling of 'conn' is correct. If 'conn' is the first word in a compound adjective, use a hyphen between the two words. Note: This error message can occur if you use a verb as a noun, and the word is not a noun in standard English. (A_INFINITIVE[1])
Rule: https://community.languagetool.org/rule/show/A_INFINITIVE?lang=en-US&subId=1
Category: GRAMMAR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
Don't put a space before the closing parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: }
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

}

// use the conn
```

Instead of this style.
</td><td>

```go
// bad
if conn, err := net.Dial("tcp", "localhost:80"); err != nil {
conn, err := net.Dial("tcp", "localhost:80")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
Add a space between sentences (SENTENCE_WHITESPACE)
Suggestions: Dial
Rule: https://community.languagetool.org/rule/show/SENTENCE_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
Don't put a space after the opening parenthesis (COMMA_PARENTHESIS_WHITESPACE)
Suggestions: {
Rule: https://community.languagetool.org/rule/show/COMMA_PARENTHESIS_WHITESPACE?lang=en-US
Category: TYPOGRAPHY

// handle error
} else {
// use the conn
}
// use the conn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LanguageTool] reported by reviewdog 🐶
After 'the', do not use a verb. Make sure that the spelling of 'conn' is correct. If 'conn' is the first word in a compound adjective, use a hyphen between the two words. Note: This error message can occur if you use a verb as a noun, and the word is not a noun in standard English. (A_INFINITIVE[1])
Rule: https://community.languagetool.org/rule/show/A_INFINITIVE?lang=en-US&subId=1
Category: GRAMMAR

```

</td></tr></tbody></table>

### Logging

We define our own logging interface in [internal/log package](https://github.com/vdaas/vald/blob/master/internal/log). By default we use [glg](https://github.com/kpango/glg) to do the logging internally.
Expand All @@ -356,6 +441,65 @@ We defined the following logging levels.
| ERROR | The message that indicates the application is having a serious issue or,<br>represent the failure of some important going on in the application.<br>It does not cause the application to go down.<br>Someone must investigate the error later. | Failed to insert an entry into the database, with retry count exceeded.<br>Failed to update the cache, and the cache is not important. | User 1 is failed to insert in the database, errors:<br>retry count: 1, error: ErrMsg1, retry count: 2, error: ErrMsg2, .... |
| FATAL | Message that indicate the application is corrupting or having serious issue.<br>The application will go down after logging the fatal error. <br>Someone must investigate and resolve the fatal as soon as possible. | Failed to init the required cache during the application start. | |

## Implementation

This section includes some examples of general implementation which is widely used in Vald.
The implementation may differ based on your use case.

### Functional Option

In Vald, the functional option pattern is widely used in Vald.
You can refer to [this section](#Struct-initialization) for more details of the use case of this pattern.

We strongly recommend the following implementation to set the value using functional option.

```go
func WithVersion(version string) Option {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
return func(c *client) error {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
if len(version) != 0 {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
c.version = version
}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
return nil
}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
}
```

We recommend the following implementation to parse the time string and set the time to the target struct.

```go
func WithTimeout(dur string) Option {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
func(c *client) error {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
if dur == "" {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
return nil
}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
d, err := timeutil.Parse(dur)
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
return err
}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
c.timeout = d
return nil
}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
}
```

We recommend the following implementation to append the value to the slice if the value is not nil.

```go
func WithHosts(hosts ...string) Option {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
return func(c *client) error {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
if len(hosts) == 0 {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
return nil
}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
if c.hosts == nil {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
c.hosts = hosts
} else {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
c.hosts = append(c.hosts, hosts...)
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
return nil
}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
}
```

## Program comments

Program comments make easier to understand the source code. We suggest not to write many comments inside the source code unless the source code is very complicated and confusing; otherwise we should divide the source code into methods to keep the readability and usability of the source code.
Expand Down