Various style discussions

[nictuku]

Since we don't have a mailing list (not saying we need one) I'd like to discuss here a few style changes:

• when naming a method that simply returns Foo, name it Foo() instead of GetFoo(). ~ GetFoo() is currently common in the code, but one example: item.GetPosition()
• interfaces should be named ISomething, as is the case in interface.go ~ there are a few exceptions, but we should probably normalize this.
• should we have a guideline for when a method should return a reference or a value? It feels a bit confusing at the moment. We could be tempted at always returning references, but that's unsafe. Literature like "Code Complete" point out that this is a common source of bugs, since the caller obviously can change the value and this is not always what we want. At the same time, it's argued that the possible performance gains are hard to notice. ~ an option is to always return by value unless not possible. But I feel a bit bad in copying values around, so I honestly don't know what's best. I just miss having some consistency. ~ All that said, if we settle down with some kind of pure-channel design where only a single goroutine has access to an object, we could stick to a "always return by reference" rule.

[huin]

I will reply more when I get home, but I think I broadly agree with every point there.

Additionally, a mailing list or some other tool is tempting - partly for dev communication but also as public image. Not sure if we need such a thing right now but I'm open to suggestions. On 12 May 2011 17:53, "nictuku" < [email protected]> wrote:

Since we don't have a mailing list (not saying we need one) I'd like to discuss here a few style changes:

• when naming a method that simply returns Foo, name it Foo() instead of GetFoo(). ~ GetFoo() is currently common in the code, but one example: item.GetPosition()
• interfaces should be named <ISomething>, as is the case in interface.go ~ there are a few exceptions, but we should probably normalize this.
• should we have a guideline for when a method should return a reference or a value? It feels a bit confusing at the moment. We could be tempted at always returning references, but that's unsafe. Literature like "Code Complete" point out that this is a common source of bugs, since the caller obviously can change the value and this is not always what we want. At the same time, it's argued that the possible performance gains are hard to notice. ~ an option is to always return by value unless not possible. But I feel a bit bad in copying values around, so I honestly don't know what's best. I just miss having some consistency. ~ All that said, if we settle down with some kind of pure-channel design where only a single goroutine has access to an object, we could stick to a "always return by reference" rule.

Reply to this email directly or view it on GitHub: https://github.com/huin/chunkymonkey/issues/42

[nictuku]

Should I update README.md with these suggestions?

[huin]

That would be excellent, thanks :)

Ideally styles should be checked by the style checker, but it's not a high priority at the moment. I just picked long-hanging fruit of checking interface names.

[nictuku]

https://github.com/nictuku/chunkymonkey/tree/style-candidate has my proposed changes. It's synced with your shard_refactor_no_player_split branch, but I'll wait for the refactor to slow down or finish before I fix the offending names, then request a pull.

