Hard-Coded bundle params + few extra strings removed #714 #719

MattiaPrimavera · 2017-09-11T00:58:55Z

Done for fragments package bundle parameters + some other hard-coded strings ;)

maniac103 · 2017-09-11T05:13:42Z

I'd prefer if the string constants had an EXTRA_ prefix to make their usage more obvious.
Also, the filter and sort strings should be moved to the Github API bindings instead.

maniac103 · 2017-09-11T05:14:13Z

app/src/main/java/com/gh4a/fragment/RepositoryListContainerFragment.java

@@ -26,9 +26,32 @@

 import org.eclipse.egit.github.core.Repository;

+import java.io.CharArrayReader;


typing error (ALT+ENTER) :P

maniac103 · 2017-09-11T05:14:39Z

app/src/main/java/com/gh4a/fragment/CommitFragment.java

+    private static final String STATUS_ADDED = "added";
+    private static final String STATUS_MODIFIED = "modified";
+    private static final String STATUS_RENAMED = "renamed";
+    private static final String STATUS_REMOVED = "removed";


Please move to Github API.

maniac103 · 2017-09-11T05:14:56Z

app/src/main/java/com/gh4a/fragment/CommitCompareFragment.java

+    private static final String HEAD_LABEL = "head_label";
+    private static final String PR = "pr";
+
+


Please remove the second newline.

eagle eyed :P

maniac103 · 2017-09-11T05:17:43Z

app/src/main/java/com/gh4a/fragment/SearchFragment.java

