PileProject / drive

The drive project
Other
5 stars 4 forks source link

Replace string keys in code with string keys in xml #30

Closed tiwanari closed 8 years ago

tiwanari commented 8 years ago

I removed keys (for string values) in codes and extracted them into keys.xml.

mandaiy commented 8 years ago

By the way how about that kind of method to launch a new activity: https://coderwall.com/p/nehwfw/android-open-activities-with-a-static-method

We cannot apply the method to our app because we have inherited classes, which cannot have static override methods. But I believe it is worth considering some way to transfer the responsibility of maintaining keys to the classes that use the keys.

tiwanari commented 8 years ago

Thank you for your suggestion about making a method launch an activity. I think it is reasonable enough to apply our apps, and to avoid overriding static methods, how about throwing the role of launching an activity to subclasses instead of using getIntentTo** like:

public abstract class Base {
    protected int foo = 0;
    findViewById(R.id.a_button).setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            launchAnActivity("something");
        }
    });
    protected abstract void launchAnActivity(String bar);
}

public class Child extends Base {
    protected void launchAnActivity(String bar) {
        ActivityToOpen.show(this, foo, bar);
    }
}
tiwanari commented 8 years ago

@myusak I think the changes based on the meaningful discussion should be done in other PR. What do you think?

mandaiy commented 8 years ago

I do not think your snippet solves the problem of scattering keys, my explanation must be not enough, vague, and not concise.

What I hate is that an activity has to know keys of an arg bundle, like in https://github.com/PileProject/drive/blob/develop/app/src/main/java/com/pileproject/drive/programming/visual/activity/ProgrammingActivityBase.java#L127-L129

In these lines ProgrammingActivity has to know the key "is_connected" (though this hard code has been removed by your commits). If the ExecutionActivity has no child classes, you can write like ExecutioinActivity.launch(...), in which the key "is_connected" would be used as the bundle key, like the above link. But because it has a child class, (maybe) you cannot.

Why this kind of situation could be problematic is well written in http://qiita.com/Nkzn/items/f6c4582a92862b3b6f45

Anyway, please close this pull-request.

tiwanari commented 8 years ago

I think my explanation was not enough. Please let me show more detail.

I believe my snippet will solve the situation because it can provide subclasses (in nxt/ev3/pile/other packages) of a class (in main package) a way to launch other classes in their packages, e.g.,NxtTitileActivity can launch NxtProgrammingActivity.

So, for example, after preparing a launch method (this should be static anyway) in NxtExecutionActiviity, NxtProgrammingActivity can call it in its overridden method like launchExecutionAcitivity (this may be protected void ~) instead of using getIntentToExecute, and, of course, in that launchExecutionAcitivity method, we just call NxtExecutionActivity.launch(...) to avoid specifying keys. In that snippet, I didn't specify any key.

Does it make sense?

tiwanari commented 8 years ago

Anyway, let's keep the discussion as an issue (I will refer this from an issue) and I will close this PR.