amzn / amazon-pay-sdk-csharp

Amazon Pay C# SDK
https://payments.amazon.com/developer
Apache License 2.0
48 stars 42 forks source link

Adjustments to properly deal with null and empty string configuration values #4

Closed jeremy-loffredo closed 9 years ago

jeremy-loffredo commented 9 years ago

string input was not being reset to null, if merchant_id was ever included in requestParameters, input would still hold a non-null value when reaching the check for platform_id, where it would then try and include a non-existent value from requestParameters.

shrakuma commented 9 years ago

In my logic in the setDefaultValues function, if merchant_id is included in the requestParameters if (fieldMappings.ContainsKey("merchant_id")) .. then it will check if the value in requestParameters contains a value for merchant_id or it will look for config["merchant_id"] contains a value.

then it does the same check for "platform_id" In this case if (requestParameters.ContainsKey("platform_id")) { input = requestParameters["platform_id"].ToString().Trim(); } here if the requestParameters["platform_id"] is null or "" it will add either null or "" to it then the next line input!=null && input!="" will do the required check and if satisfied then adds it to parameters. I am trying to avoid string.isnullorempty as it throws a nullreferenceexception which will need to be handled. throwing/handling the exception can be avoided by my design. I have condensed my design to this

List optionalParams = new List() { "merchant_id", "platform_id", "currency_code" };

        string input = "";
        foreach (string param in optionalParams)
        {
            input = "";
            if (fieldMappings.ContainsKey(param))
            {
                if (requestParameters.ContainsKey(param))
                {
                    input = requestParameters[param].ToString().Trim();
                }
                if (input != null && input != "")
                {
                    parameters[fieldMappings[param]] = requestParameters[param];
                }
                else if (config[param] != null && config[param].ToString().Trim() != "")
                {
                    parameters[fieldMappings[param]] = config[param];
                }
            }
        }

Also your for loop loops through the entire FeildMappings hashtable . in this case the default values should only be checked for specific params as per List optionalParams above

If I am missing something here , My apologies. Please let me know your thoughts

jeremy-loffredo commented 9 years ago

This looks like it will work just fine.

The problem that I was running into with the last update was calling setOrderReferenceValues with merchant_id and currency_code but not platform_id.

In the previous version, input would get set to a non-null value because of the existence of merchant_id, then when testing the value of requestParameters["platform_id"] by testing the value of input, parameters[fieldMappings["platform_id"]] would be set to requestParameters["platform_id"] which is a null value.

When the parameters with the null value reached the HashTableToDictionary call, a null value exception is thrown on kvp.Value.ToString(). (something else that should probably be protected).

As far as isnullorempty throwing a nullexception goes, as long as the proper values are checked at the proper times, it should never throw an exception.

Hashtable test = new Hashtable()
{
    {"key1","value1"},
    {"key2",null}
};

string.IsNullOrEmpty(test["key2"].ToString()); // true
string.IsNullOrEmpty(test["key3"].ToString()); // null value exception

if ( test.ContainsKey("key3") && !string.IsNullOrEmpty(test["key3"].ToString()) ){ ... }
// the IsNullOrEmpty portion will never get called. So there will not be an exception.

Thank you for your help and thoughts.