Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ ext.deps = [rxjava : 'io.reactivex:rxjava:1.3.0',
butterknifecompiler : 'com.jakewharton:butterknife-compiler:8.6.0',
junit : 'junit:junit:4.12',
truth : 'com.google.truth:truth:0.33',
robolectric : 'org.robolectric:robolectric:3.1.2',
robolectric : 'org.robolectric:robolectric:3.3.2',
mockitocore : 'org.mockito:mockito-core:2.8.47']

buildscript {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ private void createAccessors() {
private void createBooleanAccessor() {
accessors.put(Boolean.class, new Accessor<Boolean>() {
@Override public Boolean get(String key, Boolean defaultValue) {
return preferences.getBoolean(key, defaultValue);
return preferences.getBoolean(key, (defaultValue == null) ? false : defaultValue);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this use case, we are defining default behavior for the user in the case of passing null. We don't know if a user wants true or false, but we're forcing using one of these. The same rule applies to all of the changes below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we are not defining default behavior for the user, because if the user sends null in defValue, it would already return before in Prefser class (and it would return null).

Copy link
Owner

@pwittchen pwittchen Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would user pass the null? Why should the user expect false in the case of passing null, but not true? How could he or she guess such behavior just by looking at the library API without browsing source code or debugging?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user will not get true or false if he/she passing null, the user will get null. Look the other comment I mention you, we are returning defValue before that check.

if (!contains(key)) {
  return defaultValue;
}

Line 250 of Prefser.

}

@Override public void put(String key, Boolean value) {
Expand All @@ -60,7 +60,7 @@ private void createBooleanAccessor() {
private void createFloatAccessor() {
accessors.put(Float.class, new Accessor<Float>() {
@Override public Float get(String key, Float defaultValue) {
return preferences.getFloat(key, defaultValue);
return preferences.getFloat(key, (defaultValue == null) ? 0f : defaultValue);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above.

}

@Override public void put(String key, Float value) {
Expand All @@ -72,7 +72,7 @@ private void createFloatAccessor() {
private void createIntegerAccessor() {
accessors.put(Integer.class, new Accessor<Integer>() {
@Override public Integer get(String key, Integer defaultValue) {
return preferences.getInt(key, defaultValue);
return preferences.getInt(key, (defaultValue == null) ? 0 : defaultValue);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above.

}

@Override public void put(String key, Integer value) {
Expand All @@ -84,7 +84,7 @@ private void createIntegerAccessor() {
private void createLongAccessor() {
accessors.put(Long.class, new Accessor<Long>() {
@Override public Long get(String key, Long defaultValue) {
return preferences.getLong(key, defaultValue);
return preferences.getLong(key, (defaultValue == null) ? 0L : defaultValue);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above.

}

@Override public void put(String key, Long value) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,8 @@ public <T> T get(@NonNull String key, @NonNull Class<T> classOfT, T defaultValue
Preconditions.checkNotNull(key, KEY_IS_NULL);
Preconditions.checkNotNull(classOfT, CLASS_OF_T_IS_NULL);

if (!contains(key) && defaultValue == null) {
return null;
if (!contains(key)) {
return defaultValue;
}

return get(key, TypeToken.fromClass(classOfT), defaultValue);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1169,4 +1169,179 @@ public void testPutShouldThrowAnExceptionWhenKeyIsNullForRemove() {
// then
assertThat(value).isNull();
}

@Test public void testShouldReturnStringValueWithDefaultNull() {
// given
prefser.clear();
String key = GIVEN_KEY;
String givenValue = "someText";

// when
prefser.put(key, givenValue);
String readValue = prefser.get(key, String.class, null);

// then
assertThat(givenValue).isEqualTo(readValue);
}

// alteracoes comeca aqui
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that comment, after your review I can delete it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be deleted.


@Test public void testGetShouldReturnNullForCustomObject() {
// given
prefser.clear();
String keyWhichDoesNotExist = KEY_WHICH_DOES_NOT_EXIST;

// when
CustomClass value = prefser.get(keyWhichDoesNotExist, CustomClass.class, null);

// then
assertThat(value).isNull();
// CustomClass givenObject = new CustomClass(23, "someText");
// CustomClass defaultObject = new CustomClass(67, "defaultText");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented code should be removed

}

@Test public void testGetShouldReturnCustomObjectWithDefaultNull() {
// given
prefser.clear();
String key = GIVEN_KEY;
CustomClass givenObject = new CustomClass(12, "someText");

// when
prefser.put(key, givenObject);
CustomClass readObject = prefser.get(key, CustomClass.class, null);

// then
assertThat(givenObject).isEqualTo(readObject);
}

@Test public void testShouldReturnNullForDoubleType() {
// given
prefser.clear();
String keyWhichDoesNotExist = KEY_WHICH_DOES_NOT_EXIST;

// when
Double value = prefser.get(keyWhichDoesNotExist, Double.class, null);

// then
assertThat(value).isNull();
}

@Test public void testShouldReturnDoubleValueWithDefaultNull() {
// given
prefser.clear();
String key = GIVEN_KEY;
Double givenValue = 7.77;

// when
prefser.put(key, givenValue);
Double readValue = prefser.get(key, Double.class, null);

// then
assertThat(givenValue).isEqualTo(readValue);
}

@Test public void testShouldReturnNullForBooleanType() {
// given
prefser.clear();
String keyWhichDoesNotExist = KEY_WHICH_DOES_NOT_EXIST;

// when
Boolean value = prefser.get(keyWhichDoesNotExist, Boolean.class, null);

// then
assertThat(value).isNull();
}

@Test public void testShouldReturnBooleanValueWithDefaultNull() {
// given
prefser.clear();
String key = GIVEN_KEY;
Boolean givenValue = true;

// when
prefser.put(key, givenValue);
Boolean readValue = prefser.get(key, Boolean.class, null);

// then
assertThat(givenValue).isEqualTo(readValue);
}

@Test public void testShouldReturnNullForIntegerType() {
// given
prefser.clear();
String keyWhichDoesNotExist = KEY_WHICH_DOES_NOT_EXIST;

// when
Integer value = prefser.get(keyWhichDoesNotExist, Integer.class, null);

// then
assertThat(value).isNull();
}

@Test public void testShouldReturnIntegerValueWithDefaultNull() {
// given
prefser.clear();
String key = GIVEN_KEY;
Integer givenValue = 7;

// when
prefser.put(key, givenValue);
Integer readValue = prefser.get(key, Integer.class, null);

// then
assertThat(givenValue).isEqualTo(readValue);
}

@Test public void testShouldReturnNullForFloatType() {
// given
prefser.clear();
String keyWhichDoesNotExist = KEY_WHICH_DOES_NOT_EXIST;

// when
Float value = prefser.get(keyWhichDoesNotExist, Float.class, null);

// then
assertThat(value).isNull();
}

@Test public void testShouldReturnFloatValueWithDefaultNull() {
// given
prefser.clear();
String key = GIVEN_KEY;
Float givenValue = 7.45f;

// when
prefser.put(key, givenValue);
Float readValue = prefser.get(key, Float.class, null);

// then
assertThat(givenValue).isEqualTo(readValue);
}

@Test public void testShouldReturnNullForLongType() {
// given
prefser.clear();
String keyWhichDoesNotExist = KEY_WHICH_DOES_NOT_EXIST;

// when
Long value = prefser.get(keyWhichDoesNotExist, Long.class, null);

// then
assertThat(value).isNull();
}

@Test public void testShouldReturnLongValueWithDefaultNull() {
// given
prefser.clear();
String key = GIVEN_KEY;
Long givenValue = 7L;

// when
prefser.put(key, givenValue);
Long readValue = prefser.get(key, Long.class, null);

// then
assertThat(givenValue).isEqualTo(readValue);
}

}