-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UserContext building without static references. (bunq/sdk_java#93) #99
Changes from 1 commit
714d486
20c7811
aa9aae0
20304a2
3f93d18
d77e23d
0bc1987
6768192
8491b9e
cdc15e7
0d7cae4
61dee09
be155d7
c416a86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,5 +101,4 @@ Date getExpiryTime() { | |
public Integer getUserId() { | ||
return userId; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
import com.bunq.sdk.exception.BunqException; | ||
import com.bunq.sdk.model.core.BunqModel; | ||
import com.bunq.sdk.model.core.UserContextHelper; | ||
import com.bunq.sdk.model.generated.endpoint.MonetaryAccountBank; | ||
import com.bunq.sdk.model.generated.endpoint.User; | ||
import com.bunq.sdk.model.generated.endpoint.UserCompany; | ||
|
@@ -18,54 +19,41 @@ public class UserContext { | |
private static final String ERROR_NO_ACTIVE_MONETARY_ACCOUNT_FOUND = "No active monetary account found."; | ||
private static final String ERROR_PRIMARY_MONETARY_ACCOUNT_IS_NOT_SET = "Primary monetaryAccount is not set."; | ||
|
||
private static final String MONETARY_ACCOUNT_STATUS_ACTIVE = "ACTIVE"; | ||
private static final int INDEX_FIRST = 0; | ||
|
||
private final ApiContext apiContext; | ||
private UserCompany userCompany; | ||
private UserPerson userPerson; | ||
private MonetaryAccountBank primaryMonetaryAccountBank; | ||
private Integer userId; | ||
|
||
public UserContext(Integer userId) { | ||
this.setUser(this.getUserObject()); | ||
this.userId = userId; | ||
} | ||
|
||
private BunqModel getUserObject() { | ||
return User.list().getValue().get(INDEX_FIRST).getReferencedObject(); | ||
public UserContext(ApiContext apiContext) { | ||
this.apiContext = apiContext; | ||
refreshContext(); | ||
} | ||
|
||
private void setUser(BunqModel user) { | ||
if (user instanceof UserPerson) { | ||
this.userPerson = (UserPerson) user; | ||
} else if (user instanceof UserCompany) { | ||
this.userCompany = (UserCompany) user; | ||
private void initUser(User user) { | ||
if (user!=null && user.getUserPerson()!=null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are you not using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This increases the complexity of the if statement without any benefits. So please revert back to using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw; your code review thingie should mark the |
||
this.userPerson = user.getUserPerson(); | ||
} else if (user!=null && user.getUserCompany()!=null) { | ||
this.userCompany = user.getUserCompany(); | ||
} else { | ||
throw new BunqException(ERROR_UNEXPECTED_USER_INSTANCE); | ||
} | ||
} | ||
|
||
public void initMainMonetaryAccount() { | ||
List<MonetaryAccountBank> allMonetaryAccount = MonetaryAccountBank.list().getValue(); | ||
|
||
for (MonetaryAccountBank monetaryAccountBank : allMonetaryAccount) { | ||
if (monetaryAccountBank.getStatus().equals(MONETARY_ACCOUNT_STATUS_ACTIVE)) { | ||
this.primaryMonetaryAccountBank = monetaryAccountBank; | ||
|
||
return; | ||
} | ||
private void initMainMonetaryAccount(MonetaryAccountBank monetaryAccountBank) { | ||
if(monetaryAccountBank==null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing space around operators |
||
throw new BunqException(ERROR_NO_ACTIVE_MONETARY_ACCOUNT_FOUND); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not make sense to throw this error here. It would made sense if this error says There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not make sense to throw an error here saying |
||
} | ||
|
||
throw new BunqException(ERROR_NO_ACTIVE_MONETARY_ACCOUNT_FOUND); | ||
this.primaryMonetaryAccountBank = monetaryAccountBank; | ||
} | ||
|
||
public void refreshContext() { | ||
this.setUser(this.getUserObject()); | ||
this.initMainMonetaryAccount(); | ||
UserContextHelper helper = new UserContextHelper(this.apiContext); | ||
this.initUser(helper.getFirstUser()); | ||
this.initMainMonetaryAccount(helper.getFirstActiveMonetaryAccountBank(getUserId())); | ||
} | ||
|
||
public Integer getUserId() { | ||
return this.userId; | ||
return this.apiContext.getSessionContext().getUserId(); | ||
} | ||
|
||
public boolean isOnlyUserPersonSet() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package com.bunq.sdk.model.core; | ||
|
||
import com.bunq.sdk.context.ApiContext; | ||
import com.bunq.sdk.http.ApiClient; | ||
import com.bunq.sdk.http.BunqResponse; | ||
import com.bunq.sdk.http.BunqResponseRaw; | ||
import com.bunq.sdk.model.generated.endpoint.MonetaryAccountBank; | ||
import com.bunq.sdk.model.generated.endpoint.User; | ||
|
||
import java.util.List; | ||
|
||
public class UserContextHelper extends BunqModel { | ||
private static final String MONETARY_ACCOUNT_STATUS_ACTIVE = "ACTIVE"; | ||
private static final int INDEX_FIRST = 0; | ||
|
||
private final ApiClient apiClient; | ||
|
||
public UserContextHelper(ApiContext apiContext) { | ||
this.apiClient = new ApiClient(apiContext); | ||
} | ||
|
||
private BunqResponseRaw getRawResponse(String url) { | ||
return this.apiClient.get(url, null, null); | ||
} | ||
|
||
public User getFirstUser() { | ||
BunqResponseRaw responseRaw = getRawResponse("user"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please extract magic value in constant. |
||
BunqResponse<List<User>> response = fromJsonList(User.class, responseRaw); | ||
return response.getValue().get(INDEX_FIRST); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing new line before return statement. |
||
} | ||
|
||
public MonetaryAccountBank getFirstActiveMonetaryAccountBank(Integer userId) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
BunqResponseRaw responseRaw = getRawResponse(String.format("user/%s/monetary-account-bank", userId)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please extract magic value into constant. |
||
BunqResponse<List<MonetaryAccountBank>> response = fromJsonList(MonetaryAccountBank.class, responseRaw, MonetaryAccountBank.class.getSimpleName()); | ||
for (MonetaryAccountBank monetaryAccountBank : response.getValue()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing new line before loop. |
||
if (monetaryAccountBank.getStatus().equals(MONETARY_ACCOUNT_STATUS_ACTIVE)) { | ||
return monetaryAccountBank; | ||
} | ||
} | ||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing new line before return statement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dont return null here, throw the exception here. |
||
} | ||
|
||
@Override | ||
public boolean isAllFieldNull() { | ||
return true; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing new line after |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
package com.bunq.sdk.context; | ||
|
||
import com.bunq.sdk.BunqSdkTestBase; | ||
import org.junit.Test; | ||
|
||
import static org.junit.Assert.*; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now wild imports please. |
||
|
||
public class UserContextTest extends BunqSdkTestBase { | ||
|
||
@Test | ||
public void testConstruct() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test construct of what ? Please rename method to reflect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bump |
||
ApiContext context = getApiContext(); | ||
|
||
UserContext sut = new UserContext(context); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bump There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sut = system-under-test, mostly to specify the thing you are testing but will rename |
||
|
||
assertNotNull(sut.getUserId()); | ||
assertNotNull(sut.getMainMonetaryAccountId()); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing new line at EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert the removal of this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert the removal of this line. As described here: https://google.github.io/styleguide/javaguide.html#s4.6.1-vertical-whitespace