PeterA - Pet park assignment #22
Conversation
contracts/PetPark.sol
Outdated
| pragma solidity ^0.8.0; | ||
|
|
||
| contract PetPark | ||
| { | ||
| // -- SCAFFOLDING ------------------------- |
There was a problem hiding this comment.
If adding comments, the better way would be to use natspec
| // -- SCAFFOLDING ------------------------- | ||
| enum AnimalType | ||
| { | ||
| NONE |
There was a problem hiding this comment.
It will be good to use a solidity plugin to format here - https://marketplace.visualstudio.com/items?itemName=NomicFoundation.hardhat-solidity
There was a problem hiding this comment.
formatting is a must? i kind of like this c-style formatting...
thanks for the extension!
There was a problem hiding this comment.
Generally, all projects in production use formatted code. Although the code still works but it is better to have it formatted
| { | ||
| uint8 age; | ||
| AnimalType animal; | ||
| bool isFemale; |
There was a problem hiding this comment.
This can be gender and you can use enum to store it similar to AnimalType
There was a problem hiding this comment.
yes; a bool is faster, no?
There was a problem hiding this comment.
We don't optimise for time in solidity (mentioned the reason in my other comment). The optimisation should be for gas and storage
contracts/PetPark.sol
Outdated
| // -- STACK ------------------------------ | ||
| address private owner; | ||
|
|
||
| uint8[] private counts = [0,0,0,0,0,0]; // stores number of "instances" of each animal |
There was a problem hiding this comment.
You can use a mapping here
There was a problem hiding this comment.
yes, but an immutable array is faster to search, also more appropriate for the task, since we're only changing uint values and not actually allocating new animals?
There was a problem hiding this comment.
Mappings are always faster for lookup. Although being fast isn't a concern in solidity because your transaction would anyhow confirm after the block is mined (same time as other transactions). There are other advantages to using mappings too like
- Code readability
- You can just declare a mapping named animalCounts and make it public, in that case you won't have to define a new function
- When you switch to enums for animal type, you will have to use that enum type as key which is possible only in mapping
There was a problem hiding this comment.
I miss-spoke; I meant faster to access, not to search :)
in this particular case, where the animal park is stored in counts per animal type, we will always have a value for each animal type (sounds like a good candidate for an array of fixed size); if using a map, we will still end up having all animal types in it.
the key here, which made the array choice more preferable, is that when calling map[index] it does a binary search (log n time), but when doing array[index] it returns the address of array, then adds index to it (constant time)
WRT to each your point:
- "Code readability" -- right, uint conversion every time you access the array is not so great :)
- "You can just declare a mapping named animalCounts and make it public, in that case you won't have to define a new function" -- AHA, can't do that with an array, because you would need to convert AnimalType to an uint ;)
- "When you switch to enums for animal type, you will have to use that enum type as key which is possible only in mapping" -- Already using enum for animal type
I'll revert the commit which made it as mapping
you mentioned in Solidity we don't optimize for time, only for gas. I find this very, very curious, and counter-intuitive to most programming. Can you elaborate or perhaps share an article discussing this in more detail?
There was a problem hiding this comment.
Map in solidity is a hash table. It does lookup in O(1) and doesn't do binary search. Both map[index] and array[index] here happen in constant time
Ref - https://docs.soliditylang.org/en/v0.4.21/types.html#mappings
On 3rd: My bad, I meant in mapping you could have it declared as (AnimalType => uint) which is a bit more readable
There was a problem hiding this comment.
About optimizing code for time. Let's take an example - Let's say you and I both submit a transaction at the same time and your code is faster. Since both these transactions are in same block, we will both get the output at the same time i.e., when the block is mined. A block is only minted when every transaction in that particular block is executed
There was a problem hiding this comment.
Thanks for the link, and clarification abt binary search in map;
Abt your example:
Hmm...can't seem to wrap my head around it; I would expect code that does fewer ops to cost less gas.
Will counter with another example: a simple method which transfers an amount and another method which takes 3s to (needlessly) calculate the square root of PI before transferring the amount.
Wouldn't the second method cost more gas?
There was a problem hiding this comment.
As I mentioned earlier, in solidity we do optimise for gas. Speed and gas are not exactly co-related. You could do more ops and still cost less gas (reading from memory is significantly cheaper). We will go into more details on this in an upcoming lecture
| @@ -187,5 +187,19 @@ describe("PetPark", function () { | |||
|
|
|||
| expect(reducedPetCount).to.equal(currentPetCount - 1); | |||
| }); | |||
|
|
|||
| it("should emit returned event when valid details are provided", async function () { | |||
There was a problem hiding this comment.
Thanks for adding this test!
No description provided.