Best Practices and Anti-Patterns Updates

[joshuahannan]

Closes onflow/flow#708

IMPORTANT: If there are any best practices or anti-patterns that you think are missing from here and should be included, please let me know and I will add them. I added all the ones I could think of, but I'm sure there are more.

Description

• Reorganizes some of the best practices and anti-patterns
• Adds new ones
• Removes some that I disagree with. 😉
• Fixes some links

---

• Targeted PR against master branch
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work
• Updated relevant documentation
• Re-reviewed Files changed in the Github PR explorer
• Added appropriate labels

[codecov-commenter]

Codecov Report

Merging #1471 (edd743b) into master (5238cce) will not change coverage.
The diff coverage is n/a.

❗ Current head edd743b differs from pull request most recent head 685616c. Consider uploading reports for the commit 685616c to get more accurate results

@@           Coverage Diff           @@
##           master    #1471   +/-   ##
=======================================
  Coverage   74.68%   74.68%           
=======================================
  Files         289      289           
  Lines       55607    55607           
=======================================
  Hits        41528    41528           
  Misses      12584    12584           
  Partials     1495     1495           

Flag	Coverage Δ
unittests	74.68% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5238cce...685616c. Read the comment docs.

[github-actions]

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 5238cce
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Results

	old.txt	new.txt
	time/op		delta
RuntimeResourceDictionaryValues-2	17.0ms ± 4%	16.9ms ± 0%	~	(p=0.755 n=7+5)
RuntimeFungibleTokenTransfer-2	1.49ms ±22%	1.54ms ±25%	~	(p=1.000 n=7+7)
ParseArray-2	18.1ms ± 8%	17.6ms ± 4%	~	(p=0.209 n=7+7)
ParseInfix-2	10.1µs ± 2%	10.1µs ± 3%	~	(p=0.971 n=7+7)
ParseDeploy/byte_array-2	30.1ms ± 7%	28.8ms ± 7%	~	(p=0.165 n=7+7)
ParseDeploy/decode_hex-2	1.44ms ± 2%	1.43ms ± 2%	~	(p=0.456 n=7+7)
ParseFungibleToken-2	228µs ± 6%	222µs ± 3%	~	(p=0.128 n=7+7)
QualifiedIdentifierCreation/Three_levels-2	174ns ± 2%	171ns ± 2%	~	(p=0.179 n=6+7)
CheckContractInterfaceFungibleTokenConformance-2	162µs ± 2%	160µs ± 0%	~	(p=0.052 n=6+5)
NewInterpreter/new_sub-interpreter-2	2.75µs ± 5%	2.75µs ± 2%	~	(p=0.832 n=7+7)
NewInterpreter/new_interpreter-2	1.40µs ± 1%	1.38µs ± 2%	−1.45%	(p=0.024 n=7+6)
InterpretRecursionFib-2	3.34ms ± 3%	3.28ms ± 2%	−1.65%	(p=0.017 n=7+7)
QualifiedIdentifierCreation/One_level-2	2.86ns ± 5%	2.79ns ± 0%	−2.22%	(p=0.003 n=7+5)
ContractInterfaceFungibleToken-2	49.7µs ± 3%	48.3µs ± 1%	−2.76%	(p=0.001 n=7+6)
	alloc/op		delta
RuntimeResourceDictionaryValues-2	4.05MB ± 0%	4.05MB ± 0%	~	(p=0.435 n=7+7)
RuntimeFungibleTokenTransfer-2	273kB ± 0%	273kB ± 0%	~	(p=0.777 n=7+7)
QualifiedIdentifierCreation/One_level-2	0.00B	0.00B	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	64.0B ± 0%	64.0B ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	66.2kB ± 0%	66.2kB ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	26.6kB ± 0%	26.6kB ± 0%	~	(p=0.192 n=7+7)
NewInterpreter/new_interpreter-2	848B ± 0%	848B ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	1.34kB ± 0%	1.34kB ± 0%	~	(all equal)
InterpretRecursionFib-2	1.26MB ± 0%	1.26MB ± 0%	~	(p=0.385 n=7+6)
	allocs/op		delta
RuntimeResourceDictionaryValues-2	102k ± 0%	102k ± 0%	~	(p=0.344 n=7+6)
RuntimeFungibleTokenTransfer-2	4.54k ± 0%	4.54k ± 0%	~	(p=1.000 n=7+7)
QualifiedIdentifierCreation/One_level-2	0.00	0.00	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	2.00 ± 0%	2.00 ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	1.07k ± 0%	1.07k ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	458 ± 0%	458 ± 0%	~	(all equal)
NewInterpreter/new_interpreter-2	13.0 ± 0%	13.0 ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	40.0 ± 0%	40.0 ± 0%	~	(all equal)
InterpretRecursionFib-2	26.2k ± 0%	26.2k ± 0%	~	(all equal)

[turbolent]

Awesome work! 👏 I'll have a read :-)

If there are any best practices or anti-patterns that you think are missing from here and should be included, please let me know and I will add them.

I'd have a performance one: Prefer in-place mutations.

For example, if the task is to update some data structure, e.g. a dictionary in storage or a contract, don't copy the whole data onto the "stack", modify it, then overwrite the existing data with the updated copy.

Instead, use e.g a reference to mutate the data in-place.

Here is a real-world example of this: https://github.com/dapperlabs/nba-smart-contracts/pull/125/files#diff-1493a8d9c5e6feabf2be9290b17f904a9e89e871cfcb0c21fce75dfb524eb406L105-R108

[joshuahannan]

@turbolent Thanks for the feedback!

I'd have a performance one: Prefer in-place mutations.

Yep, that one is in there!
https://github.com/onflow/cadence/blob/best-practices/docs/design-patterns.mdx#avoid-excessive-load-and-save-storage-operations-prefer-in-place-mutations

[satyamakgec]

LGTM, Good work @joshuahannan

[satyamakgec]

pseudo code here as well, If possible

[kimcodeashian]

FYI - I'll be reviewing through the lens as the intended audience, and suggestions will be based on improving clarity & understanding for the reader. 😄

ALSO - love the new organization/presentation of content. Much cleaner than before.

I'll be continuing to review this next week 👍

[joshuahannan]

Hey @turbolent, looks like I need an approval from a code owner to merge this. Can you approve it please?

Thanks!

[turbolent]

Great work!

[joshuahannan]

Thank you so much for the reviews @turbolent and @psiemens ! I've addressed all your comments and pushed my latest changes.

[psiemens]

LGTM! Great work!

[turbolent]

Great work! 👏