Code review: zcached key-value cache database

[crude mason]

https://github.com/sectasy0/zcached

[crude mason]

bump

[patent sparrow]

full project code reviews are a pretty big time investment. is there a particular part of the code you'd like specific feedback on?

[crude mason]

hmm, mostly overall project architecture review, network layer (https://github.com/sectasy0/zcached/tree/master/src/server/network) and processing layer (https://github.com/sectasy0/zcached/tree/master/src/server/processing)

[patent sparrow]

so like, that's kind of a lot of code to review ^^'

you're basically asking someone to read and understand your entire project and give you feedback on it

it'd help if you picked out some particular snippets, or drew an architecture diagram. that way it requires much less time investment from the reviewer, because all the relevant context has already been provided by someone who knows the codebase (you)

[crude mason]

Okay, I'll make detailed diagram how it works soon

For now i'll chose just two files for review:
https://github.com/sectasy0/zcached/blob/master/src/server/network/listener.zig
https://github.com/sectasy0/zcached/blob/master/src/server/network/warden.zig

[patent sparrow]

here's a couple random comments from a cursory glance:

• server is the entire project; just move all those files into the root src dir, there's no point in adding extra nesting when it's not needed
• the types_map thing in protocol/seralizer.zig is weird and slow. just use switch (reader.readByte())

also the protocol seems simple enough that you could probably merge it all into one file

[crude mason]

for now there's only server but in future there will be also a client, they'll be sharing things like network layer

[patent sparrow]

this is weird. just use hashmap directly https://github.com/sectasy0/zcached/blob/master/src/protocol/set.zig

[patent sparrow]

that's fine; if you feel like you need to, you can move server-specific stuff into a subdirectory when you do that refactor, not before

i would start with src/ containing the server, and then when you add a client lib you can split it into separate client/, server/, and protocol/ modules, rather than having one src directory

or some other structure, depending on what makes the most sense

focus on the code you have now, and refactor it when you need to. moving code around is easy, it's nothing to be afraid of

locking in your project structure in advance is a very bad idea. you can't know in advance what the structure needs to be, because you've not written the code yet

[crude mason]

you mean instead of wraping this use HashMap diectly in types.zig?

[patent sparrow]

yes, that wrapper seems like pointless abstraction

[patent sparrow]

some high-level structure/style critique:

• listener.zig is just a function. it shouldn't be a struct (and if it were, the filename should be Listener.zig because it's a file-level struct)
• Context shouldn't be in employer.zig if it's used by other things
• ...but also Context shouldn't exist. just pass the allocator, logger, and storage layer separately. listen only needs a logger, for example, so there's no point in passing in the other stuff
• _processIncoming is a weird name. there's no point in prefixing non-public functions with an underscore, because they're already non-public.
• you seem to be mixing snake_case and camelCase function names. pick one and stick to it (zig's official style guide uses camelCase for functions)

you also have a ton of very small files and structs. merge them; splitting things up too much only makes code harder to read

[crude mason]

I created context struct to store params here, main reason was to not have many arguments in fuctions, so just packed in struct instead and this is a fairly common pattern in programming

Thank you

[patent sparrow]

you can solve this problem by making bigger structs. if you have eg. a Server struct that contains all server state, you don't need to pass around a Context either :)

alternatively, create a one-off struct that's used for a specific function's arguments. you can do it inline: ```rs
fn foo(args: struct {
allocator: std.mem.Allocator,
thing: Thing,
another: Thing2,
// ...
}) !void { ... }

the Context thing is a weird middle ground that means you end up passing more stuff than is needed to things and also make the code harder to read

[crude mason]

For me this one is harder to read and figure out

[patent sparrow]

sure, it's often not a good idea. passing a few extra args is totally fine

for stuff that takes a lot of args though, a struct can be helpful

you don't have to do it inline, you can declare it separately if you prefer

but trying to share a "common context" struct is silly. it's simultaneously too small to be useful as a namespace, and too big to actually be fully needed for every function it's passed to

also, dependency injection (ie. passing resources like Allocator etc into functions) is useful because it clearly shows what resources a function uses, and lets you easily override them. bundling all your dependencies into a context struct throws that away

[crude mason]

Most of the things are used, config and logger is used almost everywhere and memory is only passed during command handle creation

[patent sparrow]

bestie it's literally three things, just pass them as args

that way you actually immediately know what's being used, rather than having to read the code to figure out whether it touches the database or not, etc

[crude mason]

Okay, I'll think about it, thank you so much

If you have time to look through the rest of the code I would be grateful, I will post a diagram of the entire system soon.

[patent sparrow]

if you do some of the refactors i mentioned (specifically merging files and making bigger structs) i'm happy to have another look :)

it's quite difficult to get proper context and understand how things fit together currently because of how spread out everything is

(which is why most of my critique so far has been on structure, rather than the actual code itself)

[crude mason]

Which files you suggest to merge together?

Those small ones, right? Like connection, warden?

[patent sparrow]

the whole protocol dir can probably be one file

everything networking-related should probably be in one file

and the rest of the server code should probably either be in the same file as the server networking stuff, or in one or two separate files

i tend to start with just a single file and then split things into their own files when it feels like they've become a standalone module

in particular, mutual imports are pretty clear sign that two files should be merged into one

basically anything that's tightly coupled to something else should probably be in the same file as that thing

[crude mason]

I made diagram that is supposed to show the flow of the program, key element dependecies and their responsibilities, could you take a look at it in your spare time?