Feat: Added OptimizelyJson class to support GetFeatureVariableJson #214
Conversation
| logger.Log(LogLevel.ERROR, ex.Message); | ||
| } | ||
| } | ||
| public OptimizelyJson(Dictionary<string, object> payload, ILogger logger) |
| { | ||
| try | ||
| { | ||
| Dict = Newtonsoft.Json.JsonConvert.DeserializeObject<Dictionary<string, object>>(payload); |
There was a problem hiding this comment.
Don't Deserialize here.
There was a problem hiding this comment.
But check in other SDKs, if string is not valid, what's the behavior.
There was a problem hiding this comment.
If Deserializing here, there should be no reference of Jobect inside Dict.
|
|
||
| public Dictionary<string, object> ToDictionary() | ||
| { | ||
| return Dict; |
There was a problem hiding this comment.
Before returning it, Convert all JObjects, JArrays ... to IList and IDictionary.
There was a problem hiding this comment.
Please do requested changes. I have seen variables are declared as Pascal case, it should be camel case inside method.
Second thing, instead of manually asserted every field, use TestData.CompareObject.
Third thing, I have added a test case, please complete that.
Once it's done, it's lgtm.
| return default(T); | ||
| } | ||
|
|
||
| private T GetObject<T>(object o) |
There was a problem hiding this comment.
this approach doesn't look good to me. Second thing, if we really need to check. use
as instead of catching exception and doing nothing.
Revise this method.
OptimizelySDK/OptimizelyJson.cs
Outdated
| { | ||
| try | ||
| { | ||
| return (T)o; |
There was a problem hiding this comment.
(T)o
} else {
...
}```
There was a problem hiding this comment.
please add these quick tests for getValue:
- jsonPath: "field4." - invalid path
- jsonPath: "field4..inner_field1" - invalid path
- check if the object is not modified: for that call getValue twice with the valid non empty json path ( "field4.inner_field1" will do)
otherwise it looks good.
| { | ||
| var payload = "{ \"field1\" : {\"1\":\"Csharp\",\"2\":\"Java\"} }"; | ||
| var optimizelyJSONUsingString = new OptimizelyJson(payload, ErrorHandlerMock.Object, LoggerMock.Object); | ||
| var expectedValue = optimizelyJSONUsingString.GetValue<Dictionary<float, string>>("field1"); |
There was a problem hiding this comment.
shouldn't it be <string, string> instead of <float, string> ?
There was a problem hiding this comment.
No, Actually it's to test that when invalid type is passed in getValue it will return default value.
Summary