4
\$\begingroup\$

I have been slowly getting more and more "at home" with Android development and I would like some guidance. I have made an app that uses a ListActivity, that attaches to an SQLite DB. I'm using the onListItemClick function to fire up a "single record view" where I can delete and edit the record. There is also a way to edit the record from the list view by using the Context Menu and selecting so there are potentially 2 ways to edit a record.

  • Directly from a ListView via Context Menu
  • Selecting Edit from a single record view

Both the single record view and the edit view are being handled by the same Activity (not sure if this is common practice or not). Now having been a programmer for some time something tells me that I am not doing this in the best way that I could so I thought I would submit it for code review. So here is the "overly-complicated feeling" Activity which I have commented to try and help understand my logic. I did think that this might be something I should be using a FrameLayout for but don't really know if that is the best way to do things. I am still new to Android dev so have a lot to learn.

public class CardActivity extends Activity implements OnClickListener {

    private Card card;
    private EditText questionEdit;
    private EditText answerEdit;
    private CardSQLiteHelper dbHelper;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);

        dbHelper = new CardSQLiteHelper(this);
        dbHelper.open();

        Bundle b = getIntent().getExtras();

        if(b == null) {                                     // Coming in from Add there is no card in the Intent
            card = new Card();
            setupEdit();
        } else if( b.containsKey(Intent.ACTION_VIEW)) {     // Coming in from View the card is stored using the ACTION_VIEW key
            card = (Card) b.get(Intent.ACTION_VIEW);
            setupView();
        } else {                                            // Coming from Edit the card is stored using the ACTION_EDIT key
            card = (Card) b.get(Intent.ACTION_EDIT);
            setupEdit();
        }

    }

    /**
     * Sets up the view for editing using the right layout and registering the buttons
     */
    private void setupEdit() {
        setContentView(R.layout.card_edit);

        questionEdit = (EditText) findViewById(R.id.questionEditText);
        answerEdit = (EditText) findViewById(R.id.answerEditText);

        questionEdit.setText(card.getQuestion());
        answerEdit.setText(card.getAnswer());

        Button saveButton = (Button) findViewById(R.id.cardSaveButton);
        Button cancelButton = (Button) findViewById(R.id.cardCancelButton);

        saveButton.setOnClickListener(this);
        cancelButton.setOnClickListener(this);
    }

    /**
     * Sets up the view for "Single record View" mode displaying the data and offering a Delete and Edit button
     */
    private void setupView() {
        setContentView(R.layout.card_view);

        TextView questionText = (TextView) findViewById(R.id.questionTextView);
        TextView answerText = (TextView) findViewById(R.id.answerTextView);

        questionText.setText(card.getQuestion());
        answerText.setText(card.getAnswer());

        Button deleteButton = (Button) findViewById(R.id.cardDeleteButton);
        Button editButton = (Button) findViewById(R.id.cardEditButton);

        deleteButton.setOnClickListener(this);
        editButton.setOnClickListener(this);
    }

    @Override
    public void onClick(View v) {

        switch (v.getId()) {
        case R.id.cardEditButton:       
            // Clicked the Edit button in the "Single record view" so we switch to an Edit view
            setupEdit();
            break;
        case R.id.cardDeleteButton:
            // Delete is easy, just nuke the card and return an OK result
            dbHelper.deleteCard(card);
            setResult(RESULT_OK);
            finish();
            break;
        case R.id.cardCancelButton:
            // Cancel again is easy, do nothing and return the CANCELLED result
            setResult(RESULT_CANCELED);
            finish();
            break;
        case R.id.cardSaveButton:
            // Save is not hard just a bit lengthy because of the things we need to do
            // Check there is content in the Question EditText
            if(questionEdit.getText().toString().length() == 0) {
                Toast.makeText(this, "Your question must have some text", Toast.LENGTH_LONG).show();
                break;
            }

            // Check there is content in the Answer EditText
            if(answerEdit.getText().toString().length() == 0) {
                Toast.makeText(this, "Your answer must have some text", Toast.LENGTH_LONG).show();
                break;
            }

            // Put the text into my object
            card.setQuestion(questionEdit.getText().toString());
            card.setAnswer(answerEdit.getText().toString());

            // If the card has no ID then we are adding a new card
            if(card.getId() == 0) {
                dbHelper.addCard(card);
            } else {
                dbHelper.updateCard(card);
            }
            setResult(RESULT_OK);
            finish();
            break;
        }
    }

}

As requested this is a bit more of an explanation of what the app does. It's a basic app that I am using as a learning process, of a revision or learning card system. The end user can add & remove cards from a list which they can store a Question and Answer on as if you were using them to revise for an exam, eventually I will give the user a way to randomise the cards and answer the relevant question as well as provide more ways to store data and get between the cards.

\$\endgroup\$
  • \$\begingroup\$ Do you think you could provide a little more context for what the app does? I think I understand most of what's going on, but it helps to have an explanation behind it. \$\endgroup\$ – boztalay Jan 11 '14 at 5:24
  • \$\begingroup\$ Certainly I will edit the main text \$\endgroup\$ – Catharsis Jan 11 '14 at 7:10
1
\$\begingroup\$

One of the tenets fo SOLID programming (the first one, in fact) is that classes should have a single responsibility.

Here you have a class which has two distinct functions: view a card ; edit a card.

The code complexity you have introduced just to differentiate between the uses of your class is more than the complexity of what you would have if you had two classes.

Your instinct is right, you are not doing this the right way.

In this particular case, I would recommend copy/pasting the class, and making each version a specialization for either View or Edit. The setupEdit and setupView should instead be incorporated in to the onCreate() method. The onClick() logic can be split quite simply as well.

What is nice about this is that the code becomes much more reusable... you can have the two specialized classes, and adding a new way to access the Activity (swiping '>next', etc.) would be much easier to do.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.