Skip to content

Commit

Permalink
1.24 Prevent crash with blank editor
Browse files Browse the repository at this point in the history
  • Loading branch information
Beginning Android committed Aug 23, 2016
1 parent 20d0524 commit 6a82f79
Showing 1 changed file with 16 additions and 1 deletion.
17 changes: 16 additions & 1 deletion app/src/main/java/com/example/android/pets/EditorActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,29 @@ private void savePet() {
String nameString = mNameEditText.getText().toString().trim();
String breedString = mBreedEditText.getText().toString().trim();
String weightString = mWeightEditText.getText().toString().trim();
int weight = Integer.parseInt(weightString);

// Check if this is supposed to be a new pet
// and check if all the fields in the editor are blank
if (mCurrentPetUri == null &&

This comment has been minimized.

Copy link
@SzasznikaJanos

SzasznikaJanos Apr 7, 2018

Why do we check if the mCurrentPetUri is null? Should't we check if we have input for everything regardless if we are in the save a pet or update a pet mode? Because user can accidentally delete an input in update pet mode and press save pet.

This comment has been minimized.

Copy link
@seedfac

seedfac May 23, 2018

This is the first time I came into solution code to see why this (uri == null) check is here.

This comment has been minimized.

Copy link
@TheCount511

TheCount511 Sep 6, 2018

Why do we check if the mCurrentPetUri is null? it doesn't seem to make any difference to the intended functionality

This comment has been minimized.

Copy link
@Ivoiran2014

Ivoiran2014 Aug 15, 2020

We shouldn't check for mCurrentPetUri being null , as if it's not null and we try to modify an existing pet , we would get another crash for example if we delete all the property and saved it.

TextUtils.isEmpty(nameString) && TextUtils.isEmpty(breedString) &&

This comment has been minimized.

Copy link
@kwhitejr

kwhitejr Nov 15, 2017

Is there a meaningful difference between TextUtils.isEmpty(nameString) and nameString.isEmpty()?

Additionally, I used or operators here because if any field is empty, the save should be rejected:
if (weightString.isEmpty() || nameString.isEmpty() || breedString.isEmpty()) { Toast.makeText(this, "Cannot save without name, breed, and weight.", Toast.LENGTH_SHORT).show(); return; }

This comment has been minimized.

Copy link
@emanuele78

This comment has been minimized.

Copy link
@TheCount511

TheCount511 Sep 6, 2018

why do we use && operator for comparison and not || ? since just like @kwhitejr said; we want to reject the app from saving the data if the any input field is empty?

TextUtils.isEmpty(weightString) && mGender == PetEntry.GENDER_UNKNOWN) {

This comment has been minimized.

Copy link
@TheCount511

TheCount511 Sep 6, 2018

why is the weight also been checked since we have already assigned a default value to it?
if its going to be validated here why dont we just remove the default value ?

// Since no fields were modified, we can return early without creating a new pet.

This comment has been minimized.

Copy link
@danieldogeanu

danieldogeanu Jun 2, 2018

This whole if statement is wrong and useless. It doesn't prevent the user to update the pet with empty values. The name was supposed to be NOT NULL.

This comment has been minimized.

Copy link
@AlaaElden98

AlaaElden98 Aug 10, 2020

Use || instead of && and it will work, this way you check for each value

if (mCurrentPetUri == null || TextUtils.isEmpty(nameString) || TextUtils.isEmpty(breedString)
                || TextUtils.isEmpty(weightString) || mGender == PetEntry.GENDER_UNKNOWN) {
            return;
        }

This comment has been minimized.

Copy link
@Ivoiran2014

Ivoiran2014 Aug 15, 2020

The whole statement is incorrect and some value are unnecessary.
first we shouldn't check for uri being null as we'll be free to input blank value in the edit pet.
Second we shouldn't also include UNKNOWN as a check as people can still insert it as a valid option and i'll be rejected.
And finally We don't have to check for the weight as it get a default value of 0 below
So the final code for checking should look like this.

if (TextUtils.isEmpty(nameString) || TextUtils.isEmpty(breedString)){ return; }

// No need to create ContentValues and no need to do any ContentProvider operations.
return;
}

// Create a ContentValues object where column names are the keys,
// and pet attributes from the editor are the values.
ContentValues values = new ContentValues();
values.put(PetEntry.COLUMN_PET_NAME, nameString);
values.put(PetEntry.COLUMN_PET_BREED, breedString);
values.put(PetEntry.COLUMN_PET_GENDER, mGender);
// If the weight is not provided by the user, don't try to parse the string into an
// integer value. Use 0 by default.
int weight = 0;

This comment has been minimized.

Copy link
@bhavikvashi1804

bhavikvashi1804 Nov 28, 2018

weight is already defined
so make int weight1

if (!TextUtils.isEmpty(weightString)) {
weight = Integer.parseInt(weightString);
}
values.put(PetEntry.COLUMN_PET_WEIGHT, weight);

This comment has been minimized.

Copy link
@ihalsh

ihalsh Dec 20, 2017

    // If the weight is not provided by the user, don't try to parse the string into an
    // integer value. Use 0 by default.
    values.put(PetEntry.COLUMN_PET_WEIGHT, TextUtils.isEmpty(weightString) ? 0 : Integer.parseInt(weightString));
// Determine if this is a new or existing pet by checking if mCurrentPetUri is null or not
Expand Down

25 comments on commit 6a82f79

@mgillieron
Copy link

Choose a reason for hiding this comment

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

For me the code for checking is a bit strange.
What is happening if the user add a pet leaving only the blank name ?
This pet is saved in the database whereas it has been specified that the "Name" column cannot be NULL.

What do you think about that ?
Thanks.

@oleohi
Copy link

@oleohi oleohi commented on 6a82f79 Apr 5, 2017

Choose a reason for hiding this comment

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

Good question. I was thinking about that too. In that case, we might need to check for name specifically to ensure the user inputs something.

@mgillieron
Copy link

Choose a reason for hiding this comment

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

Yes I think so, thanks oleohi !

@my-jabin
Copy link

Choose a reason for hiding this comment

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

Should we check the name and breed only in the EditorActivity, when we call the savePet() method?
If there are 10 EditText, how could we check them all?

@oleohi
Copy link

@oleohi oleohi commented on 6a82f79 Apr 20, 2017

Choose a reason for hiding this comment

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

Yes. Since they are the compulsory or mandatory fields, I think you would have to check for all of them. You can use an if-else statement with logical operators.

@PackHg
Copy link

@PackHg PackHg commented on 6a82f79 May 3, 2017

Choose a reason for hiding this comment

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

An empty String is not NULL, isn't it?

@mgillieron
Copy link

Choose a reason for hiding this comment

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

Absolutely ! NULL and empty are 2 different things. Moreover, the "TextUtils" class has a method called "isEmpty" verifying if a string is null OR empty (here empty means that the string has a length of 0)

@andresual
Copy link

Choose a reason for hiding this comment

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

I was skipped the sanity check video. Isn't work yet

@namclu
Copy link

@namclu namclu commented on 6a82f79 Jun 2, 2017

Choose a reason for hiding this comment

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

I feel as if the code needs a nested if-else statement to check for the possibility that each field could be left blank, though only the name field really matters

@linucksrox
Copy link

Choose a reason for hiding this comment

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

@namclu You're right! I'm not sure which part of the lesson you're on, but they go over "Sanity Checking" in section 3.17. Eventually the sanity checking happens in the ContentProvider which acts like a "gatekeeper" between the data and the application accessing it. It's better to do checking there because in the case where multiple different activities might be inputting data, you can avoid having to write the error checking code in all of those places.

@namclu
Copy link

@namclu namclu commented on 6a82f79 Jun 3, 2017

Choose a reason for hiding this comment

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

@linucksrox yes I thought this would be best handled in a centralized location, specifically as you mentioned in the PetProvider. Right now, the code seems a bit ad-hoc in that we do data checks at the points when we need to, but maybe in order to have PetProvider (or another class) manage all that would make the code more unwieldy

@ankurg22
Copy link

@ankurg22 ankurg22 commented on 6a82f79 Aug 12, 2017

Choose a reason for hiding this comment

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

Here is my solution.

String name = mNameEditText.getText().toString().trim();
        String breed = mBreedEditText.getText().toString().trim();
        String weightString = mWeightEditText.getText().toString().trim();
        int weight;
        try {
            weight = Integer.parseInt(weightString);
        } catch (NumberFormatException e) {
            weight = 0;
        }
        int gender = mGender;
        if (currentUri == null && TextUtils.isEmpty(name) || TextUtils.isEmpty(breed) || gender == PetEntry.GENDER_UNKNOWN) {
            return;
        }

@Daggertho
Copy link

Choose a reason for hiding this comment

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

I still don't know why do we implement "Sanity check" yet even after putting it in our functions after following every lesson. I know it is used for making that "gate" but it didn't show up any messages except, well in my case, my app crashed and was sent to the activity_catalog.xml if I didn't put any name or breed or weight... I thought it was supposed to show some messages.... if I used the logic as @ankurg22 made then what was the use of that sanity check?

@romeyng
Copy link

Choose a reason for hiding this comment

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

My app crashes whenever i use the fab to enter the editor activity. Clicking an item from the existing list lets me edit successfully. It turns out that mCurrentPetUri is causing the error because it is null in the return statement of onCreate loader. Any help greatly appreciated. Thank you!

public Loader onCreateLoader(int i, Bundle bundle) {
Log.v("Editor", "loader manager called");
String[] projection = {PetEntry._ID,
PetEntry.COLUMN_PET_NAME,
PetEntry.COLUMN_PET_BREED,
PetEntry.COLUMN_PET_GENDER,
PetEntry.COLUMN_PET_WEIGHT};
Log.v("Editor", "mcurrentPetUri: "+mCurrentPetUri);

    return new CursorLoader(this, mCurrentPetUri,projection,null,null,null);
}

@n-abdelmaksoud
Copy link

Choose a reason for hiding this comment

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

we have only two values not to be null in the CREATE TABLE statement: name and gender.
So we should save pet regardless breed is null or empty String and i think we should show Toast msg if the name field is empty or null.
and if name String is not empty, we should save the pet with setting default values for gender and weight.
That's my code, it works fine

 int defaultWeight=0;
        String name = mNameEditText.getText().toString().trim();
        String breed = mBreedEditText.getText().toString().trim();
        String weight = mWeightEditText.getText().toString().trim();

        if (TextUtils.isEmpty(name)) {
            Toast.makeText(this, getString(R.string.insert_pet_name_toast), Toast.LENGTH_LONG).show();
        } else {
            ContentValues values = new ContentValues();
            values.put(PetsContract.NAME, name);
            values.put(PetsContract.BREED, breed);
            values.put(PetsContract.GENDER, mGender);
            if(TextUtils.isEmpty(weight)){
                values.put(PetsContract.WEIGHT,defaultWeight);
            } else {
                values.put(PetsContract.WEIGHT, Integer.parseInt(weight));
            } 

mGender is declared as a global variable and initialized:
private int mGender = 0;
it also takes a new value from the spinner selection

@pooriant
Copy link

Choose a reason for hiding this comment

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

i think we don't need to check all parameters before saving becuse at DbHelper class we set NOTNULL for 3 parameter Name,Weight,Gender
also we set a deafult value for Weight =0 there !!!

so we just need to check 2 or 1 parameter (Name,Gender) ,
if we don't want to save pet with Gender=Unknown we must use (OR then AND) because if either of Name or Gender will be empty we must return and don't save anything.
but if gender value doesn't matter ,we just need to check Name value doesn't have null
and if name has a value we can save pet to database

this is my solution

if(uri==null&&TextUtils.isEmpty(mNameEditText.getText())||mGender==PetEntry.GENDER_UNKNOWN){Toast.makeText(this, "Nothing Saved", Toast.LENGTH_SHORT).show(); return;}

@nick-running
Copy link

Choose a reason for hiding this comment

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

I use this code to parse weight int weight = Integer.parseInt(TextUtils.isEmpty(weightString)?"0":weightString);

@parasarorahere
Copy link

Choose a reason for hiding this comment

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

why we can't do in petProvider
screenshot 1001

@isthiswrite
Copy link

Choose a reason for hiding this comment

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

I don't agree with TextUtils.isEmpty for name, weight, breed, and gender. I only used it for name. The reason is if a user selects the dropdown spinner "male" for example, it will save a blank pet in the catalog activity leaving empty field. This is how I fixed that issue.

`private void savePet() {

    String nameString = mNameEditText.getText().toString().trim();
    String breedString = mBreedEditText.getText().toString().trim();
    String weightString = mWeightEditText.getText().toString().trim();

    // If the mCurrentPetUri null then create a new pet
   // No need to worry about Gender, as it will say Unknown by default, and if a name is not input
  // That would be the only scenario that would give error, pet not saved. So, if someone just chooses               //spinner or just adds weight, it won't matter because of if statements for breed and weight below

    if (mCurrentPetUri == null &&
            TextUtils.isEmpty(nameString)) {
        Toast.makeText(this, getString(R.string.pet_not_saved), Toast.LENGTH_SHORT).show();
        return;
    }

    // Create ContentValues object
    ContentValues values = new ContentValues();
    values.put(PetEntry.COLUMN_PET_NAME, nameString);
    values.put(PetEntry.COLUMN_PET_GENDER, mGender);

    // Breed String can be null, so when it is, make it say "Unknown Breed" or something
   
     
   String breed = "Unknown Breed";
    if (!TextUtils.isEmpty(breedString)){
      breed = breedString;
    }
    values.put(PetEntry.COLUMN_PET_BREED, breed);

  // Same thing with weight as breed 
   int weight = 0;
    if (!TextUtils.isEmpty(weightString)) {
        weight = Integer.parseInt(weightString);
    }
    values.put(PetEntry.COLUMN_PET_WEIGHT, weight);

`

@Babadzhanov
Copy link

Choose a reason for hiding this comment

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

This is not great way of handling errors..

@Hendhussein
Copy link

@Hendhussein Hendhussein commented on 6a82f79 Aug 15, 2018

Choose a reason for hiding this comment

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

I am still having cruch when I insert new pet with name only or any item (not completed data) it dosnt show the toast message and close this is the code I dont know where is the problem

`private void savePet() {
// Read from input fields
// Use trim to eliminate leading or trailing white space
String nameString = mNameEditText.getText().toString().trim();
String breedString = mBreedEditText.getText().toString().trim();
String weightString = mWeightEditText.getText().toString().trim();

    // Check if this is supposed to be a new pet
    // and check if all the fields in the editor are blank
    if (mCurrentPetUri == null &&
            TextUtils.isEmpty(nameString) && TextUtils.isEmpty(breedString) &&
            // Since no fields were modified, we can return early without creating a new pet.
            // No need to create ContentValues and no need to do any ContentProvider operations.
            TextUtils.isEmpty(weightString) && mGender == PetEntry.GENDER_UNKNOWN) {return;}

    // Create a ContentValues object where column names are the keys,
    // and pet attributes from the editor are the values.
    ContentValues values = new ContentValues();
    values.put(PetEntry.COLUMN_PET_NAME, nameString);
    values.put(PetEntry.COLUMN_PET__BREED, breedString);
    values.put(PetEntry.COLUMN_PET__GENDER, mGender);
    // If the weight is not provided by the user, don't try to parse the string into an
    // integer value. Use 0 by default.
    int weight = 0;
    if (!TextUtils.isEmpty(weightString)) {
        weight = Integer.parseInt(weightString);
    }
    values.put(PetEntry.COLUMN_PET__WEIGHT, weight);

            // Determine if this is a new or existing pet by checking if mCurrentPetUri is null or not
    if (mCurrentPetUri == null) {
        // This is a NEW pet, so insert a new pet into the provider,
        // returning the content URI for the new pet.
        Uri newUri = getContentResolver().insert(PetEntry.CONTENT_URI, values);
        // Show a toast message depending on whether or not the insertion was successful.
        if (newUri == null) {
            // If the new content URI is null, then there was an error with insertion.
            Toast.makeText(this, getString(R.string.editor_insert_pet_failed),
                    Toast.LENGTH_SHORT).show();
        } else {
            // Otherwise, the insertion was successful and we can display a toast.
            Toast.makeText(this, getString(R.string.editor_insert_pet_successful),
                    Toast.LENGTH_SHORT).show();
        }
    }
    // Otherwise this is an EXISTING pet, so update the pet with content URI: mCurrentPetUri
    // and pass in the new ContentValues. Pass in null for the selection and selection args
    // because mCurrentPetUri will already identify the correct row in the database that
    // we want to modify.
    int rowsAffected = getContentResolver().update(mCurrentPetUri, values, null, null);
    // Show a toast message depending on whether or not the update was successful.
    if (rowsAffected == 0) {
        // If no rows were affected, then there was an error with the update.
        Toast.makeText(this, getString(R.string.editor_update_pet_failed),
                Toast.LENGTH_SHORT).show();
    } else {
        // Otherwise, the update was successful and we can display a toast.
        Toast.makeText(this, getString(R.string.editor_update_pet_successful),
                Toast.LENGTH_SHORT).show();
    }

}

@aydinaytac
Copy link

Choose a reason for hiding this comment

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

In this case user can't add a pet without name, but after addind a pet, in edit mode user can save without name! This is useless...

@xht418
Copy link

@xht418 xht418 commented on 6a82f79 Nov 14, 2018

Choose a reason for hiding this comment

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

I. Method "value.put(columnName, value)" won't let "NULL" situation happen to the SQLite database, no matter you set "NOT NULL" or not for the column in your sql create statement of your table in the dbHelper class.

II. EditText has its own default value for each "inputType", for example "0" for the "number" type, empty String "" for the "text" or other text type. Besides that, "getText()" will never return null value, it will return its default value for specific "inputType", which means empty String "" and number "0" will be "put" into the table if you don't enter anything. At this point, think about I.

III. Based on the above, we better handle user input in the right inside the "action_save" action menu button.

Please point out if anything is wrong.

@Ivoiran2014
Copy link

Choose a reason for hiding this comment

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

The whole checking statement is incorrect and some value are unnecessary.
first we shouldn't check for uri being null ,as if we did ,we'll be free to input blank value into an existing pet.
Second we shouldn't also include UNKNOWN as a check as UNKNOW is a valid option and some could update MALE to UNKNOW.
And finally We don't have to check for the weight as it get a default value of 0 below
So the final code for checking should look like this.

if (TextUtils.isEmpty(nameString) || TextUtils.isEmpty(breedString)){ return; }
int petWeight = 0;
if (!TextUtils.isEmpty(stringWeight)) { petWeight = Integer.parseInt(textWeight); }

@JoaoCGualb
Copy link

Choose a reason for hiding this comment

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

Alterei o codigo do if para
if (mCurrentPetUri == null &&
TextUtils.isEmpty(breedString) &&
TextUtils.isEmpty(weightString) && mGender == PetEntry.GENDER_UNKNOWN || TextUtils.isEmpty(nameString))
e funcionou ele da erro e não salva quando o nome esta em branco

Please sign in to comment.