Conversation
src/MPT/MPTCache.cs
Outdated
| using System.IO; | ||
|
|
||
| namespace Neo.Plugins.MPT | ||
| namespace Neo.Cryptography.MPT |
There was a problem hiding this comment.
A Neo.Cryptography in the plugin? It is misleading. We have Neo.Cryptography in the neo core already.
There was a problem hiding this comment.
I disagree. .NET guidelines specifies "the goal when naming namespaces is creating sufficient clarity for the programmer using the framework to immediately know what the content of the namespace is likely to be". So, I think using Neo.Cryptography.MPT here is fine.
Since the classes in the namespace are all name prefixed with MPT, having MPT in the namespace is as well seems redundant. Neo.Cryptography.MPT.MPTTrie? Do we need to repeat MPT here? Why not just Neo.Cryptography.MPT.MPTTrie?
Note, Neo already has places where namespaces are used across multiple binaries. In particular, all the plugins in this repo are in the Neo.Plugins namespace. We even have multiple types named Neo.Plugins.Settings (ApplicationLogs and RpcServer)
There was a problem hiding this comment.
The namespace is used to organize code, I don't think it is a good idea to have the same namespace across multiple projects.
There was a problem hiding this comment.
I think it could be a distinct namespace
There was a problem hiding this comment.
We have Neo.IO namespace in LevelDBPlugin as well as in core.
There was a problem hiding this comment.
Since the classes in the namespace are all name prefixed with
MPT, havingMPTin the namespace is as well seems redundant.Neo.Cryptography.MPT.MPTTrie? Do we need to repeatMPThere? Why not justNeo.Cryptography.MPT.MPTTrie?
Does make sense. Neo.Cryptography.MPTTrie? Should we change all class names? They all have MPT prefix.
|
@devhawk Names updated. |
shargon
left a comment
There was a problem hiding this comment.
Please, wait @erikzhang 's review
|
Can you fix the ut? |
Sure. |
|
@erikzhang @shargon Merge? |
Close #648