Clean Code

Hamdi Ahmadi Muzakkiy

Outline

  • Why ?
  • Meaningful Name
  • Function
  • Comments
  • Error Handling
  • Unit Test
  • Case
  • Golang Coding Tools

Why ?

  • Collaborate With Other
  • Easy to Improve
  • Make Development Faster
  • Minimize Bug On Future

Meaningful Name

Use Intention-Revealing Names

avoid :

  • var d int // days

use :

  • var days int // days

Make Meaningful Distinction

avoid :

  • func CopyString(s1, s2 string){ ... }

use :

  • func CopyString(source, destination string){ ... }

Class Name

class, object or struct better use noun

avoid :

  • type Data struct {}

use :

  • type Customer struct { }

Method Name

method name better use verb

avoid :

  • func (customer Customer) Name(){ ... }

use :

  • func (customer Customer) GetName(){ ... }

Don't Be Cute

avoid :

  • func (cart Cart) HolyHandGrenade(){ ... }

use :

  • func (cart Cart) DeleteItem(){ ... } 

Pick 1 Word Perconcept

method name better use verb

avoid :

  • func (customer Customer) GetName(){ ... }
  • func (customer Customer) FetchAge(){ ... }
  • func (customer Customer) RetrieveHobby(){ ... }
func CompareAppVersion(s1, s2 string) int {
	tmps1 := strings.Split(s1, ".")
	tmps2 := strings.Split(s2, ".")

	s1L := len(s1)
	s2L := len(s2)

	ss1 := 0
	ss2 := 0

	for {
		if ss1 == s1L && ss2 == s2L {
			return 0
		}

		if ss1 == s1L {
			return 1
		}

		if ss2 == s2L {
			return -1
		}

		a1, _ := strconv.Atoi(tmps1[ss1])
		a2, _ := strconv.Atoi(tmps2[ss2])

		if a1 > a2 {
			return -1
		} else if a1 < s2 {
			return 1
		}
		ss1++
		ss2++
	}
	return 0
}
func CompareAppVersion(appVersion, appVersionCompared string) int {
	splitedAppVersion := strings.Split(appVersion, ".")
	splitedAppVersionCompared := strings.Split(appVersionCompared, ".")

	splitedAppVersionLength := len(splitedAppVersion)
	splitedAppVersionComparedLength := len(splitedAppVersionCompared)

	start := 0
	startCompared := 0
	for {
		if start == splitedAppVersionLength && startCompared == splitedAppVersionComparedLength {
			return APPVERSION_EQUAL // 0
		}

		if start == splitedAppVersionLength {
			return COMPARED_APPVERSION_HIGHER // 1
		}

		if startCompared == splitedAppVersionComparedLength {
			return APPVERSION_HIGHER // -1
		}

		numAppVersion, _ := strconv.Atoi(splitedAppVersion[start])
		numAppVersionCompared, _ := strconv.Atoi(splitedAppVersionCompared[startCompared])

		if numAppVersion > numAppVersionCompared {
			return -1
		} else if numAppVersion < numAppVersionCompared {
			return 1
		}
		start++
		startCompared++
	}
	return APPVERSION_EQUAL
}

Const

func (payment Payment) EvaluatePayment() int {
    
    if ... {
        return 1
    }
    
    ...
    
    if ... {
        return 2    
    }
    return 3
    
}
func (payment Payment) EvaluatePayment() int {
    
    if ... {
        return PAYMENT_ABUSE
    }
    
    ...
    
    if ... {
        return PAYMENT_FAILED    
    }
    return PAYMENT_SUCCESS
    
}

Function

Small!

  • should be small
  • should be smaller than that
  • should be no bigger than a screen-full
func (adDataGroup *AdDataGroup) GetAdsRawData(from, to string) error {
	var result AdDataGroup

	param := url.Values{}

	param.Add("api_token", AMod.Config.API["blacklotus"].Token)
	param.Add("from", from)
	param.Add("to", to)

	agent := utils.NewHTTPRequest()
	agent.Url = AMod.Config.API["blacklotus"].URL
	agent.Path = AMod.Config.API["blacklotus"].Path
	agent.Method = "GET"
	agent.Param = param
	agent.Timeout = time.Duration(60 * 60 * time.Second)

	body, err := agent.DoReq()

	if err != nil {
		utils.Error(err)
		return err
	}

	repostCSVString := string(*body)
	reportCSVGroup := csv.NewReader(strings.NewReader(repostCSVString))

	flags := true
	for {
		record, err := reportCSVGroup.Read()

		// passed first rec
		if flags {
			flags = !flags
			continue
		}

		if err == io.EOF {
			break
		}
		if err != nil {

			utils.Error(err)
			return err
		}

		var adData AdData
		adData.actionSetReport(record)

		result = append(result, adData)
	}
	*adDataGroup = result
	return nil
}
func (adDataGroup *AdDataGroup) GetAdsRawData(from, to string) *io.Reader {
	var result AdDataGroup

	param := url.Values{}

	param.Add("api_token", AMod.Config.API["blacklotus"].Token)
	param.Add("from", from)
	param.Add("to", to)

	agent := utils.NewHTTPRequest()
	agent.Url = AMod.Config.API["blacklotus"].URL
	agent.Path = AMod.Config.API["blacklotus"].Path
	agent.Method = "GET"
	agent.Param = param
	agent.Timeout = time.Duration(60 * 60 * time.Second)

	body, err := agent.DoReq()

	if err != nil {
		utils.Error(err)
		return err
	}

	repostCSVString := string(*body)
	reportCSVGroup := csv.NewReader(strings.NewReader(repostCSVString))
        return reportCSVGroup
}

