N0ury
A large number of small changes to improve readability and maintainability
I'm using this project as a way of becoming more familiar with both Android and Java.
My personal way of doing this is to do a code review exercise.
I have made a lot of small changes all aimed at making the code easier to read and safer to maintain. The largest change is the introduction of an enum called Security that allows the switch statements that define proto and acceptcrt to be eliminated along with several other if statements. I have tried to use strong typed fields and methods instead of using strings.
It seems to work but I can't claim to have tested it extensively.
Anyway, if you think it is useful here it is.
Thanks, very good idea. I’m a little bit busy, and I’ll be on leave this week. I’ll try to have a look at your changes next week
Noury
Le 5 nov. 2016 à 01:40, kwhitefoot [email protected] a écrit :
I'm using this project as a way of becoming more familiar with both Android and Java.
My personal way of doing this is to do a code review exercise.
I have made a lot of small changes all aimed at making the code easier to read and safer to maintain. The largest change is the introduction of an enum called Security that allows the switch statements that define proto and acceptcrt to be eliminated along with several other if statements. I have tried to use strong typed fields and methods instead of using strings.
It seems to work but I can't claim to have tested it extensively.
Anyway, if you think it is useful here it is.
You can view, comment on, or merge this pull request online at:
https://github.com/nbenm/ImapNote2/pull/28
Commit Summary
• Added comments to make it easier to understand • Added comments. • Reformatted all files to get consistent indentation, etc. • Corrected Accont -> Account. • Added proto and acceptcrt properties to Security enum so that there is no longer a need for a switch statement to find them. • Folder override can never be null so do not check for it. File Changes
• M ImapNote2/build.gradle (4) • M ImapNote2/src/main/AndroidManifest.xml (152) • D ImapNote2/src/main/java/com/Pau/ImapNotes2/AccontConfigurationActivity.java (349) • A ImapNote2/src/main/java/com/Pau/ImapNotes2/AccountConfigurationActivity.java (356) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Data/ConfigurationFile.java (255) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Data/ImapNotes2Account.java (88) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Data/NotesDb.java (157) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/ImapNotes2.java (102) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Listactivity.java (458) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Miscs/ImapNotes2Result.java (26) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Miscs/ImapNotesAuthenticatorService.java (159) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Miscs/Imaper.java (286) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Miscs/OneNote.java (106) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Miscs/Sticky.java (78) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Miscs/SyncThread.java (34) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Miscs/UpdateThread.java (137) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/NewNoteActivity.java (72) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/NoteDetailActivity.java (266) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/NotesListAdapter.java (84) • A ImapNote2/src/main/java/com/Pau/ImapNotes2/Sync/Security.java (75) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Sync/SyncAdapter.java (120) • M ImapNote2/src/main/java/com/Pau/ImapNotes2/Sync/SyncUtils.java (857) • M ImapNote2/src/main/res/layout/account_selection.xml (354) • M ImapNote2/src/main/res/layout/main.xml (12) • M ImapNote2/src/main/res/layout/new_note.xml (12) • M ImapNote2/src/main/res/layout/note_detail.xml (41) • M ImapNote2/src/main/res/layout/note_element.xml (45) • M ImapNote2/src/main/res/menu/detail.xml (54) • M ImapNote2/src/main/res/menu/list.xml (37) • M ImapNote2/src/main/res/menu/newnote.xml (8) • M ImapNote2/src/main/res/values-v14/styles.xml (29) • M ImapNote2/src/main/res/values/strings.xml (5) • M ImapNote2/src/main/res/values/styles.xml (25) • M ImapNote2/src/main/res/xml/authenticator.xml (9) • M ImapNote2/src/main/res/xml/searchable.xml (4) • M ImapNote2/src/main/res/xml/syncadapter.xml (7) • M build.gradle (2) • M gradle/wrapper/gradle-wrapper.properties (4) Patch Links:
• https://github.com/nbenm/ImapNote2/pull/28.patch • https://github.com/nbenm/ImapNote2/pull/28.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.
I would start squashing related commits if I were you. Its a bit hard to review in this state due to the numerous commits. For example, all the commits that add comments to the code can be squashed together.
Edit: swash changed to squash
It never occurred to me that anyone would really be interested enough to make it worthwhile to be more organised. The fork is really just for me to use as a learning exercise. So far all of the changes are simply tidying up the code to reduce the risk of things like null references, uncaught exceptions, etc. , and to introduce enumerated types and named constants to reduce the risk of typos.
Now that someone has shown an interest I'll make an effort to make the changes in a more readable fashion.
One question, "swashing": is this some git related term? I'm far from being an expert git user, I normally use tfs in my professional life.
On 29 Nov 2016 14:52, "Jeremy Newton" <[email protected]mailto:[email protected]> wrote:
I would start swashing related commits if I were you. Its a bit hard to review in this state due to the numerous commits. For example, all the commits that add comments to the code can be swashed together.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/nbenm/ImapNote2/pull/28#issuecomment-263575048, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AALNd2wcRanf-8sHfLV9YeVu0DZtWQIWks5rDC4LgaJpZM4KqHOu.
I don't see why not, improving opensource code is always a good thing! Also sorry, I meant to say "squashing". I was typing on my phone this morning and auto-correct had a sense of humour ;)
This guide is pretty good: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html But if you use a gui tool, doing a rebase would depend on what you use.
Basically squashing is just merging similar commits together to make it cleaner for the pull request. I.e. you can merge the first two commits you made together, as they are both adding comments to the code.
Note that you'll need to do a forced push when you push back to GitHub, as it will need to remove some commits.
@Mystro256 I don't think I have time to redo the commits that are already there but I'll certainly bear in mind the idea for the future.
@kwhitefoot Well you don't have to redo them per-say, squashing just merges commits together, so rather than lots of little commits, you can merge the ones that are related together.