@@ -91,7 +94,7 @@ public static SearchFragment newInstance(int initialType, String initialQuery) {
    private static final String SUGGESTION_ORDER = SuggestionsProvider.Columns.DATE + " DESC";

    private static final String STATE_KEY_QUERY = "query";
-    private static final String STATE_KEY_SEARCH_TYPE = "search_type";
+    private static final String STATE_KEY_SEARCH_TYPE = SEARCH_TYPE;



Extra key and state key are unrelated, please restore the string literal to not give the impression they are related.

maniac103 · 2017-09-11T05:18:55Z

app/src/main/java/com/gh4a/fragment/UserFragment.java

+    private static final String LOGIN = "login";
+    private static final String SORT = "sort";
+    private static final String PUSHED = "pushed";
+    private static final String AFFILIATINO = "affiliation";


Typo.

Also, the filter strings should live in the Github API bindings.

you meant "AFFILIATINO" spelling error right ?

the filter strings should live in the Github API bindings.

I created a Constants file

you meant "AFFILIATINO" spelling error right ?

Yes.

I created a Constants file

Plesse change that to either use the existing IGithubConstants class or put the constants to the respective service classes. If the constants only apply to one service, I'd prefer the latter.

MattiaPrimavera · 2017-09-11T07:15:40Z

Argh I though about the prefix, was not sure on your conventions ...

Tunous · 2017-09-11T08:49:01Z

app/src/main/java/com/gh4a/fragment/EventListFragment.java

@@ -83,6 +83,7 @@

    protected String mLogin;
    private EventAdapter mAdapter;
+    private static final String LOGIN = "login";


I'd say to move it up to the rest of statics.

Tunous · 2017-09-11T08:52:41Z

Not sure if we follow this rule elsewhere but I think it's a good idea to keep static fields above instance fields. I saw that you placed them together in some files.

maniac103 · 2017-09-11T08:54:11Z

Not sure if we follow this rule elsewhere but I think it's a good idea to keep static fields above instance fields.

👍 If we don't do it in all places yet, we definitely should do that.

Tunous

Nearly there. Remaining questions should be answered by @maniac103.

Tunous · 2017-09-11T17:26:50Z

app/src/main/java/com/gh4a/fragment/CommitFragment.java

@@ -191,19 +198,19 @@ protected void fillStats(List<CommitFile> files, List<CommitComment> comments) {
            final LinearLayout parent;

            switch (file.getStatus()) {
-                case "added":
+                case Constants.STATUS_ADDED:


These should go to CommitFile.java instead.

Tunous · 2017-09-11T17:28:38Z

app/src/main/java/com/gh4a/fragment/PullRequestFragment.java

-            return new CommitStatusLoader(getActivity(), mRepoOwner, mRepoName,
-                    mPullRequest.getHead().getSha());
-        }
+                @Override


Revert. This was indented like this on purpose.

Tunous · 2017-09-11T17:30:10Z

app/src/main/java/com/gh4a/fragment/RepositoryListContainerFragment.java

+    private static final String STATE_KEY_QUERY = "search_query";
+    private static final String STATE_KEY_SEARCH_IS_EXPANDED = "search_is_expanded";
+    private static final String FILTER_ALL = "all";
+    private static final String OWNER = "owner";


These will probably need some prefix or be moved to another file.
@maniac103

FilterConstants file is good ?

I have found that RepositoryService already contains some of these so the rest should be moved here too.

For the naming. Filter constants should start with TYPE_ as these already present in RepositoryService. Sort constants should be prefixed with SORT_. And the main, search tag are specific to this fragment so these 2 leave here.

Tunous · 2017-09-11T17:31:51Z

app/src/main/java/com/gh4a/fragment/UserFragment.java

@@ -56,7 +57,7 @@ public static UserFragment newInstance(String login) {
        UserFragment f = new UserFragment();

        Bundle args = new Bundle();
-        args.putString("login", login);
+        args.putString(Constants.EXTRA_LOGIN, login);


Keep extra in this file.

Tunous · 2017-09-11T17:32:45Z

app/src/main/java/com/gh4a/fragment/UserFragment.java

@@ -85,8 +86,8 @@ protected void onResultReady(User result) {
        @Override
        protected Loader<LoaderResult<Collection<Repository>>> onCreateLoader() {
            Map<String, String> filterData = new HashMap<>();
-            filterData.put("sort", "pushed");
-            filterData.put("affiliation", "owner,collaborator");
+            filterData.put(Constants.EXTRA_SORT, Constants.PUSHED);


Probably move to Repository.java?

I moved it to IGithubConstants

Tunous · 2017-09-11T17:33:06Z

github-api/src/main/java/org/eclipse/egit/github/core/util/Constants.java

@@ -0,0 +1,12 @@
+package org.eclipse.egit.github.core.util;
+
+public class Constants {


This file shouldn't be needed.

maniac103 · 2017-09-19T06:58:11Z

app/src/main/java/com/gh4a/fragment/CommitFragment.java

+import static org.eclipse.egit.github.core.CommitFile.STATUS_ADDED;
+import static org.eclipse.egit.github.core.CommitFile.STATUS_MODIFIED;
+import static org.eclipse.egit.github.core.CommitFile.STATUS_REMOVED;
+import static org.eclipse.egit.github.core.CommitFile.STATUS_RENAMED;


Please avoid static imports and use CommitFile.STATUS_* below instead.

maniac103 · 2017-09-19T06:58:59Z

app/src/main/java/com/gh4a/fragment/CommitFragment.java

    public static CommitFragment newInstance(String repoOwner, String repoName, String commitSha,
            RepositoryCommit commit, List<CommitComment> comments) {
+


Please remove that newline.

maniac103 · 2017-09-19T07:00:57Z

app/src/main/java/com/gh4a/fragment/EventListFragment.java

+            Event.TYPE_PUSH, Event.TYPE_ISSUES, Event.TYPE_WATCH, Event.TYPE_CREATE,
+            Event.TYPE_PULL_REQUEST, Event.TYPE_COMMIT_COMMENT, Event.TYPE_DELETE,
+            Event.TYPE_DOWNLOAD, Event.TYPE_FORK_APPLY, Event.TYPE_PUBLIC,
+            Event.TYPE_MEMBER, Event.TYPE_ISSUE_COMMENT


This change is unrelated, please remove.

maniac103 · 2017-09-19T07:01:59Z

app/src/main/java/com/gh4a/fragment/PagedDataBaseFragment.java

    private RootAdapter<T, ? extends RecyclerView.ViewHolder> mAdapter;
    private PageIteratorWithSaveableState<T> mIterator;
    private boolean mIsLoadCompleted;
    private View mLoadingView;

-    private static final String STATE_KEY_ITERATOR_STATE = "iterator_state";


Unrelated change, please remove.

maniac103 · 2017-09-19T07:02:42Z

app/src/main/java/com/gh4a/fragment/PullRequestFragment.java


-   private void fillStatus(List<CommitStatus> statuses) {
+    private void fillStatus(List<CommitStatus> statuses) {


Whitespace errors.

maniac103 · 2017-09-19T07:08:23Z

app/src/main/java/com/gh4a/fragment/UserFragment.java

 public class UserFragment extends LoadingFragmentBase implements View.OnClickListener {
+    public static final String EXTRA_LOGIN = "login";


public -> private

maniac103 · 2017-09-19T07:08:58Z

app/src/main/java/com/gh4a/fragment/WikiListFragment.java

 import org.xml.sax.SAXException;
-


No unrelated whitespace changes please.

maniac103 · 2017-09-19T07:11:29Z

app/src/main/java/com/gh4a/fragment/UserFragment.java

-            filterData.put("sort", "pushed");
-            filterData.put("affiliation", "owner,collaborator");
+            filterData.put(EXTRA_SORT, PUSHED);
+            filterData.put(EXTRA_AFFILIATION, "owner,collaborator");


Those aren't extras, but filter fields. Please move those to RepositoryService (see IssueService for example on variable naming).

maniac103 · 2017-09-19T07:11:51Z

github-api/src/main/java/org/eclipse/egit/github/core/client/IGitHubConstants.java

+
+	String EXTRA_SORT = "sort";
+	String EXTRA_AFFILIATION = "affiliation";
+	String PUSHED = "pushed";


Please remove as per above.

maniac103 · 2017-09-19T07:13:11Z

github-api/src/main/java/org/eclipse/egit/github/core/util/FilterConstants.java

+    public static final String STARRED = "starred";
+    public static final String FULL_NAME = "full_name";
+    public static final String MAIN_TAG = "main";
+    public static final String SEARCH_TAG = "search";


Fragment tags don't belong into the Github API, but to their respective activity.

All other items are filter fields (right?) and thus should be moved to the respective service (RepositoryService, I guess) with appropriate naming.

Tunous · 2018-03-24T16:52:09Z

I've rebased this onto master, fixed mentioned issues and added similar constants to activity files.

Pushed my own changes

MattiaPrimavera mentioned this pull request Sep 11, 2017

Hard-coded Extra parameters #714

Open

maniac103 reviewed Sep 11, 2017

View reviewed changes

Tunous reviewed Sep 11, 2017

View reviewed changes

Tunous previously requested changes Sep 11, 2017

View reviewed changes

maniac103 requested changes Sep 19, 2017

View reviewed changes

Tunous force-pushed the fixing_hard_coded_params branch 2 times, most recently from d9247f1 to 397eea9 Compare March 24, 2018 16:29

Extract hard-coded bundle params names

3e5e2b8

Tunous force-pushed the fixing_hard_coded_params branch from 397eea9 to 3e5e2b8 Compare March 24, 2018 16:51

Tunous requested a review from maniac103 March 24, 2018 16:53

		@@ -26,9 +26,32 @@

		import org.eclipse.egit.github.core.Repository;

		import java.io.CharArrayReader;

		private static final String HEAD_LABEL = "head_label";
		private static final String PR = "pr";

		@@ -0,0 +1,12 @@
		package org.eclipse.egit.github.core.util;

		public class Constants {

		public static CommitFragment newInstance(String repoOwner, String repoName, String commitSha,
		RepositoryCommit commit, List<CommitComment> comments) {


		private void fillStatus(List<CommitStatus> statuses) {
		private void fillStatus(List<CommitStatus> statuses) {

		public class UserFragment extends LoadingFragmentBase implements View.OnClickListener {
		public static final String EXTRA_LOGIN = "login";

Hard-Coded bundle params + few extra strings removed #714 #719

Are you sure you want to change the base?

Hard-Coded bundle params + few extra strings removed #714 #719

Conversation

MattiaPrimavera commented Sep 11, 2017

maniac103 commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maniac103 Sep 11, 2017 • edited

Choose a reason for hiding this comment

MattiaPrimavera commented Sep 11, 2017

Choose a reason for hiding this comment

Tunous commented Sep 11, 2017

maniac103 commented Sep 11, 2017

Tunous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tunous commented Mar 24, 2018

maniac103 Sep 11, 2017 •

edited