[jira] [Updated] (MINIFICPP-1329) Fix usages of StringUtils::StringToBool

["Adam Hunyadi (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/MINIFICPP-1329?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Adam Hunyadi updated MINIFICPP-1329:
------------------------------------
    Description: 
*Background:*

Conversions from string to other values in MINIFI usually follow the convention of changing an output value and returning a boolean denoting the success of the conversion. For booleans however, this is not the case:
{code:c++|title=Current Implementation}
 bool StringUtils::StringToBool(std::string input, bool &output) {
  std::transform(input.begin(), input.end(), input.begin(), ::tolower);
  std::istringstream(input) >> std::boolalpha >> output;
  return output;
}
{code}
It is known to be misused in the code, for example this code assumes the return value false corresponds to a parse failure:
 [https://github.com/apache/nifi-minifi-cpp/blob/rel/minifi-cpp-0.7.0/extensions/opc/src/putopc.cpp#L319-L323]

*Proposal:*
 If we want to stay consistent with the other conversions, we can do this:
{code:c++|title=Minimum change for the new implementation}
bool StringUtils::StringToBool(std::string input, bool &output) {
  std::transform(input.begin(), input.end(), input.begin(), ::tolower);
  output = "true" == input; 
  return output || "false" == input;
}
{code}
However, many cases use the return value as the conversion result. One should be cautious:
 # First we should copy the implementation to one with a different name
 # Change the return value to void on the original
 # Until the code compiles:
 ## Eliminate all the usages of return values as parsed values
 ## Redirect the checked value implementations to the copy
 # Change the implementation of the original to return the conversion success
 # Delete the copy
 # Search and replace the name of the copy to the original

We can potentially change the return type to an optional, or a success enum.

  was:
*Background:*

Conversions from string to other values in MINIFI usually follow the convention of changing an output value and returning a boolean denoting the success of the conversion. For booleans however, this is not the case:
{code:c++|title=Current Implementation}
 bool StringUtils::StringToBool(std::string input, bool &output) {
  std::transform(input.begin(), input.end(), input.begin(), ::tolower);
  std::istringstream(input) >> std::boolalpha >> output;
  return output;
}
{code}
It is known to be misused in the code, for example this code assumes the return value false corresponds to a parse failure:
 [https://github.com/apache/nifi-minifi-cpp/blob/rel/minifi-cpp-0.7.0/extensions/opc/src/putopc.cpp#L319-L323]

*Proposal:*
 If we want to stay consistent with the other conversions, we can do this:
{code:c++|title=Minimum change in signature}
bool StringUtils::StringToBool(std::string input, bool &output) {
  std::transform(input.begin(), input.end(), input.begin(), ::tolower);
  output = "true" == input; 
  return output || "false" == input;
}
{code}
However, many cases use the return value as the conversion result. One should be cautious:
 # First we should copy the implementation to one with a different name
 # Change the return value to void on the original
 # Until the code compiles:
 ## Eliminate all the usages of return values as parsed values
 ## Redirect the checked value implementations to the copy
 # Change the implementation of the original to return the conversion success
 # Delete the copy
 # Search and replace the name of the copy to the original

We can potentially change the return type to an optional, or a success enum.


> Fix usages of StringUtils::StringToBool
> ---------------------------------------
>
>                 Key: MINIFICPP-1329
>                 URL: https://issues.apache.org/jira/browse/MINIFICPP-1329
>             Project: Apache NiFi MiNiFi C++
>          Issue Type: Bug
>            Reporter: Adam Hunyadi
>            Priority: Minor
>              Labels: MiNiFi-CPP-Hygiene
>
> *Background:*
> Conversions from string to other values in MINIFI usually follow the convention of changing an output value and returning a boolean denoting the success of the conversion. For booleans however, this is not the case:
> {code:c++|title=Current Implementation}
>  bool StringUtils::StringToBool(std::string input, bool &output) {
>   std::transform(input.begin(), input.end(), input.begin(), ::tolower);
>   std::istringstream(input) >> std::boolalpha >> output;
>   return output;
> }
> {code}
> It is known to be misused in the code, for example this code assumes the return value false corresponds to a parse failure:
>  [https://github.com/apache/nifi-minifi-cpp/blob/rel/minifi-cpp-0.7.0/extensions/opc/src/putopc.cpp#L319-L323]
> *Proposal:*
>  If we want to stay consistent with the other conversions, we can do this:
> {code:c++|title=Minimum change for the new implementation}
> bool StringUtils::StringToBool(std::string input, bool &output) {
>   std::transform(input.begin(), input.end(), input.begin(), ::tolower);
>   output = "true" == input; 
>   return output || "false" == input;
> }
> {code}
> However, many cases use the return value as the conversion result. One should be cautious:
>  # First we should copy the implementation to one with a different name
>  # Change the return value to void on the original
>  # Until the code compiles:
>  ## Eliminate all the usages of return values as parsed values
>  ## Redirect the checked value implementations to the copy
>  # Change the implementation of the original to return the conversion success
>  # Delete the copy
>  # Search and replace the name of the copy to the original
> We can potentially change the return type to an optional, or a success enum.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)