Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intial #1

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Intial #1

wants to merge 2 commits into from

Conversation

mhemdan
Copy link

@mhemdan mhemdan commented Sep 30, 2018

No description provided.

Copy link

@IslamSalah IslamSalah left a comment

Choose a reason for hiding this comment

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

@mhemdan

  • Overall I think everything is very good. Good job! I liked the blink22_boilerplate name BTW.
  • I believe the sample followed is not that much organized. This might be reflected in that it somewhat has a few stars on github and a few claps on its related medium articles:
  • I believe other highly starred samples are more organized. Check
    - Mindorks
    - Bourbon
    - Google Todo

At the end of the day, having different point of views doesn't mean that someone is right and the other is wrong. But it will lead us to have a better decision. We may discuss more in the next meeting. CC @engmonsh

@@ -0,0 +1,10 @@
*.iml

Choose a reason for hiding this comment

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

@mhemdan I believe the .gitignore file needs to be revisited as some irrelevant files are tracked. I believe this file can be helpful.

Copy link
Author

Choose a reason for hiding this comment

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

Sure we can modify it and this file is really helpful we can use it as a Ref


// Dependencies Section

libraries = [

Choose a reason for hiding this comment

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

@mhemdan I believe a developer might be confused which should be added to libraries and which to appLibraries. The categorization in this link can be helpful also.

Copy link
Author

@mhemdan mhemdan Sep 30, 2018

Choose a reason for hiding this comment

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

good point and we can work on that to make it look more understandable but I have one comment regarding this link as you see there are many dependence repeated I know this is for the sake of modularization but I don't know if this is good or not we can discuss that further more

import com.tapadoo.alerter.Alerter;
import com.tapadoo.alerter.OnHideAlertListener;

public abstract class BaseActivity extends AppCompatActivity implements BaseView{

Choose a reason for hiding this comment

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

@mhemdan I think we can use IcePick in the base activity and all the extending ones won't suffer from having the state parameters nulled once the activity destroyed. Check the library Readme file

- core : contains almost all app logic pure java -> presenters - api client - models

## Code Standards
[Code Style](.github/CodeStyle.md)

Choose a reason for hiding this comment

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

@mhemdan If I understand correctly, the code style adopted here is from this repo. Although it looks good it hasn't much stars. This code style has many stars so it can be helpful.

Copy link
Author

Choose a reason for hiding this comment

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

this code style was only for unifying naming conventions but for sure we can use this plugin too as it has many stars.

@@ -0,0 +1 @@
/build

Choose a reason for hiding this comment

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

@mhemdan Although I totally agree with distributing the code among different modules, I believe we can categorize code in modules in a more arranged way. I believe it may be more organized to have a module handling the model & presentation code and another one handling the view code.

Check the attached screenshot from this project
screen shot 2018-09-27 at 3 56 29 pm

import dagger.Provides;

@Module
public class ExampleModule {

Choose a reason for hiding this comment

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

@mhemdan I believe the DI classes, e.g Modules & Components, need to be in a separate package rather than being distributed among different packages. I found many projects, Google Todo, Mindorks, and Bourbon doing this. Check the attached screenshot from this project
screen shot 2018-09-27 at 3 49 38 pm

@mahmoudyusuf94
Copy link

Nice work Hemdan.
I agree with Islam that other samples like Mindorks is very well organised. I think also we follow their convention in the current IEHP project with similar architecture and even package names and distributions.
You may take a look at this sample repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants