Refactored based on code review

3 files changed, 107 insertions, 175 deletions

diff --git a/test/cpp/util/grpc_tool.cc b/test/cpp/util/grpc_tool.cc
index b4eeee1a9b..a82e7418ab 100644
--- a/test/cpp/util/grpc_tool.cc
+++ b/test/cpp/util/grpc_tool.cc
@@ -193,8 +193,9 @@ void ReadResponse(CliCall* call, const grpc::string& method_name,
     fprintf(stderr, "got response.\n");
     if (!FLAGS_binary_output) {
       gpr_mu_lock(parser_mu);
-      serialized_response_proto = parser->GetTextFormatFromMethod(
-          method_name, serialized_response_proto, false /* is_request */);
+      serialized_response_proto = parser->GetFormattedStringFromMethod(
+          method_name, serialized_response_proto, false /* is_request */,
+          FLAGS_json_output);
       if (parser->HasError() && print_mode) {
         fprintf(stderr, "Failed to parse response.\n");
       }
@@ -555,12 +556,9 @@ bool GrpcTool::CallMethod(int argc, const char** argv,
           request_text.clear();
         } else {
           gpr_mu_lock(&parser_mu);
-          serialized_request_proto =
-              FLAGS_json_input ?
-                  parser->GetSerializedProtoFromMethodJsonFormat(
-                      method_name, request_text, true /* is_request */) :
-                  parser->GetSerializedProtoFromMethodTextFormat(
-                      method_name, request_text, true /* is_request */);
+          serialized_request_proto = parser->GetSerializedProtoFromMethod(
+              method_name, request_text, true /* is_request */,
+              FLAGS_json_input);
           request_text.clear();
           if (parser->HasError()) {
             if (print_mode) {
@@ -643,12 +641,9 @@ bool GrpcTool::CallMethod(int argc, const char** argv,
             serialized_request_proto = request_text;
             request_text.clear();
           } else {
-            serialized_request_proto =
-                FLAGS_json_input ?
-                    parser->GetSerializedProtoFromMethodJsonFormat(
-                        method_name, request_text, true /* is_request */) :
-                    parser->GetSerializedProtoFromMethodTextFormat(
-                        method_name, request_text, true /* is_request */);
+            serialized_request_proto = parser->GetSerializedProtoFromMethod(
+                method_name, request_text, true /* is_request */,
+                FLAGS_json_input);
             request_text.clear();
             if (parser->HasError()) {
               if (print_mode) {
@@ -684,14 +679,10 @@ bool GrpcTool::CallMethod(int argc, const char** argv,
                 break;
               }
             } else {
-              grpc::string response_text =
-                  FLAGS_json_output ?
-                      parser->GetJsonFormatFromMethod(method_name,
+              grpc::string response_text = parser->GetFormattedStringFromMethod(method_name,
                                                       serialized_response_proto,
-                                                      false /* is_request */) :
-                      parser->GetTextFormatFromMethod(method_name,
-                                                      serialized_response_proto,
-                                                      false /* is_request */);
+                                                      false /* is_request */,
+                                                      FLAGS_json_output);
 
               if (parser->HasError() && print_mode) {
                 fprintf(stderr, "Failed to parse response.\n");
@@ -748,12 +739,8 @@ bool GrpcTool::CallMethod(int argc, const char** argv,
     if (FLAGS_binary_input) {
       serialized_request_proto = request_text;
     } else {
-      serialized_request_proto =
-          FLAGS_json_input ?
-              parser->GetSerializedProtoFromMethodJsonFormat(
-                  method_name, request_text, true /* is_request */) :
-              parser->GetSerializedProtoFromMethodTextFormat(
-                  method_name, request_text, true /* is_request */);
+      serialized_request_proto = parser->GetSerializedProtoFromMethod(
+          method_name, request_text, true /* is_request */, FLAGS_json_input);
       if (parser->HasError()) {
         fprintf(stderr, "Failed to parse request.\n");
         return false;
@@ -776,18 +763,14 @@ bool GrpcTool::CallMethod(int argc, const char** argv,
              &serialized_response_proto,
              receive_initial_metadata ? &server_initial_metadata : nullptr);
          receive_initial_metadata = false) {
-      if (FLAGS_json_output) {
-        serialized_response_proto = parser->GetJsonFormatFromMethod(
-            method_name, serialized_response_proto, false /* is_request */);
-      } else if (!FLAGS_binary_output) {
-        serialized_response_proto = parser->GetTextFormatFromMethod(
-            method_name, serialized_response_proto, false /* is_request */);
-      }
-      if (FLAGS_json_output || !FLAGS_binary_output) {
-          if (parser->HasError()) {
-            fprintf(stderr, "Failed to parse response.\n");
-            return false;
-          }
+      if (!FLAGS_binary_output) {
+        serialized_response_proto = parser->GetFormattedStringFromMethod(
+            method_name, serialized_response_proto, false /* is_request */,
+            FLAGS_json_output);
+        if (parser->HasError()) {
+          fprintf(stderr, "Failed to parse response.\n");
+          return false;
+        }
       }
 
       if (receive_initial_metadata) {
@@ -878,12 +861,8 @@ bool GrpcTool::ParseMessage(int argc, const char** argv,
   if (FLAGS_binary_input) {
     serialized_request_proto = message_text;
   } else {
-    serialized_request_proto =
-        FLAGS_json_input ?
-            parser->GetSerializedProtoFromMessageTypeJsonFormat(type_name,
-                                                                message_text) :
-            parser->GetSerializedProtoFromMessageTypeTextFormat(type_name,
-                                                                message_text);
+    serialized_request_proto = parser->GetSerializedProtoFromMessageType(
+        type_name, message_text, FLAGS_json_input);
     if (parser->HasError()) {
       fprintf(stderr, "Failed to serialize the message.\n");
       return false;
@@ -894,14 +873,8 @@ bool GrpcTool::ParseMessage(int argc, const char** argv,
     output_ss << serialized_request_proto;
   } else {
     grpc::string output_text;
-    if (FLAGS_json_output) {
-      output_text = parser->GetJsonFormatFromMessageType(
-          type_name, serialized_request_proto);
-    } else {
-      output_text = parser->GetTextFormatFromMessageType(
-          type_name, serialized_request_proto);
-    }
-
+    output_text = parser->GetFormattedStringFromMessageType(
+        type_name, serialized_request_proto, FLAGS_json_output);
     if (parser->HasError()) {
       fprintf(stderr, "Failed to deserialize the message.\n");
       return false;
diff --git a/test/cpp/util/proto_file_parser.cc b/test/cpp/util/proto_file_parser.cc
index 997808cbc3..71bda1005d 100644
--- a/test/cpp/util/proto_file_parser.cc
+++ b/test/cpp/util/proto_file_parser.cc
@@ -216,55 +216,33 @@ bool ProtoFileParser::IsStreaming(const grpc::string& method, bool is_request) {
                     : method_desc->server_streaming();
 }
 
-grpc::string ProtoFileParser::GetSerializedProtoFromMethodTextFormat(
-    const grpc::string& method, const grpc::string& text_format_proto,
-    bool is_request) {
+grpc::string ProtoFileParser::GetSerializedProtoFromMethod(
+    const grpc::string& method, const grpc::string& formatted_proto,
+    bool is_request, bool is_json_format) {
   has_error_ = false;
   grpc::string message_type_name = GetMessageTypeFromMethod(method, is_request);
   if (has_error_) {
     return "";
   }
-  return GetSerializedProtoFromMessageTypeTextFormat(message_type_name,
-                                           text_format_proto);
+  return GetSerializedProtoFromMessageType(message_type_name, formatted_proto,
+                                           is_json_format);
 }
 
-grpc::string ProtoFileParser::GetSerializedProtoFromMethodJsonFormat(
-    const grpc::string& method, const grpc::string& json_format_proto,
-    bool is_request) {
-  has_error_ = false;
-  grpc::string message_type_name = GetMessageTypeFromMethod(method, is_request);
-  if (has_error_) {
-    return "";
-  }
-  return GetSerializedProtoFromMessageTypeJsonFormat(message_type_name,
-                                           json_format_proto);
-}
-
-grpc::string ProtoFileParser::GetTextFormatFromMethod(
-    const grpc::string& method, const grpc::string& serialized_proto,
-    bool is_request) {
-  has_error_ = false;
-  grpc::string message_type_name = GetMessageTypeFromMethod(method, is_request);
-  if (has_error_) {
-    return "";
-  }
-  return GetTextFormatFromMessageType(message_type_name, serialized_proto);
-}
-
-grpc::string ProtoFileParser::GetJsonFormatFromMethod(
+grpc::string ProtoFileParser::GetFormattedStringFromMethod(
     const grpc::string& method, const grpc::string& serialized_proto,
-    bool is_request) {
+    bool is_request, bool is_json_format) {
   has_error_ = false;
   grpc::string message_type_name = GetMessageTypeFromMethod(method, is_request);
   if (has_error_) {
     return "";
   }
-  return GetJsonFormatFromMessageType(message_type_name, serialized_proto);
+  return GetFormattedStringFromMessageType(message_type_name, serialized_proto, is_json_format);
 }
 
-grpc::string ProtoFileParser::GetSerializedProtoFromMessageTypeTextFormat(
+grpc::string ProtoFileParser::GetSerializedProtoFromMessageType(
     const grpc::string& message_type_name,
-    const grpc::string& text_format_proto) {
+    const grpc::string& formatted_proto,
+    bool is_json_format) {
   has_error_ = false;
   grpc::string serialized;
   const protobuf::Descriptor* desc =
@@ -275,38 +253,24 @@ grpc::string ProtoFileParser::GetSerializedProtoFromMessageTypeTextFormat(
   }
   std::unique_ptr<grpc::protobuf::Message> msg(
       dynamic_factory_->GetPrototype(desc)->New());
-  bool ok = protobuf::TextFormat::ParseFromString(text_format_proto, msg.get());
-  if (!ok) {
-    LogError("Failed to parse text format to proto.");
-    return "";
-  }
-  ok = msg->SerializeToString(&serialized);
-  if (!ok) {
-    LogError("Failed to serialize proto.");
-    return "";
-  }
-  return serialized;
-}
 
-grpc::string ProtoFileParser::GetSerializedProtoFromMessageTypeJsonFormat(
-    const grpc::string& message_type_name,
-    const grpc::string& json_format_proto) {
-  has_error_ = false;
-  grpc::string serialized;
-  const protobuf::Descriptor* desc =
-      desc_pool_->FindMessageTypeByName(message_type_name);
-  if (!desc) {
-    LogError("Message type not found");
-    return "";
+  bool ok;
+  if (is_json_format) {
+    ok = grpc::protobuf::json::JsonStringToMessage(formatted_proto, msg.get())
+        .ok();
+    if (!ok) {
+      LogError("Failed to convert json format to proto.");
+      return "";
+    }
+  } else {
+    ok = protobuf::TextFormat::ParseFromString(formatted_proto, msg.get());
+    if (!ok) {
+      LogError("Failed to convert text format to proto.");
+      return "";
+    }
   }
-  std::unique_ptr<grpc::protobuf::Message> msg(
-      dynamic_factory_->GetPrototype(desc)->New());
 
-  if (!grpc::protobuf::json::JsonStringToMessage(json_format_proto, msg.get()).ok()) {
-    LogError("Failed to parse json format to proto.");
-    return "";
-  }
-  bool ok = msg->SerializeToString(&serialized);
+  ok = msg->SerializeToString(&serialized);
   if (!ok) {
     LogError("Failed to serialize proto.");
     return "";
@@ -314,9 +278,10 @@ grpc::string ProtoFileParser::GetSerializedProtoFromMessageTypeJsonFormat(
   return serialized;
 }
 
-grpc::string ProtoFileParser::GetTextFormatFromMessageType(
+grpc::string ProtoFileParser::GetFormattedStringFromMessageType(
     const grpc::string& message_type_name,
-    const grpc::string& serialized_proto) {
+    const grpc::string& serialized_proto,
+    bool is_json_format) {
   has_error_ = false;
   const protobuf::Descriptor* desc =
       desc_pool_->FindMessageTypeByName(message_type_name);
@@ -330,38 +295,24 @@ grpc::string ProtoFileParser::GetTextFormatFromMessageType(
     LogError("Failed to deserialize proto.");
     return "";
   }
-  grpc::string text_format;
-  if (!protobuf::TextFormat::PrintToString(*msg.get(), &text_format)) {
-    LogError("Failed to print proto message to text format");
-    return "";
-  }
-  return text_format;
-}
+  grpc::string formatted_string;
 
-grpc::string ProtoFileParser::GetJsonFormatFromMessageType(
-    const grpc::string& message_type_name,
-    const grpc::string& serialized_proto) {
-  has_error_ = false;
-  const protobuf::Descriptor* desc =
-      desc_pool_->FindMessageTypeByName(message_type_name);
-  if (!desc) {
-    LogError("Message type not found");
-    return "";
-  }
-  std::unique_ptr<grpc::protobuf::Message> msg(
-      dynamic_factory_->GetPrototype(desc)->New());
-  if (!msg->ParseFromString(serialized_proto)) {
-    LogError("Failed to deserialize proto.");
-    return "";
-  }
-  grpc::string json_format;
-  grpc::protobuf::json::JsonPrintOptions jsonPrintOptions;
-  jsonPrintOptions.add_whitespace = true;
-  if (!grpc::protobuf::json::MessageToJsonString(*msg.get(), &json_format, jsonPrintOptions).ok()) {
-    LogError("Failed to print proto message to json format");
-    return "";
+  if (is_json_format) {
+    grpc::protobuf::json::JsonPrintOptions jsonPrintOptions;
+    jsonPrintOptions.add_whitespace = true;
+    if (!grpc::protobuf::json::MessageToJsonString(*msg.get(),
+                                                   &formatted_string,
+                                                   jsonPrintOptions).ok()) {
+      LogError("Failed to print proto message to json format");
+      return "";
+    }
+  } else {
+    if (!protobuf::TextFormat::PrintToString(*msg.get(), &formatted_string)) {
+      LogError("Failed to print proto message to text format");
+      return "";
+    }
   }
-  return json_format;
+  return formatted_string;
 }
 
 void ProtoFileParser::LogError(const grpc::string& error_msg) {
diff --git a/test/cpp/util/proto_file_parser.h b/test/cpp/util/proto_file_parser.h
index d2b2bd753e..0758d2f9f3 100644
--- a/test/cpp/util/proto_file_parser.h
+++ b/test/cpp/util/proto_file_parser.h
@@ -53,37 +53,45 @@ class ProtoFileParser {
   // used as the argument of Stub::Call()
   grpc::string GetFormattedMethodName(const grpc::string& method);
 
-  grpc::string GetSerializedProtoFromMethodTextFormat(
-      const grpc::string& method, const grpc::string& text_format_proto,
-      bool is_request);
-
-  grpc::string GetSerializedProtoFromMethodJsonFormat(
-      const grpc::string& method, const grpc::string& json_format_proto,
-      bool is_request);
-
-  grpc::string GetTextFormatFromMethod(const grpc::string& method,
-                                       const grpc::string& serialized_proto,
-                                       bool is_request);
-
-  grpc::string GetJsonFormatFromMethod(const grpc::string& method,
-                                       const grpc::string& serialized_proto,
-                                       bool is_request);
-
-  grpc::string GetSerializedProtoFromMessageTypeTextFormat(
+  /// Converts a text or json string to its binary proto representation for the
+  /// given method's input or return type.
+  /// \param method the name of the method (does not need to be fully qualified name)
+  /// \param formatted_proto the text- or json-formatted proto string
+  /// \param is_request if \c true the resolved type is that of the input parameter of
+  ///        the method, otherwise it is the output type
+  /// \param is_json_format if \c true the \c formatted_proto is treated as a
+  ///        json-formatted proto, otherwise it is treated as a text-formatted proto
+  /// \return the serialised binary proto represenation of \c formatted_proto
+  grpc::string GetSerializedProtoFromMethod(
+      const grpc::string& method, const grpc::string& formatted_proto,
+      bool is_request, bool is_json_format);
+
+  /// Converts a text or json string to its proto representation for the given
+  /// message type.
+  /// \param formatted_proto the text- or json-formatted proto string
+  /// \return the serialised binary proto represenation of \c formatted_proto
+  grpc::string GetSerializedProtoFromMessageType(
       const grpc::string& message_type_name,
-      const grpc::string& text_format_proto);
-
-  grpc::string GetSerializedProtoFromMessageTypeJsonFormat(
-      const grpc::string& message_type_name,
-      const grpc::string& json_format_proto);
-
-  grpc::string GetTextFormatFromMessageType(
-      const grpc::string& message_type_name,
-      const grpc::string& serialized_proto);
+      const grpc::string& formatted_proto,
+      bool is_json_format);
+
+  /// Converts a binary proto string to its text or json string representation
+  /// for the given method's input or return type.
+  /// \param method the name of the method (does not need to be a fully qualified name)
+  /// \param the serialised binary proto representation of type \c message_type_name
+  /// \return the text- or json-formatted proto string of \c serialized_proto
+  grpc::string GetFormattedStringFromMethod(const grpc::string& method,
+                                       const grpc::string& serialized_proto,
+                                       bool is_request, bool is_json_format);
 
-  grpc::string GetJsonFormatFromMessageType(
+  /// Converts a binary proto string to its text or json string representation
+  /// for the given message type.
+  /// \param the serialised binary proto representation of type \c message_type_name
+  /// \return the text- or json-formatted proto string of \c serialized_proto
+  grpc::string GetFormattedStringFromMessageType(
       const grpc::string& message_type_name,
-      const grpc::string& serialized_proto);
+      const grpc::string& serialized_proto,
+      bool is_json_format);
 
   bool IsStreaming(const grpc::string& method, bool is_request);