./src/chunkymonkey/slot.go:27:16: Bad name for method "GetItemTypeId" ./src/chunkymonkey/slot.go:36:16: Bad name for method "GetAttr" ./src/chunkymonkey/item.go:34:19: Bad name for method "GetEntity" ./src/chunkymonkey/item.go:38:19: Bad name for method "GetSlot" ./src/chunkymonkey/item.go:42:19: Bad name for method "GetItemTypeId" ./src/chunkymonkey/item.go:46:19: Bad name for method "GetCount" ./src/chunkymonkey/chunkstore/beta.go:79:27: Bad name for method "GetOffset" ./src/chunkymonkey/chunkstore/beta.go:94:29: Bad name for method "GetDataReader" ./src/chunkymonkey/chunkstore/chunk_reader.go:57:23: Bad name for method "GetRootTag" ./src/chunkymonkey/entity.go:15:23: Bad name for method "GetEntityId" ./src/chunkymonkey/shardserver/side.go:89:27: Bad name for method "GetCachedBlock" ./src/chunkymonkey/shardserver/side.go:127:30: Bad name for method "GetCachedBlock" ./src/chunkymonkey/shardserver/chunk.go:95:21: Bad name for method "GetRand" ./src/chunkymonkey/shardserver/chunk.go:99:21: Bad name for method "GetItemType" ./src/chunkymonkey/shardserver/chunk.go:130:21: Bad name for method "GetBlockExtra" ./src/chunkymonkey/shardserver/chunk.go:165:21: Bad name for method "GetBlock" ./src/chunkymonkey/shardserver/chunk.go:176:21: Bad name for method "GetRecipeSet" ./src/chunkymonkey/inventory/window.go:89:18: Bad name for method "GetWindowId" ./src/chunkymonkey/player/player.go:81:23: Bad name for method "GetEntityId" ./src/chunkymonkey/player/player.go:85:23: Bad name for method "GetEntity" ./src/chunkymonkey/player/player.go:101:23: Bad name for method "GetName" ./src/chunkymonkey/player/player.go:105:23: Bad name for method "GetHeldItemType" ./src/chunkymonkey/types.go:156:15: Bad name for method "GetDxyz" ./src/chunkymonkey/types.go:336:23: Bad name for method "GetDxz" ./src/chunkymonkey/types.go:350:23: Bad name for method "GetOpposite" ./src/chunkymonkey/types.go:531:26: Bad name for method "GetChunkCornerBlockXY" ./src/chunkymonkey/types.go:592:22: Bad name for method "GetBlockId" ./src/chunkymonkey/types.go:596:22: Bad name for method "GetBlockData" ./src/chunkymonkey/mob/mob.go:55:17: Bad name for method "GetEntityId" ./src/chunkymonkey/mob/mob.go:59:17: Bad name for method "GetEntity" ./src/chunkymonkey/nbt/nbt.go:65:17: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:82:20: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:129:14: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:145:15: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:161:13: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:177:14: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:193:15: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:209:16: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:225:19: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:255:16: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:285:14: Bad name for method "GetType" ./src/chunkymonkey/nbt/nbt.go:325:18: Bad name for method "GetType"

[huin]

Awesome. I'm wondering if it might be acceptable to merge the branch back into master soon. There's still some changes that need to happen, but I think that the majority of larger changes are done, and I'm into the stage of reconnecting a bunch of broken wires (most notably not being able to pick up items or interact with workbenches).

[nictuku]

I wouldnt mind if you merge to master, even if it breaks things. On May 23, 2011 10:37 AM, "huin" < [email protected]> wrote:

Awesome. I'm wondering if it might be acceptable to merge the branch back into master soon. There's still some changes that need to happen, but I think that the majority of larger changes are done, and I'm into the stage of reconnecting a bunch of broken wires (most notably not being able to pick up items or interact with workbenches).

Reply to this email directly or view it on GitHub: https://github.com/huin/chunkymonkey/issues/42#comment_1220509

[huin]

Okay, I've merged that lot to master. Feel free to merge in the style checker changes :)

[nictuku]

I spend some time today doing a small refactoring to get rid of GetFoo() methods and cleanup entity/item/mob a bit. I thought it would be piece of cake, but I got sidetracked with other 'easy fixes', I broke lots of stuff and didn't achieve anything by the end of the day :-P.

So, to make my life easier I'll do small incremental fixes instead, while reviewing the existing code. That is, expect "make check" to look ugly for some time. Sorry about that.

[huin]

That's fine, we can live with that for a bit longer :) On 26 May 2011 00:46, "nictuku" < [email protected]> wrote:

I spend some time today doing a small refactoring to get rid of GetFoo() methods and cleanup entity/item/mob a bit. I thought it would be piece of cake, but I got sidetracked with other 'easy fixes', I broke lots of stuff and didn't achieve anything by the end of the day :-P.

So, to make my life easier I'll do small incremental fixes instead, while reviewing the existing code. That is, expect "make check" to look ugly for some time. Sorry about that.

Reply to this email directly or view it on GitHub: https://github.com/huin/chunkymonkey/issues/42#comment_1239212

[huin]

A lot of these names should be fixable now that the refactor is over.