• 10 Posts
  • 89 Comments
Joined 1 year ago
Cake day: January 21st, 2024
  • xtapatoGolang@programming.dev•Are code reviews allowed? I wrote a pretty basic RBAC module while working on a separate learning project and was hoping to get the opinion of more experienced Gophers
    2·
    22 days ago

    I personally try to avoid deeply nested if/else. Or else in general, because I think it makes code more readable. Especially when one of the branches is just an exit condition.

    if exitCondition {
    return false
}
// long ass code execution

    is way more readable than

    if !exitCondition {
    // long ass code execution
} else {
   return false
}

    In a loop, you can just return the value instead of passing it to a “retVal” variable.

    With those in mind, you could refactor HasPermissions to

    func (r *RBAC) HasPermission(assignedRoles []string, requiredPermission string, visited map[string]bool) bool {
	for _, assigned := range assignedRoles {
		if visited[assigned] {
			continue
		}
		role, ok := r.Roles[assigned]
		if !ok {
			//role does not exist, so skip it
			continue
		}
		for _, permission := range role.Permissions {
			if permission.String() == requiredPermission {
				//Permission has been found! Set permitted to true and bust out of the loop
				return true
			}
		}
		//check inherited roles
		if permitted := r.HasPermission(role.Inherits, requiredPermission, visited); permitted {
			return true
		}
	}
	return false
}

    The same could be applied to LoadJSONFile and I think that really would approve the readability and maintainability of your code.

    edit: This refactor is not tested