Golang Code : Review Guides

Issues found Frequently

New lines
Double newlines
No space after //
DRY principle violations
Magic String/Number
Error Shallow
Naming convention not following

Guides

[General] Never submit comment code-
[General] Commnet should have space after //, eg. // This my comment not //this is my comment
[General] Naming convention must.
- Override by project guide-lines.
- Naming Convention
[General] Every variable should be assigned only once, the best case senario. Try to have const or immutable object.
- At the worst case can be a loop take data or slice appending item can have multiple assigns.
- Or also, if variables changed, it should be done in one method, not in multiple.
  - Except for the design pattern implementations
[General] No magic string, or number, make constant for each magic number or string. ("…anything", 1, 2). Use constant to resolve this review rejection. Unless language has some sort of overruling.
[General] There should be a newline (New Line Code Style Examples)
- [General] After any curly brace }, but not with consecutive ones
- [General] After comma [,], if there is more than 2 parameters or arguments in function calling or in function definition or contructor injection or in property assigning. (JavaScript can have some exceptions, will be added in future.)
- [General] Before any return if there are multiple lines in the method or within the block.
  - Exception: If a single line in the method then no newline before.
  - Exception: If any if statements returns and there is a single line in it then no newline. However, if the if statement block contains more than one line, there should be a new line.
No new line (New Line Code Style Examples)
- [General] Consecutive curly braces or parentthesis.
- [General] After Return Statement
- [General] After Newline (that mean no double new line in the code)
Function guideline must
[General] Avoid multiple nesting or branching, to resolve this we can use function or inversing logic or taking variable approach.
- How to reduce code nested brancing in golang
[Golang] DON'T use more than one defer in one function as defer works as stack and it becomes complicated in future code maintain. If use it use it, on top or bottom.

Function Guideline

Function should be small, follow these principles DRY principle and SOLD principle wiki SOLID.

[Golang] Should have proper naming, what it returns.
[General] Function should not(❌) exceed 3 arguments or parameters.
- Exceptions :
  - Frameworks can have more (✅) than 3 args it is fine.
  - Frameworks need to fit all kinds of things, thus it can have it.
[General] Method Documenting: Usually simple methods doesn't require method documenting. However, if a method is doing something different (bad practice) or something implicit (bad practice) which cannot be directly explained with the naming then method documenting is a must. (The best approach is always to explain everything in naming if possible), these below cases method documenting is good and must.
- Processing some data, an example can be added to the method. For example, path.Clean does path cleaning and normalizing what it does.
- Or, If we are taking someone else's code then we should cite it as a must.
- Or, the team prefers to have the doc to generate automatic documentation from the codebase at any time. [Which can be the most important factor in method documenting]
- [Golang] Method documenting example starts with // on top of the method with no newline in between method and the docs
Golang:
- [Golang] Getters:
  - Should have Field() not GetField()
- [Golang] Setters:
  - Should have SetField(..) not Field(...)