func (adDataGroup *AdDataGroup) MappingAdsCSVData (csvData *io.Reader) {
        flags := true
	for {
		record, err := csvData.Read()

		// passed first rec
		if flags {
			flags = !flags
			continue
		}

		if err == io.EOF {
			break
		}
		if err != nil {

			utils.Error(err)
			return err
		}

		var adData AdData
		adData.actionSetReport(record)

		result = append(result, adData)
	}
	*adDataGroup = result
	return nil

} 

Do One Thing

  • Should do one thing

Arguments

  • passed data from argument rather instance 
// Avoid
func (adData AdData) isOperatorContainOnOperatorList() bool {
        operatorGroup := GetOperatorGroup()
	searchedOperator := strings.ToLower(adData.Operator)
	for _, operator := range operatorGroup {
		if strings.Contains(searchedOperator, operator) {
			return true
		}
	}
	return false
}

// Use
func (adData AdData) isOperatorContainOnOperatorList(operatorGroup []string) bool {
	searchedOperator := strings.ToLower(adData.Operator)
	for _, operator := range operatorGroup {
		if strings.Contains(searchedOperator, operator) {
			return true
		}
	}
	return false
}

Arguments

  • avoid boolean arguments
// Avoid
func rander(flag bool) {
    if flag {
         ...       
        return
    }
    ...
}

// Use
func randerHomePage() {
    ...
}

func randerLoginPage() {
    ...
}

Arguments

  • Ideal:
    • Zero ( niladic ) // Use
    • One ( Monodic )
    • Two ( dyadic )
    • three ( triadic ) // Avoid
    • more then three ( polyadic ) // Don't Use
// Avoid

func MakeCircle(x, y ,radius float) Circle {
    ...
}


// strategy : make x and y an object/struct
// Use 

func MakeCircle(point Center, radius float) Circle {
    ...
}

Have No Side Effects

  • Make sure a function have no side effect

func CheckPassword(username, password string) bool {
     var user User   
     user = getUserPassword(username)
     
    if user.Password == password {
         user.SetSession()
         return true   
    }
    return false
}
func (cartGroup *[]Cart) CalculateTotalPaymentWithDiscount() int {
    for _, cart := cartGroup {
        if cart.Amount < 100000 {
            cartGroup.Total -= (cart.Amount/0.1)
        } else {
            cartGroup.Total -= (cart.Amount/0.5)
        }
    }
    return cartGroup.Total
}

Comments

Good Comments

truly good comment is the comment you found a way not to write

Legal Comments

// Copyright (C) 2003,2004,2005 by Object Mentor, Inc. All rights reserved. 
// Released under the terms of the GNU General Public License version 2 or later.

Informative Comments

// format matched kk:mm:ss EEE, MMM dd, yyyy

Warning of Consequences

// Don't run unless you 
// have some time to kill.

Bad Comments

  • Redundant Comments (take more time to read comment then read the code )
    
  • Journal Comments
  • Noise Comment ( Comment for obvious function )

Error Handling

f, err := os.Open("filename.ext")
if err != nil {
    ...
}
if err := os.Unsetenv("test"); err != nil {
    ...
}

Unit Test

http://slides.com/hamdiahmadi/tdd#/

Casê

type Event struct {
    Name string
    Schedule time.Time
}

func NotifyEventA() { ... }
func NotifyEventB() { ... }
func NotifyEventC() { ... }

func (event Event) SetCron(option string) {
    cronClient := cron.New()
    
    if option == "event a" {
        cronClient.AddFunc(FormatingSchedule(event.Schedule), func(){
            NotifyEventA()
        })
    } else if option == "event b" {
        cronClient.AddFunc(FormatingSchedule(event.Schedule), func(){
            NotifyEventB()
        })
    } else if option == "event c" {
        cronClient.AddFunc(FormatingSchedule(event.Schedule), func(){
            NotifyEventC()
        })
    }

    cronClient.Start()
}

func main() {
    event := Event{
        Name : "Event Something"
        Schedule : time.Now().Add(24 * time.Hour * 10)    
    }
    event.SetCron("event a") 
}
type Event struct {
    Name string
    Schedule time.Time
}

type NotifyJob func()

func NotifyEventA() { ... }
func NotifyEventB() { ... }
func NotifyEventC() { ... }

func (event Event) SetCron(job NotifyJob) {
    cronClient := cron.New()
    
    cronClient.AddFunc(FormatingSchedule(event.Schedule), func(){
        job()
    })
    cronClient.Start()
}

func main() {
    event := Event{
        Name : "Event Something"
        Schedule : time.Now().Add(24 * time.Hour * 10)    
    }
    event.SetCron(NotifyEventA) 
}

Golang Coding Tools

Go vet

  • Useless assignments
  • Common mistakes when using sync/atomic
  • Invalid +build tags
  • Using composite literals without keyed fields
  • Passing locks by value
  • Comparing functions with nil
  • Using wrong printf format specifiers
  • Closing over loop variables the wrong way
  • Struct tags that do not follow the canonical format
  • Unreachable code
  • Misuse of unsafe.Pointer
  • Mistakes involving boolean operators

go vet github.com/username/repository/src/package

Err Check

errcheck github.com/username/repository/src/package

Go Lint

golint src/package

Gofmt

gofmt -w src/package

Clean Code

By hamdi ahmadi