Brief code-review #61

New Issue

2019-09-03T20:38:43-04:00

lganzzzo commented

2019-09-03 20:38:43 -04:00

(Migrated from github.com)

Hello @kdeng00 ,

I am Leonid, author of oatpp.

Just stumbled upon your project and was happy to notice that you are using oatpp.
I briefly looked through the controllers code and I have some comments/tips for you:

oatpp::String is already using shared_ptr under the hood, so there is no need to make it shared here.
Just use it directly:
```
auto rawSong = oatpp::String(buf.str().data(), (v_int32)buf.str().size(), true);
```
Here you may opt to use oatpp streaming API (which is not documented yet - unfortunately) in order to stream file contents directly from file - without loading IT into RAM.
In order to do so - take a look at ChunkedBody. Basically what you need is to provide your own ReadCallback to the ChunkedBody. ReadCallback will request you for the next chunk of data (here) giving you buffer that you have to fill. All you need - is to read next chunk of data from the file to this provided buffer.
There is a PR with new Authorization feature which is about to be merged to master.
You may opt to use this newly introduced AUTHORIZATION annotation macro in order to enhance your auth functionality.

Please feel free to contact me in case of any questions - through the Github issues or in the dev-chat on gitter.

Cheers!
Leonid

Hello @kdeng00 , I am Leonid, author of [oatpp](https://github.com/oatpp/oatpp). Just stumbled upon your project and was happy to notice that you are using oatpp. I briefly looked through the controllers code and I have some comments/tips for you: - `oatpp::String` is already using shared_ptr under the hood, so there is no need to make it shared [here](https://github.com/kdeng00/Icarus/blob/master/include/controller/songController.hpp#L156). Just use it directly: ```cpp auto rawSong = oatpp::String(buf.str().data(), (v_int32)buf.str().size(), true); ``` - [Here](https://github.com/kdeng00/Icarus/blob/master/include/controller/songController.hpp#L147) you may opt to use oatpp streaming API (which is not documented yet - unfortunately) in order to stream file contents directly from file - without loading IT into RAM. In order to do so - take a look at [ChunkedBody](https://github.com/oatpp/oatpp/blob/master/src/oatpp/web/protocol/http/outgoing/ChunkedBody.hpp). Basically what you need is to provide your own [ReadCallback](https://github.com/oatpp/oatpp/blob/master/src/oatpp/core/data/stream/Stream.hpp#L518) to the `ChunkedBody`. `ReadCallback` will request you for the next chunk of data ([here](https://github.com/oatpp/oatpp/blob/master/src/oatpp/core/data/stream/Stream.hpp#L534)) giving you buffer that you have to fill. All you need - is to read next chunk of data from the file to this provided buffer. - There is a [PR](https://github.com/oatpp/oatpp/pull/114) with new Authorization feature which is about to be merged to master. You may opt to use this newly introduced `AUTHORIZATION` annotation macro in order to enhance your [auth functionality](https://github.com/kdeng00/Icarus/blob/master/include/controller/songController.hpp#L49). Please feel free to contact me in case of any questions - through the Github issues or in the [dev-chat on gitter](https://gitter.im/oatpp-framework/Lobby). Cheers! Leonid

kdeng00 commented

2019-09-04 08:28:07 -04:00

(Migrated from github.com)

Hey @lganzzzo, thanks for the tips and the comments are welcomed! I was actually thinking about bringing up Authorization on the gitter chat but it seems like it's already at the point where there is a PR. I believe it's a great thing you're doing with oatpp, it's rare to find something like this that is so well documented and robust.

lganzzzo commented

2019-09-04 11:09:46 -04:00

(Migrated from github.com)

Hey @kdeng00 ,

Thank you for the feedback! I am happy that oatpp can help your project!

Hey @kdeng00 , Thank you for the feedback! I am happy that oatpp can help your project!

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: phoenix/icarus#61