random unsolicited code review
Whilst reading the code I came across a few things that aren't functional/API problems but more like code review, so I thought I'd share it here.
- Any reason not to use
RequestResponseHandler
. That would be a much more efficient and more correct implementation ofRedisCommandHandler
. -
RedisCommandHandler
uses FIFOArray
as a queue andArray
is a very bad FIFO queue as insertion/removal at the front is O(n). If you really can't useRequestResponseHandler
, please use NIO'sCircularBuffer
which is a good queue. - don't use
@_exported
, it breaks SemVer - please don't use
@inline(__always)
unless you measured that@inlinable
doesn't cut it. All the uses of@inline(__always)
I've seen can almost certainly also be@inlinable
and honestly they looked like they should just be normal functions. - Did you have any specific reasons for all the
@inlinable
s? Pretty much all the functions where I found an@inlinable
don't seem to actually benefit from@inlinable
very much at all so I was wondering why we need them. -
EventLoopFuture.mapFromRESP
is not amap
-like function for 2 reasons:- because it may fail, ie. go from the future's success track to the future's failed track and that's not something map functions usually do.
- it doesn't actually take a function to
map
. Maybe should it be namedtryConvert<Type>(to: Type.Type)
?
- in the
UInt8
extensions we have stuff likestatic let newline: UInt8 = 0xA
, this should be spelledstatic let newline = UInt8(ascii: "\n")
, that's much more readable and just as fast. - any reason why all the
parse*
functions carryposition
?ByteBuffer
is designed with areaderIndex
which is exactly what you seem to useposition
for. So there's duplicated information with the possibility of mismatch. - functions like for example
parseSimpleString
should be much easier to implement like this (didn't actually compile it)
extension ByteBuffer {
mutating func readSimpleString() -> ByteBuffer? {
guard let newlineIndex = self.readableBytesView.firstIndex(of: .newline) else {
return nil
}
return self.readSlice(length: newlineIndex.distance(to: self.readerIndex)).readableBytesView.dropLast(2 /* crlf */).map {
ByteBuffer($0)
}
}
}
- this is an example for writing safe & more complicated parsers on
ByteBuffer
.