Improve decoder bunq/sdk csharp#43 #57

OGKevin · 2017-12-20T15:45:59Z

Closes #56
Closes #43

epels

Looks good overall, just a few things that are not necessarily good or bad, as long as it's done with careful consideration

epels · 2017-12-21T11:02:52Z

BunqSdk.Tests/Model/Generated/Object/NotificationUrlTest.cs

-            string referencedObjectPropertyName
+            Type classTypeExpected,
+            string referencedObjectPropertyName,
+            string subClassObjectPropertyName = null,


Considering creating an overload for this method that omits optional params

epels · 2017-12-21T11:06:35Z

BunqSdk.Tests/Model/Generated/Object/NotificationUrlTest.cs

+            Assert.Equal(classTypeExpected, referencedModel.GetType());
+
+            if (subClassObjectPropertyName == null || subClassTypeExpected == null) return;
+            var subClass = referencedModel.GetType().GetProperty(subClassObjectPropertyName).GetValue(


Push invocations to newlines, e.g.

referencedModel.GetType() .GetProperty(subClassObjectPropertyName) .GetValue(referencedModel);

epels · 2017-12-21T11:19:48Z

BunqSdk.Tests/Model/Generated/Object/NotificationUrlTest.cs

-            Assert.IsType(classNameExpected, referencedModel);
-            Assert.Equal(classNameExpected, referencedModel.GetType());
+            Assert.IsType(classTypeExpected, referencedModel);
+            Assert.Equal(classTypeExpected, referencedModel.GetType());


Looks like these two assertions (L80 & L81) do the exact same thing

I'd personally go with Assert.IsType(). Just make sure it's consistent with the assertion below (L88)

epels · 2017-12-21T11:21:21Z

BunqSdk/Json/AnchorObjectConverter.cs

+
+        public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
+        {
+


Remove newline

epels · 2017-12-21T11:21:25Z

BunqSdk/Json/AnchorObjectConverter.cs

+        {
+
+            new JsonSerializer().Serialize(writer, value);
+


Remove newline

epels · 2017-12-21T11:29:44Z

BunqSdk/Json/AnchorObjectConverter.cs

+            var model = (IAnchorObjectInterface) jsonSerializer.Deserialize(token.CreateReader(), objectType);
+
+            if (!model.IsAllFieldNull()) return model;
+            var fields = objectType.GetProperties();


epels · 2017-12-21T11:30:25Z

BunqSdk/Json/AnchorObjectConverter.cs

+
+        public override bool CanConvert(Type objectType)
+        {
+            throw new NotImplementedException();


Is this intended? If so, maybe add a comment.

epels · 2017-12-21T11:38:22Z

BunqSdk/Json/BunqContractResolver.cs

+
+            foreach (var type in typesToExclude)
+            {
+                if (converterRegistry.ContainsKey(type))


No need to check if the key is present: Dictionary.Remove(). If the key is not found, it returns false but no exception is thrown

epels · 2017-12-21T11:48:23Z

BunqSdk/Model/Core/AnchorObjectInterface.cs

+
+        BunqModel GetReferencedObject();
+    }
+}


Add newline at EOF

epels · 2017-12-21T11:51:33Z

BunqSdk/Model/Core/BunqModel.cs

@@ -146,5 +146,7 @@ public override string ToString()
        {
            return BunqJsonConvert.SerializeObject(this);
        }
+
+        public abstract bool IsAllFieldNull();


Is it correct BunqModel does not implement IAnchorObjectInterface, but still defines a IsAllFieldNull() method? Consider extracting this method to its own interface, and implement that.

epels

Small comment.

epels · 2017-12-21T13:10:47Z

BunqSdk/Json/AnchorObjectConverter.cs

@@ -55,7 +55,7 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist

        public override bool CanConvert(Type objectType)
        {
-            throw new NotImplementedException();
+            return objectType == typeof(IAnchorObjectInterface);


You're probably looking for Type.IsAssignableFrom here? This will return false for types implementing IAnchorObjectInterface.

epels

LGTM

epels · 2017-12-21T13:28:12Z

BunqSdk/Json/AnchorObjectConverter.cs

+
+        public override bool CanConvert(Type objectType)
+        {
+            return objectType.IsAssignableFrom(typeof(IAnchorObjectInterface));


Shouldn't this be inverted? E.g. return typeof(IAnchorObjectInterface).IsAssignableFrom(objectType); checks whether objectType implements IAnchorObjectInterface

rip Correct 😁

OGKevin · 2017-12-21T13:38:31Z

@andrederoos All yours please 👀

andrederoos · 2017-12-21T15:00:20Z

BunqSdk.Tests/Model/Generated/Object/NotificationUrlTest.cs

+            Type classTypeExpected,
+            string referencedObjectPropertyName,
+            string subClassObjectPropertyName = null,
+            Type subClassTypeExpected = null) {


please lookup c# style conventions... I think ) { needs to be on a new line.

The SDK has the following structure:

private BunqResponseRaw SendRequest(HttpRequestMessage requestMessage, IDictionary<string, string> customHeaders, string uriRelative) {

Only { on its own line without )

OGKevin · 2017-12-21T15:14:24Z

@andrederoos all yours again

OGKevin added 10 commits December 20, 2017 12:07

Modefied tests to use parent object. (#43)

1312147

Added converter for anchor objects. (#43)

63c180b

Added interface for anchor objects. (#43)

c6b36a0

Added abstract method for are all fields null. (#43)

f31bd37

Regenerated code. (#43)

268a7e4

Implemeted missing methods in core models. (#43)

293727b

Finished notification url test. (#43)

7fdba57

Finished converter and contract resolver. (#43)

a5efc63

Renamed areAllFieldNull to isAllFieldNull(). (#46)

0611f3b

Regnerated code. (#46)

d6a489a

OGKevin added this to the 0.12.4 milestone Dec 20, 2017

OGKevin assigned andrederoos and OGKevin Dec 20, 2017

OGKevin requested a review from andrederoos December 20, 2017 15:46

epels reviewed Dec 21, 2017

View reviewed changes

OGKevin added 6 commits December 21, 2017 13:52

Added entries to git ignore and modified .idea files. (#43)

e327ecc

Refactored the way reflection is used. (#43)

9d7955d

Implemeted canConvert. (sdk_csharp#43)

2a071fa

Fixed indentation and added missing new line. (sdk_csharp#43)

e4e36d4

Removed redundant check. (sdk_csharp#43)

d4b49d2

Added new line at EOF. (sdk_csharp#43)

92cd8ee

epels reviewed Dec 21, 2017

View reviewed changes

Using IsAssignableFrom to compare types. (#43)

3e290c8

epels approved these changes Dec 21, 2017

View reviewed changes

epels reviewed Dec 21, 2017

View reviewed changes

Flipped IsAssignableFrom statement. (#43)

43d9903

andrederoos requested changes Dec 21, 2017

View reviewed changes

Added { on its own line. (#43)

fb0d1fa

andrederoos approved these changes Dec 21, 2017

View reviewed changes

andrederoos merged commit 118bed4 into develop Dec 21, 2017

andrederoos deleted the improve_decoder_bunq/sdk_csharp#43 branch December 21, 2017 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve decoder bunq/sdk csharp#43 #57

Improve decoder bunq/sdk csharp#43 #57

OGKevin commented Dec 20, 2017

epels left a comment

epels Dec 21, 2017

epels Dec 21, 2017

epels Dec 21, 2017

epels Dec 21, 2017

epels Dec 21, 2017

epels Dec 21, 2017

epels Dec 21, 2017

epels Dec 21, 2017

epels Dec 21, 2017

epels Dec 21, 2017

epels Dec 21, 2017

epels left a comment

epels Dec 21, 2017

epels left a comment

epels Dec 21, 2017

epels Dec 21, 2017

OGKevin Dec 21, 2017

OGKevin commented Dec 21, 2017

andrederoos Dec 21, 2017

OGKevin Dec 21, 2017

andrederoos Dec 21, 2017

OGKevin commented Dec 21, 2017


		public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
		{

Improve decoder bunq/sdk csharp#43 #57

Improve decoder bunq/sdk csharp#43 #57

Conversation

OGKevin commented Dec 20, 2017

epels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin commented Dec 21, 2017