UserContext building without static references. (bunq/sdk_java#93) #99

tubbynl · 2018-06-14T20:20:44Z

for prior art see #98

i'm attempting some minor (non-breaking) changes to enable creation of the UserContext without referencing the static BunqContext

References #93

tubbynl · 2018-06-14T20:23:06Z

i noticed that the actual user contained in the UserContext is the first User accessible for within the ApiContext. I would expect that the userId contained in the SessionContext would specify the User

tubbynl · 2018-06-15T08:25:13Z

test fail on
com.bunq.sdk.model.generated.endpoint.TabUsageSingleTest > classMethod FAILED
com.bunq.sdk.exception.TooManyRequestsException at TabUsageSingleTest.java:30

perhaps the API-key used for testing should be bumped

OGKevin · 2018-06-17T06:34:07Z

@tubbynl please rebase your commits to follow the following pattern.

sdk_java/.zappr.yaml

Line 7 in 7ed15ad

- '([A-Za-z0-9 ]+)\. (\(bunq\/sdk_java#[0-9]+\))'

UserContext building without static references, backwards compatible

OGKevin · 2018-06-17T19:20:50Z

src/main/java/com/bunq/sdk/context/SessionContext.java

@@ -101,5 +101,4 @@ Date getExpiryTime() {
  public Integer getUserId() {
    return userId;
  }
-


please revert the removal of this line.

please revert the removal of this line. As described here: https://google.github.io/styleguide/javaguide.html#s4.6.1-vertical-whitespace

OGKevin · 2018-06-17T19:22:08Z

src/main/java/com/bunq/sdk/context/UserContext.java

-    } else if (user instanceof UserCompany) {
-      this.userCompany = (UserCompany) user;
+  private void initUser(User user) {
+    if (user!=null && user.getUserPerson()!=null) {


why are you not using instaceof anymore ?

The original user var was based on User#getReferencedObject internally retrieved the BunqModel from either userPerson or userCompany, directly using the relevant accessors omitted the need to instanceof check

This increases the complexity of the if statement without any benefits. So please revert back to using instaceof

removed the null-checks, the risk for nullpointers is the same as the original code, extra checking was merely defensive, it now has the same complexity

btw; your code review thingie should mark theinstanceofstatement as some form of technical debt, it is not best practice and is generally avoidable (and is a code-smell)

OGKevin · 2018-06-17T19:22:26Z

src/main/java/com/bunq/sdk/context/UserContext.java

-        return;
-      }
+  private void initMainMonetaryAccount(MonetaryAccountBank monetaryAccountBank) {
+    if(monetaryAccountBank==null) {


missing space around operators

OGKevin · 2018-06-17T19:23:36Z

src/main/java/com/bunq/sdk/context/UserContext.java

-      }
+  private void initMainMonetaryAccount(MonetaryAccountBank monetaryAccountBank) {
+    if(monetaryAccountBank==null) {
+      throw new BunqException(ERROR_NO_ACTIVE_MONETARY_ACCOUNT_FOUND);


It does not make sense to throw this error here. It would made sense if this error says monetaryAccountBank can not be null. It being null will not automatically mean that no active MA has been found.

the error was kept for consistency; the helper now returns first active monetaryAccountBank or null, so if null it means no active monetaryAccountBank has been found

OGKevin · 2018-06-17T19:24:41Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+  }
+
+  public User getFirstUser() {
+    BunqResponseRaw responseRaw = getRawResponse("user");


please extract magic value in constant.

OGKevin · 2018-06-17T19:26:29Z

src/test/java/com/bunq/sdk/context/UserContextTest.java

+        assertNotNull(sut.getUserId());
+        assertNotNull(sut.getMainMonetaryAccountId());
+    }
+}


missing new line at EOF

OGKevin · 2018-06-17T19:26:40Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+        return monetaryAccountBank;
+      }
+    }
+    return null;


missing new line before return statement.

Dont return null here, throw the exception here.

OGKevin · 2018-06-17T19:26:52Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+  public MonetaryAccountBank getFirstActiveMonetaryAccountBank(Integer userId) {
+    BunqResponseRaw responseRaw = getRawResponse(String.format("user/%s/monetary-account-bank", userId));
+    BunqResponse<List<MonetaryAccountBank>> response = fromJsonList(MonetaryAccountBank.class, responseRaw, MonetaryAccountBank.class.getSimpleName());
+    for (MonetaryAccountBank monetaryAccountBank : response.getValue()) {


missing new line before loop.

OGKevin · 2018-06-17T19:27:50Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+  }
+
+  public MonetaryAccountBank getFirstActiveMonetaryAccountBank(Integer userId) {
+    BunqResponseRaw responseRaw = getRawResponse(String.format("user/%s/monetary-account-bank", userId));


please extract magic value into constant.

OGKevin · 2018-06-17T19:28:05Z

src/test/java/com/bunq/sdk/context/UserContextTest.java

+import com.bunq.sdk.BunqSdkTestBase;
+import org.junit.Test;
+
+import static org.junit.Assert.*;


now wild imports please.

OGKevin · 2018-06-18T07:24:46Z

src/main/java/com/bunq/sdk/context/UserContext.java

-    } else if (user instanceof UserCompany) {
-      this.userCompany = (UserCompany) user;
+  private void initUser(User user) {
+    if (user!=null && user.getUserPerson()!=null) {


This increases the complexity of the if statement without any benefits. So please revert back to using instaceof

OGKevin · 2018-06-18T07:25:02Z

src/main/java/com/bunq/sdk/context/SessionContext.java

@@ -101,5 +101,4 @@ Date getExpiryTime() {
  public Integer getUserId() {
    return userId;
  }
-


OGKevin · 2018-06-18T07:26:05Z

src/main/java/com/bunq/sdk/context/UserContext.java

-      }
+  private void initMainMonetaryAccount(MonetaryAccountBank monetaryAccountBank) {
+    if(monetaryAccountBank == null) {
+      throw new BunqException(ERROR_NO_ACTIVE_MONETARY_ACCOUNT_FOUND);


It does not make sense to throw an error here saying No active MA has been found The error should say that the parameter can not be null.

OGKevin · 2018-06-18T07:26:47Z

src/main/java/com/bunq/sdk/context/UserContext.java

-        return;
-      }
+  private void initMainMonetaryAccount(MonetaryAccountBank monetaryAccountBank) {
+    if(monetaryAccountBank == null) {


missing space after if

OGKevin · 2018-06-18T07:27:34Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+    BunqResponseRaw responseRaw = getRawResponse(USER_ENDPOINT_URL_LISTING);
+    BunqResponse<List<User>> response = fromJsonList(User.class, responseRaw);
+
+


This new line can go.

OGKevin · 2018-06-18T07:27:56Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+    return response.getValue().stream().findFirst().orElse(null);
+  }
+
+  public MonetaryAccountBank getFirstActiveMonetaryAccountBank(Integer userId) {


...byUserId

OGKevin · 2018-06-18T07:28:30Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+    BunqResponse<List<MonetaryAccountBank>> response = fromJsonList(MonetaryAccountBank.class, responseRaw, MonetaryAccountBank.class.getSimpleName());
+
+    return response.getValue().stream()
+            .filter(monetaryAccountBank->monetaryAccountBank.getStatus().equals(MONETARY_ACCOUNT_STATUS_ACTIVE))


This is not java 7 compatible, please refactor to make it java 7 compatible.

that is then also applicable for the use of Optional (UserContextHelper#getFirstUser)

OGKevin · 2018-06-18T07:28:48Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+    return response.getValue().stream()
+            .filter(monetaryAccountBank->monetaryAccountBank.getStatus().equals(MONETARY_ACCOUNT_STATUS_ACTIVE))
+            .findFirst()
+            .orElse(null);


Should throw exception instead of returning null.

As the mehtod name does not indicate that it will return null if not found which could cause unwanted null pointer exception.

So please throw the no active MA found exception here.

OGKevin · 2018-06-18T07:29:59Z

src/test/java/com/bunq/sdk/context/UserContextTest.java

+public class UserContextTest extends BunqSdkTestBase {
+
+    @Test
+    public void testConstruct() {


OGKevin · 2018-06-18T07:30:04Z

src/test/java/com/bunq/sdk/context/UserContextTest.java

+    public void testConstruct() {
+        ApiContext context = getApiContext();
+
+        UserContext sut = new UserContext(context);


less complexity, risk for nullpointers is the same as before

OGKevin · 2018-06-20T07:19:24Z

@tubbynl be aware i did a small push to your branch be sure to pull, ill do a review cycle now.

OGKevin

I have one more, after this LGTM.

OGKevin · 2018-06-20T07:20:43Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+      }
+    }
+
+    throw new BunqException(ERROR_NO_ACTIVE_MONETARY);


ERROR_NO_ACTIVE_MONETARY --> ERROR_NO_ACTIVE_MONETARY_ACCOUNT_BANK

OGKevin · 2018-06-20T07:26:45Z

The 2 failing test are alright. rate limit and a 500 which has nothing to do with the SDK it self. The 429 in the tests should be fixed with the throttle.

OGKevin · 2018-06-20T07:48:51Z

@patrickdw1991 please 👀

patrickdw1991 · 2018-06-20T08:11:56Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+    );
+    String wrapper = MonetaryAccountBank.class.getSimpleName();
+    BunqResponse<List<MonetaryAccountBank>> response = fromJsonList(
+        MonetaryAccountBank.class, responseRaw, wrapper


Please add a new line after the ,s for readability :)

patrickdw1991 · 2018-06-20T08:15:10Z

src/main/java/com/bunq/sdk/model/core/UserContextHelper.java

+import java.util.List;
+
+public class UserContextHelper extends BunqModel {
+  /**


Missing new line

tubbynl · 2018-06-20T19:23:06Z

see #100 close as-you-wish

OGKevin · 2018-06-22T06:48:47Z

@patrickdw1991 👁

OGKevin · 2018-06-22T07:29:15Z

@andrederoos

bunq deleted a comment Jun 14, 2018

OGKevin self-requested a review June 17, 2018 06:35

build UserContext non-static (bunq#93)

714d486

UserContext building without static references, backwards compatible

tubbynl force-pushed the develop-non-breaking-refactor branch from 2895b15 to 714d486 Compare June 17, 2018 15:01

bunq deleted a comment Jun 17, 2018

OGKevin suggested changes Jun 17, 2018

View reviewed changes

processed review feedback (bunq#93)

20c7811

bunq deleted a comment Jun 17, 2018

fix tests on missing arg (bunq#93)

aa9aae0

bunq deleted a comment Jun 18, 2018

OGKevin suggested changes Jun 18, 2018

View reviewed changes

feedback, removed null-checks (bunq#93)

20304a2

less complexity, risk for nullpointers is the same as before

bunq deleted a comment Jun 18, 2018

feedback, error handling, spacing (bunq#93)

3f93d18

bunq deleted a comment Jun 18, 2018

bunq deleted a comment Jun 20, 2018

OGKevin added 2 commits June 20, 2018 09:14

Grouped header constants accordingly. (bunq#93)

0d7cae4

Some minor formatting changes. (bunq#93)

61dee09

bunq deleted a comment Jun 20, 2018

OGKevin suggested changes Jun 20, 2018

View reviewed changes

feedback error constant name (bunq#93)

be155d7

bunq deleted a comment Jun 20, 2018

OGKevin previously approved these changes Jun 20, 2018

View reviewed changes

OGKevin requested a review from patrickdw1991 June 20, 2018 07:48

OGKevin added the Can be merged label Jun 20, 2018

OGKevin added this to the 1.0.0 milestone Jun 20, 2018

patrickdw1991 requested changes Jun 20, 2018

View reviewed changes

Minor formatting refactor. (bunq#93)

c416a86

OGKevin dismissed their stale review via c416a86 June 22, 2018 06:48

OGKevin changed the title ~~#93 UserContext building without static references, backwards compatible~~ UserContext building without static references. (bunq/sdk_java#93) Jun 22, 2018

bunq deleted a comment Jun 22, 2018

patrickdw1991 approved these changes Jun 22, 2018

View reviewed changes

OGKevin merged commit aa4736b into bunq:develop Jun 22, 2018

		BunqResponseRaw responseRaw = getRawResponse(USER_ENDPOINT_URL_LISTING);
		BunqResponse<List<User>> response = fromJsonList(User.class, responseRaw);

UserContext building without static references. (bunq/sdk_java#93) #99

UserContext building without static references. (bunq/sdk_java#93) #99

Conversation

tubbynl commented Jun 14, 2018 • edited by OGKevin Loading

tubbynl commented Jun 14, 2018

tubbynl commented Jun 15, 2018

OGKevin commented Jun 17, 2018

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

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

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

OGKevin commented Jun 20, 2018

OGKevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin commented Jun 20, 2018

OGKevin commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tubbynl commented Jun 20, 2018 • edited Loading

OGKevin commented Jun 22, 2018

OGKevin commented Jun 22, 2018

tubbynl commented Jun 14, 2018 •

edited by OGKevin

Loading

tubbynl commented Jun 20, 2018 •

edited

Loading