-
Notifications
You must be signed in to change notification settings - Fork 88
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
Bulk and RPC API support in translib #16
Bulk and RPC API support in translib #16
Conversation
Merging master to fork
goto AppendWatchTxExit | ||
} | ||
|
||
e = d.performWatch(w, tss) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use "else" here, so you can avoid goto statement ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renuka, We have used goto throughout the code. Even in the checked in code we have goto. May I know the reason to avoid goto. I think we need to leave this to programmers preference.
glog.Warning("StartTx: Empty WatchKeys. Skipping WATCH") | ||
goto StartTxSkipWatch | ||
glog.Warning("performWatch: Empty WatchKeys. Skipping WATCH") | ||
goto SkipWatch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a style requirement to avoid goto ?
Please avoid here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renuka, We have used goto throughout the code. Even in the checked in code we have goto. May I know the reason to avoid goto. I think we need to leave this to programmers preference.
app, appInfo, err := getAppModule(path, req.DeleteRequest[i].ClientVersion) | ||
|
||
if err != nil { | ||
errSrc = ProtoErr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid goto.
You may set errSrc at the start, as it is of significance only when err != nil.
Instead of checking err != nill, check err = nil, for next command execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renuka, We have used goto throughout the code. Even in the checked in code we have goto. May I know the reason to avoid goto. I think we need to leave this to programmers preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"goto" is generally not warranted in a well structured code. So in some places, they strictly block "goto", so you write better code :-).
Even here, instead of checking 'if err !=nil', if you switch to 'err == nil', you avoid goto and the code is better readable. Plus, if I am debugging a code, with goto statements, it is pretty tough, where as if not, I am certain to hit my break point and by looking at local variables, I can gauge the point of failure.
It is not a simple task, to go back and change all over. At the least, let us avoid, from here on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us keep it as a requirement, from here on to avoid "goto" for new code.
Bulk and RPC API support in translib