- [General] Any other method name can have Get which is fine.
- [Golang] Constructor methods always starts with New[StructName]
- [General] Try not to return more than 2 objects.
[General] Try to have less than 8 lines of code in one method, at worst case, the scenario could be 15 lines, and the method should not exceed 25 lines (which can be referred to as broken-fix-code).
- This can be overruled while creating a framework function or some function that is memory intensive and require optimization. Making small function adds more function stack, thus memory cost and CPU counter cost as well.
[Golang] Exporting / public method should have CamelCase.
[Golang] Non exported / private methods should have lowerCamelCase.
[General] (Doesn't apply in golang) to apply SOLID easily:
- Every class should be inherited from at least a single Interface and a single interface should hold only a single method.
- Now class which requires more than one method should inject or requires those interfaces of methods which required to be available in the class.
- This class will be in solid principal and will be short less than 60 lines of code and maintainable.
[Golang] Unit-Test names should start with Test...

Naming Convention

[General]: Can always be overridden by Project guidelines or needs or reviewers preferences.
[General] Is..., Has..., Can.., Should.., (lower priority at the end Does..) methods or variables should always returns a bool.
- Is... is highly recommend unless it doesn't go with the grammar (or may sound odd with Is... then other versions are recommended line Has... or Can...).
[Golang]: Don't use any underscore anywhere.
[General] Singular item variable should NOT have plural symbol like es,s or something like plural that represents multiple (array, list, vector, slice, enumerable items, iterable) things.
- Example: a single person object should NOT be named as people, or persons, it should be named as employeePerson or something similar to that.
- Exception case can be OsGroup, as OsGroup can be represented as a single item, like UnixGroup(Mac, Linux,..), WindowsGroup, in this case, the group is name is appropriate.
  - When we say OsGroups, it should represent an array of OsGroup items
[General] Multiple item variable ( refers to array, arraylist, linked list, list, vector, slice, enumerable items, iterable) things should NOT have singular symbol like result, person, employee but plural like results, people, OsGroups
[General] If we have a map then it would be better to represent it with resultsMap as the naming suggest what we are dealing with in the future.
[Golang] Variable naming:
- lower camel case: helloWorld not helloworld, hello_world
- no underscores
[Golang] Const naming:
- Camel case: eg. HelloWorld, NOT (helloWorld, HELLOWORLD, HELLO_WORLD, hello_world, helloWorld)
- no underscores
  - GOlang's Constant, UpperCamelCase, Only POSIX const can have underscore which is an exception | DO NOT follow

Package naming convention (golang-package-naming)

lowercase all: eg. helloworld not helloWorld, hello_world
should not have like package/package, that means, we are expecting a package name taskrunner we should not have methods taskrunner.taskrunner.DoSomething(). Go naming should be taskrunner.DoSomething()
folder and file name:
- It can be camelcase or all lower case or having hyphens or having underscores for test files.
- I would prefer hyphens over others (eg. underscore, camelcase) for folder and file names, which looks good, in terms of GitHub.

Make member - Lazy or LazyExecution 🐼🐌

Lazy means doesn't execute until needed. Don't be hungry. Lazy codes acts as caching. Basic principal in caching (Generate once and serve same data to many.)

Lazy should be applied on fields (any of the cases):

Heavy lifting code or heavy process.
Static value, doesn't change.
Or, Same data is asked by everyone.
60%+ cases don't need to execute at all.

Lazy should NOT be applied:

Code is changing repeatedly. Data is not same as per request.
- Func may require param to serve different things.

Examples of usecase

Database query could be lazy. Only excute at the final stage
Builder patterns in most cases lazy, takes instructions and executes by Build Command at last.

Converting from NonLazy to Lazy Code

Make non export field, expose as getter.
In the getter method check with flag or nil that the member is not generated.
Generate member in the if block and then assign to the non export field and return the field.
In case of working with slice or array, use lock mechanism using mutex.
Everytime the field is in used should be called from the method NOT the direct field.
If the required field is lazy, the current field should be lazy as well.
- Dangerous Code Issue, must be avoided

FROM





type Group struct {
	GroupID   string
	GroupName string
	Members   *UsersCollection // make member lazy
}






































type Group struct {
    GroupID   string
    GroupName string
    // make  Members export method, just like getter
    members   *UsersCollection // make member non-export
    // use lock if working with collection type or slice
    sync.Mutex
}

// must use the method rather than field
func (g *Group) IsMembersEmpty() int {
    return g.MembersLength() == 0
}

// must use the method rather than field
func (g *Group) MembersLength() int {
    return g.Members().Length()
}

func (g *Group) Members() *UsersCollection {
    if g.members == nil { // flag or nil check
        // generate
        members := "generator method or run something complex to return same data in future."
        // assign back to the members field
        g.members = &members
    } 

    // return the non-export field.
    return g.members
}

// Lock method for async usecase
func (g *Group) MembersLock() *UsersCollection {
    g.Lock()
    defer g.Unlock()
    
    return g.Members()
}

Lazy Code References

How to use Regex in Golang

Regex is complex and usages backtrack programming to find matches, it is extremly expensive. It should be delt with care and caution. Regex should be the last resort of programming to detect some pattern in string.

Regex should NOT be used:

Searching dots or comma and then trimming each word.
Search for specific text in each line.
- Search starts with for each line.
- Search for ends with in each line.
- Search number in line.
  - It depends if something is common then first cut out the specific part of the string which is common and then apply the regex on the dynamic string to find the pattern.

Regex should be used (must use with care):

Search dynamic text with pattern.
Code or syntax parsing.
Ignoring whitespace and finding a match with required pattern.
Where searching becomes too complex with for->for->for like bigO(n^3) then must use regex.

How to use it

Regex must be compiled in a var outside of func.
- Must used in var for golang in the constants sections.
- Use and add sample data with regex for example:
GoTooling in Action - Improve Benchmarking by Putting Regex into var rather inside function
If regex required to run in a loop or function verify with your mentor or reviewer.
- Don't write code and then get rejection.

Using Newline in Output or Splitting Confusing

It is confusing to use which newline from constant should be a priority and what to use. As os, has specific new line standards (Windows - "\r\n" and Unix "\n")

In 90% of the cases, NewLineUnix (constants.NewLineUnix
) should be used. The only case, where constants.NewLine should be used where we are asking the user for NewLine preference based on Operating System. An example from IDE can help understand this

If a file has \r\n and if we read using \n, problems can remain depending on the requirements. However, if we are parsing parse only lines without whitespace then it can be fixed at some level.

Choosing a newline preference always depends on the situation. In most cases, we should use \n constants.NewLineUnix

It is still debatable and can differ.

Draft Notes

private 
func nonExport

public
func Export

x _ don't use

the name should be meaningful.


What makes a good function:
1. Function name should be self-explanatory.
    - Function Name Short (argument) : OpenMail
    - Function Name Full (argument) : OpenSmtpProtocolMail
    - Function Combined (argument) : ReadWriteFile
        - Single Responsibility (SOLID)
            - ReadFile
            - WriteFile
2. Function should not be more than 15 lines of code. Worst case can have 25
3. Function should not have more 3 parameters
    - Interface (Animal (eat, walk, run)-> Dog, Person) - I don't care how you do it, I should have the ability to do it.
    - Struct (Person -> can have attributes of Animal)

4. Struct or class or file should not be more than 120 lines
5. Returns one item pass (good)
    - Good: Array, Map -> Interface, Struct
        -> Array, map def could change.
    - ...

Why we are writing more code?

funcA(a1Struct)

funProcess() {
    v = funcA(a1Struct)
    v...
}


funProcess2() {    
    funcA(a1,.....a6)
}

funProcess3() {    
    funcA(a1,.....a6, a)
}

funProcessN() {

    funcA(a1,.....a6)
}

Problem
 - Tedious Change in the future.
 - Chances of breaking.
 - Time-consuming
 - No guarantee to run successfully in future


Useful:
- Saves time
- Makes code more compact
- Success chances increase

New Line Code Styling

Sample

func Something() int {
  return constants.One // alright
}

Issue

func Something() int {
  doSomething()
  return constants.One // NOT Alright
}

func Something() int {
  doSomething()

  return constants.One // Alright
}

Issue

func Something() int {
  doSomething()
  if isNumber {
      doSomethingNew()
      return constants.One // NOT alright
  }
}

func Something() int {
  doSomething()
  if isNumber {
      doSomethingNew()

      return constants.One // alright
  }

  return constants.MinusOne // alright
}

No new line in between curly braces if there is no code

func Something() int {
  if isApplicable1 {
      if isApplicable2 {
          return constansts.One // alright
      }  // alright no new line recommended
      // there should be no newline
  }  // NOT alright no new line recommended
}

No new line in between curly braces if there is no code

func Something() int {
  if isApplicable1 {
      if isApplicable2 {
          return constansts.One // alright
      }  // alright no new line recommended
  }  // alright no new line recommended
}

No new line at the start of the function or double new lines together

func Something() int {
                             // <- NOT alright, however can put comment at the first line, which is fine.
  doSomething()
                             // <- NOT alright double newline
                             // alright here because of the return statement
  return constants.One 
}

Multiple nesting avoid examples

Issue with nesting

Code nesting or branching makes the code complex (even if it is simple), consequently, we should always avoid code branching or nesting at our best. It helps to write simple unit tests and improves code maintainability in future.

Issue occur: Multiple levels of code nesting or branching using ifs, it can also be for, if, while or other blocks nesting.

Exception case, it is quite alright to have one loop and then a if statement branch. But not more than that. Moreover, to be specific this exception case doesn't apply on multiple if statements.

func (wiki *WikiTimezone) Longitude() string {
	if len(wiki.LatLong) > 1{
		if len(wiki.LatLong) > 11 {
			return string(wiki.LatLong[7:])
		} else {
			return string(wiki.LatLong[5:])
		}
	}

	return ""
}

To resolve this issue we can use, we can use any of the below methods

Resolve nesting using method
















// remember to keep `is` as your method prefix or can or has or should. `is` prefered as first option.
func isLatLongLengthGraterThanEleven(wiki *WikiTimezone) bool {    
    return len(wiki.LatLong) > 11
}

func (wiki *WikiTimezone) Longitude() string {
    if isLatLongLengthGraterThanEleven(wiki) {
        return string(wiki.LatLong[7:])
    } else if len(wiki.LatLong) > 1 {
        return string(wiki.LatLong[5:])
    }

    return ""
}

Resolve nesting using inverse logic




















// remember to keep `is` as your method prefix or can or has or should. `is` prefered as first option.
// Since this empty reprsent not taking less than 4 length we should keep the name as per the logic, however, for the example case we are keeping it as is.
func isEmpty(wiki *WikiTimezone) bool {    
    return len(wiki.LatLong) <= 4
}

func (wiki *WikiTimezone) Longitude() string {
    // negative logic comes first
    if isEmpty(wiki.LatLong) {
        return ""
    }

    if len(wiki.LatLong) > 11 {
        return string(wiki.LatLong[7:])
    }
    
    return string(wiki.LatLong[5:])
}

Notes: We haven't used the else part the in the last return because it is redudant. We should always try to avoid else if it is not necessary.

Example of Resolving Nested branching on Complex Code (level 4 to level 1)

How to reduce code nested brancing in golang

Resolve nesting using inverse logic, variables and ternary operators

Golang doesn't have any ternary operators so assume that it has and the process may look like this below:


// remember to keep `is` as your method prefix or can or has or should. `is` prefered as first option.
func isEmpty(wiki *WikiTimezone) bool {    
    return len(wiki.LatLong) <= 0
}

func getLatLongSplitAtFive(wiki *WikiTimezone) string {    
    return len(wiki.LatLong) > 5 ? string(wiki.LatLong[5:]) : ""
}

func (wiki *WikiTimezone) Longitude() string {
    // negative logic comes first
    isEmptyLatLong := isEmpty(wiki.LatLong)
    length := len(wiki.LatLong)
    isApplicableForSplitAtEleven := length > 11
    
    // final result can be returned with ternary operator but golang doesn't have tarnary, however, assume we have it and then we can do it.
    splitAtFiveOrEmpty := isEmptyLatLong ? "" : 
    applicableResult := isApplicableForSplitAtEleven ? 
    string(wiki.LatLong[7:]) : getLatLongSplitAtFive(wiki)
    
    /**
      * Warning: don't write nested condition inside ternary operator ever, that is another red.       
      * this code is very simple in terms of unit testing
      * Unit testing becomes complicated once code branching or nesting introduced.

      * However, as go doesn't have ternary operator we cannot use it.
      * Which is good thing, ternary operator sometimes gets abused and it is easy to read if-else than ternary operators.
      * Summary: For golang we should use `Resolve nesting using inverse logic`, plus, we can use the method combination as well.
    **/
    
    return applicableResult    
}

Avoid `else` if not necessary

Issue








func getValue(val []string, index int) string {
	if val[index] != "" {
		return val[index]
	} else {
		return "-"
	}
}








func getValue(val []string, index int) string {
	if val[index] != "" {
		return val[index]
	} 
                // <-- must have a newline
    return "-"  // no need to have else part.
}

Code Red, Dangerous: Null Pointer Issue

Should NOT call directly a return value from a function unless it is a constructor or gurateed to have the result except nil.
Every return pointer value should be tested not nil before calling or access any member (field or function / method) of the struct.
Before making a variable to pointer, must check if the variable is not nil.







// returns risky pointer which may break in real world.
fun anyBytesRickyPtr() *[]Byte {
    anyBytes = getBytes(...) // []Byte

    return &anyBytes // send pointer of []Byte is dangerous.
}










// solution to pointer return
fun anyBytesSafePtr() *[]Byte {
    anyBytes = getBytes(...) // []Byte
    
    if anyBytes == nil {
        return nil
    }

    return &anyBytes // send pointer of []Byte
}

Every array should be tested with nil before access any index or itself.









anyBytes = getBytes(...) // []Byte

// anyBytes == nil can be nil as well
// len(anyBytes) == 0 when anyBytes == []
if anyBytes == nil || len(anyBytes) == 0 {
    return ... exitResult
}

// do stufss with anyBytes

Any pointer acessing should be checked with not nil.















anyPtr = getPointerBytes(...) // *[]Byte

if anyPtr == nil { // pointer nil
    // should not access any ptr *anyPtr before checking not nil
    return ... exitResult
}

// if it is array based ptr then
// *anyPtr == nil can be nil as well
// len(*anyPtr) == 0 when *anyPtr == []
if *anyPtr == nil || len(*anyPtr) == 0 {
    return ... exitResult
}

// do stufss with anyPtr








func CloseProcessByName(processName string) ProcessClosedStatus {
        // serious issue, code red
	output, err := exec.Command(
		constants.Wmic,
		constants.Process,
		constants.Get,
		constants.ProcessIdAndCaption,
	).Output() // ).Output() will fail at anytime.










func CloseProcessByName(processName string) ProcessClosedStatus {
	cmd, err := exec.Command(
		constants.Wmic,
		constants.Process,
		constants.Get,
		constants.ProcessIdAndCaption,
	)
    
    // wrong, must check err before cmd.
    if cmd != nil ...





















func CloseProcessByName(processName string) ProcessClosedStatus {
    cmd, err := exec.Command(
        constants.Wmic,
        constants.Process,
        constants.Get,
        constants.ProcessIdAndCaption,
    )
    
    // always first check err not nil or nil
    // !Important: If an error is returned from a function, 
    // one must handle it gracefully 
    // reference https://hackmd.io/DErHiap-RWSXwIqFkxWwCg
    if err != nil || cmd == nil {
        // return by capturing error in struct.
        return ...exit condition result.
    }
    
    // work with cmd here
    // ...
    output := cmd.Output()
    ...

, Or

Note: Handle error with panic, not appropriate here in terms of naming.
























func CloseProcessByName(processName string) ProcessClosedStatus {
    cmd, err := exec.Command(
        constants.Wmic,
        constants.Process,
        constants.Get,
        constants.ProcessIdAndCaption,
    )
    
    // always first check err not nil or nil
    // !Important: If an error is returned from a function, 
    // one must handle it gracefully 
    // reference https://hackmd.io/DErHiap-RWSXwIqFkxWwCg
    if err != nil {
        handleError(err) // or return with single struct
    }
    
    if cmd == nil {
        return ...exit condition result.
    }
    
    // work with cmd here
    // ...
    output := cmd.Output()
    ...

Code Mutation (Dangaours Issue, code RED) Avoid and Example

Code mutation, refers to the variable modification, which violates the code review rules (as no variable or things should be changed and should be assigned only once). At the wrost case senario it can be only applied for caching a variable or lazy instanciation / evaluation where an execution requires a for-loop or many checking which will be same after the first time checking then there is no point of running the same thing more than once but cache it using a variable. In that case we can mutate a variable. Keep in mind mutating a variable requires mutation lock, or else it will fail in asyc / gorountine environment. Plus, also keep in mind that adding a lock is also expensive. One can solve it with two ways (a. Lock version of the method b. non lock version of the method). So for example, if a method does GetLines (example can be obtained from strhelper) which is the non lock versin, another version could be GetLinesLock() ->






func (r *receiver) GetLinesLock() []string {
  r.Lock() // which is mutex lock from inside the struct
  defer r.Unlock()
  
  return r.GetLines()
}

Returning variable, updated multiple times, can be fixed.

Current version (with mutation issue)










// code with mutating issue
returningVal := New(...params)

returningVal.Set...(...) // or
returningVal.VarName = ... 

return returningVal

Fixed code mutation (eg. # 1)








varName := whatEverValue

// code constructor took the VarName 
returningVal := New(varName, ...params)

return returningVal

Fixed code mutation (eg. # 2)






varName := whatEverValue

return ReturningValStruct{
     varName: varName // if in the same package
     ...
}

Cheat Sheets

Strings in GO

In Go Strings passes by Reference, but the struct gets copied which has length and bytes to pointer

Note: In Go Strings passes by Reference, but the struct gets copied which has length and bytes to pointer.







// String struct which gets copied but not the data.
type StringHeader struct {
    Data uintptr
    Len  int
}

Slice

Slice Struct





type SliceHeader struct {
    Data uintptr
    Len  int
    Cap  int
}

Code Red

Code has serious issues, tends to be buggy in future.
Requires lots of investigation in future.
Fluctuating result / results.
May have locking related issue.
Looks alright but hard to detect bug.

Dangerous

In some cases complements most points in Code Red.
Code could throw or panic at uncertain times.
Code may cause UX or UI issues.
May not trace log properly and swallow error or exit. Hard to trace in future.
May be not optimized in terms of BigO, could be easily optimized.
Lazy evalution not done properly.
Code may have unknown loop holes.
Mutex not used properly. Probably mutex created in the function, where mutex can be parallel or global.