Peer Review #13
Peer Review #13
Conversation
fixed README
…ueStoreServiceImpl class
Initial stubs for backend key-value store
Service layer stubs
added ServiceLayer class
Service layer: Chirp and Read
Client to server connection and cleanup
Small details, mainly edge cases
| std::cout << "Make sure the parent id actually exists" << std::endl; | ||
| } | ||
| } | ||
| else { |
There was a problem hiding this comment.
In the google style guide, they normally put elses on the same line as the closing bracket for the if
There was a problem hiding this comment.
You should fix this throughout the codebase.
| else { | ||
| int num_chirps = reply.chirps_size(); | ||
| for(int i = 0; i < num_chirps - 1; i++) { | ||
| chirp::Chirp this_chirp = reply.chirps(i); |
There was a problem hiding this comment.
I think you could declare this_chirp outside of the loop
| std::unique_ptr<grpc::ClientReader<chirp::MonitorReply> > stream_handle (stub_->monitor(&context, request)); | ||
| while (true) { | ||
| while (stream_handle->Read(&reply)) { | ||
| chirp::Chirp this_chirp = reply.chirp(); |
There was a problem hiding this comment.
also here, chirp could be declared outside of loop
| if (FLAGS_monitor && my_username != "") { | ||
| client.monitor(my_username, stub_); | ||
| } | ||
|
|
There was a problem hiding this comment.
remove blank line maybe?
| #include "service.pb.h" | ||
| #include <iostream> | ||
| #include <gflags/gflags.h> | ||
| #include "client.h" |
There was a problem hiding this comment.
im p sure that there should be a blank line between iostream and the rest, and iostream should go first
| } | ||
| chirp::Chirp* chirp_pointer = reply->add_chirps(); | ||
|
|
||
| return grpc::Status::OK; |
There was a problem hiding this comment.
I don't know whether return statements need an empty line above them, but it should be consistent, whichever you choose I feel like.
| std::deque<chirp::Chirp> read_chirps = service_.read(request->chirp_id()); | ||
| for (chirp::Chirp c : read_chirps) { | ||
| chirp::Chirp* chirp_pointer = reply->add_chirps(); | ||
| const chirp::Chirp& added_chirp = c; |
There was a problem hiding this comment.
const should start with a k and also these chirps can be declared outside of the loop and then filled in once you enter the loop
| while (keep_monitoring) { | ||
| std::chrono::seconds seconds = std::chrono::duration_cast< std::chrono::seconds >(std::chrono::system_clock::now().time_since_epoch()); | ||
| std::chrono::microseconds useconds = std::chrono::duration_cast< std::chrono::microseconds >(std::chrono::system_clock::now().time_since_epoch()); | ||
| std::deque<chirp::Chirp> found_chirps = service_.monitor(request->username(), initial_time); |
There was a problem hiding this comment.
these three can all be declared before the loop
| initial_time.set_useconds(useconds.count()); | ||
| for (chirp::Chirp c : found_chirps) { | ||
| if(read_chirps.find(c.id()) == read_chirps.end()){ | ||
| chirp::Chirp* this_chirp = new chirp::Chirp(); |
There was a problem hiding this comment.
this_chirp could be declared before while loop
|
|
||
| private: | ||
| // interface for grpc calls to key value store | ||
| KeyValueClient clientInterface; |
There was a problem hiding this comment.
needs an underscore after its name
| std::cout << "Make sure the parent id actually exists" << std::endl; | ||
| } | ||
| } | ||
| else { |
There was a problem hiding this comment.
You should fix this throughout the codebase.
|
|
||
| ServiceLayer::ServiceLayer(KeyValueClientInterface* key_value_connection) { | ||
| store_ = key_value_connection; | ||
| const std::deque<std::string>& chirp_count = store_->get("chirp_count"); |
There was a problem hiding this comment.
Not quite -- k is only for true constants, not just vars that have the const keyword.
| ServiceLayer::ServiceLayer(KeyValueClientInterface* key_value_connection) { | ||
| store_ = key_value_connection; | ||
| const std::deque<std::string>& chirp_count = store_->get("chirp_count"); | ||
| if(chirp_count.size() == 0){ |
There was a problem hiding this comment.
Space after if (everywhere).
| store_->put("chirp_count", "0"); | ||
| } | ||
| } | ||
| bool ServiceLayer::registeruser(const std::string& username) { |
| error_chirp.set_id("ERROR"); | ||
| return error_chirp; | ||
| } | ||
| my_id = chirp_count.at(0); |
There was a problem hiding this comment.
Maybe move this object construction code to a helper?
| } | ||
| } | ||
|
|
||
| return found_chirps; |
There was a problem hiding this comment.
As I mention above, it doesn't apply to all vars that have the const keyword, only those that are truly constants.
No description provided.