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

[LEDGER] Improve readability of HandleChaincodeDeploy #1080

Merged

Conversation

cendhu
Copy link
Contributor

@cendhu cendhu commented Apr 13, 2020

Type of change

  • Improvement (improvement to code)

Description

The following are good examples of avoid disinformation, i.e., context should be explicit in the code itself mentioned in the coding guidelines.

  1. if directoryPathArray[3] == "indexes" {
  2. if directoryPathArray[3] == "collections" && directoryPathArray[5] == "indexes" {
  3. collectionName := directoryPathArray[4]

What is 3, 4, and 5? Hence, we replace the above lines with the following

 indexInfo := getIndexInfo(directoryPath)
 switch {
	case indexInfo.hasIndexForChaincode:
	case indexInfo.hasIndexForCollection:
 }

and followed the most important concept first and the low-level details in the bottom. as per the formatting rules specified in the above coding guidelines. The following may not be a perfect one. However, we try to avoid large code refactoring and restrict ourselves to the ledger package.

        chaincodeIndexDirDepth    = 3
	collectionDirDepth        = 3
	collectionNameDepth       = 4
	collectionIndexDirDepth   = 5

Additional details

These changes were initially part of #966 but we decided to push a separate PR (this one) for the readability related changes.

Related issues

FAB-17739

@cendhu cendhu requested a review from a team as a code owner April 13, 2020 18:24
@cendhu cendhu force-pushed the minor-readability-couchdb-index branch from d4ef0f9 to 231f5e6 Compare April 15, 2020 02:24
@cendhu cendhu requested a review from manish-sethi April 15, 2020 02:28
Copy link
Contributor

@manish-sethi manish-sethi left a comment

Choose a reason for hiding this comment

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

@cendhu cendhu force-pushed the minor-readability-couchdb-index branch from 231f5e6 to 21fa654 Compare April 17, 2020 11:48
Signed-off-by: senthil <cendhu@gmail.com>
@cendhu cendhu force-pushed the minor-readability-couchdb-index branch from 21fa654 to bc514a3 Compare April 17, 2020 12:01
@cendhu cendhu requested a review from manish-sethi April 17, 2020 12:01
Copy link
Contributor

@manish-sethi manish-sethi left a comment

Choose a reason for hiding this comment

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

Merging this one. However, I would like to see two improvements. That can be done in a separate PR

  1. Move the constants related to new function, just above the function.
  2. Though, not introduced in the PR but just noticed that array element access without checking lengths. A wrong input could cause panic. I recall that you mentioned that you had added that check but not included finally. But, it would be good to check for safety against a bad input.

@manish-sethi manish-sethi merged commit 89eb4a3 into hyperledger:master Apr 18, 2020
@cendhu cendhu deleted the minor-readability-couchdb-index branch April 18, 2020 05:48
scovetta added a commit to scovetta/fabric that referenced this pull request Jul 10, 2020
This change adds index checks before `dirDepths[collectionIndexDirDepth]` and `dirDepths[collectionNameDepth]` to avoid indexing beyond the end of the array. We *could* just check for the `collectionIndexDirDepth` and avoid the other two, but that would be more brittle in case the constants defined a few lines earlier were reordered. We could also just calculate `len(dirsDepth)` once as a small perf improvement.

This is related to the [suggestion](hyperledger#1080 (review)) from @manish-sethi in hyperledger#1080.
scovetta added a commit to scovetta/fabric that referenced this pull request Jul 10, 2020
This change adds index checks before `dirDepths[collectionIndexDirDepth]` and `dirDepths[collectionNameDepth]` to avoid indexing beyond the end of the array. We *could* just check for the `collectionIndexDirDepth` and avoid the other two, but that would be more brittle in case the constants defined a few lines earlier were reordered. We could also just calculate `len(dirsDepth)` once as a small perf improvement.

This is related to the [suggestion](hyperledger#1080 (review)) from @manish-sethi in hyperledger#1080.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants