Feedback zur eigenen App + paar Fragen

[werdas34]

Hallo,

für das Studium habe ich die Aufgabe eine App zu schreiben.
Einen mobilen Einkaufszettel. Dieser kann die typischen Aufgaben erledigen. Einträge erstellen, löschen, updaten (CRUD).
Einzige Besonderheit: Bei der Eingabe des Produktnamens soll ein Textvervollständigungsmenü aufploppen. Dieses Menü zeigt passende Einträge an, die man schonmal in die Liste eingetragen hat.

Dem Prof sind einhalten der Separation of Concern, Software-Design, MVVM, angemessene Datenstrukturen und so weiter sehr wichtig.
Der Fokus auf dem Design ist ihm deutlich weniger wichtig. ("Es sollte stimmig sein. minimal ansprechendes (sinnvolles) graphisches Design")

Ich habe meine App auf Gitlab hochgeladen und würde die Experten bitten mal drüber zu schauen. Wichtig wäre mir ob ihr einen groben Schnitzer/Verstoß gegen ein Pattern oder eine Sache die man viel eleganter lösen könnte findet.
Gerne könnt Ihr die App auch ausführen. Es liegt eine Room DB zugrunde und als AVD Device eignet sich am besten ein Pixel 2. Warum das Pixel 2, dazu komme ich später nochmal zurück.

Zu meinen Fragen:
1. Ich habe ein ListView. Und bei Klick auf ein bestimmtes Item, wird dieses im Bearbeitungsmodus geöffnet. Um die Infos des Objekt in der neuen Activity zu haben, gibt es mehrere Wege. Ich habe es mit Parcelable und Intent gelöst. Man könnte auch eine DB Abfrage machen, wenn man die Id per Intent übergibt. Was wäre der beste Weg? Parcelable oder DB-Abfrage? In meinem Use-Case spielt das eine untergeordnete Rolle, da man nie mehr als 50 Einträge gleichzeitig haben wird.

2. Warum ein Pixel 2? Dem Prof ist das Design nicht so wichtig. Und das Pixel 2 passt von den Maßen sehr gut zu meinem Test-Device (Galaxy S5). Leider ist es so wenn ich es auf meiner Hardware ausführe, das die EntryActivity sehr verschoben wird (Speichern Button im Landsscape wird garnicht angezeigt). Wie müsste man das Layout umschreiben, dass es auf beiden Geräten gleich aussieht? Responsive für alle Smartphone-Größen brauche ich nicht.
Dazu verstehe ich eine Sache nicht. Im Layout-Preview habe ich das Pixel 2 eingestellt, das AVD ist auch ein Pixel 2. Ich musste das "Menge"-Feld extra versetzt anbringen, damit es auf dem AVD bündig zu den anderen Feldern erscheint. Warum?



3. Zu guter Letzt möchte ich den Code noch refactorn. Abschnitte in Funktionen auslagern, etc. Unteranderem möchte ich gerne viele Warning von Android Studio auflösen (Lambda Ausdruck statt anonyme Klasse, etc). Auch die Ressourcen reinigen. Bei sämtlichen String-Werten im Layout eine Verweis auf eine Id einbauen.
Gibt es da einen Weg, das möglichst automatisch zu machen? Bzw ohne das ich jedes Warning einzeln durchgehe?

Vielen Dank.

mfg werdas34

 

[Mart]

Gibt es da einen Weg, das möglichst automatisch zu machen? Bzw ohne das ich jedes Warning einzeln durchgehe?

normalerweise sollte deine IDE eine Möglichkeit bieten dein PRojekt zu bereinigen.. das kann ein paar warnings wie unused und @override warnings weg zaubern und das automatisch

 

[Mart]

   protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        // Get reference from view elements + viewmodel
        this.toolbar = findViewById(R.id.toolbar);
        this.btnAddEntry = findViewById(R.id.btnAddEntry);
        this.listview = findViewById(R.id.listView);
        this.viewModel = new ViewModelProvider(MainActivity.this).get(ViewModel.class);

        // Set tasks for every view element
        setSupportActionBar(toolbar);

        this.btnAddEntry.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                openEntryView(null);
            }
        });

        this.viewModel.getAllEntrys().observe(this, new Observer<List<EntryModel>>() {
            @Override
            public void onChanged(List<EntryModel> entrys) {
                entryList = new ArrayList<>(entrys);
                entryAdapter = new EntryAdapter(MainActivity.this, entryList);
                listview.setAdapter(entryAdapter);
            }
        });

        this.listview.setOnItemClickListener(new AdapterView.OnItemClickListener() {
            @Override
            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
                openEntryView((EntryModel) listview.getItemAtPosition(position));
            }
        });

        this.listview.setOnItemLongClickListener(new AdapterView.OnItemLongClickListener() {
            @Override
            public boolean onItemLongClick(AdapterView<?> adapterView, View view, int i, long l) {
                new AlertDialog.Builder(MainActivity.this)
                        .setTitle("Löschen")
                        .setMessage("Möchtest du den Eintrag wirklich löschen?")
                        .setPositiveButton("Ja", new DialogInterface.OnClickListener() {
                            public void onClick(DialogInterface dialog, int which) {
                                EntryModel entry = entryList.remove(i);
                                try {
                                    int deletedRows = viewModel.deleteEntry(entry);
                                } catch (ExecutionException e) {
                                    e.printStackTrace();
                                } catch (InterruptedException e) {
                                    e.printStackTrace();
                                }
                                entryAdapter.notifyDataSetChanged();
                            }
                        })
                        .setNegativeButton("Nein", null)
                        .setIcon(android.R.drawable.ic_dialog_alert)
                        .show();
                return true;
            }
        });
    }

wenn man scrollen muss um 1e Methode ganz zu sehen weis man dass es nicht passt... eg single reponisibility

man muss das java meme wo einer einen panorama bildschirm hat wahr machen. wo drunter steht " finally i can see the complete hello world class on my screen"

 

[werdas34]

normalerweise sollte deine IDE eine Möglichkeit bieten dein PRojekt zu bereinigen.. das kann ein paar warnings wie unused und @override warnings weg zaubern und das automatisch

Ja, ich muss mich da mal bisschen einarbeiten. Habe mit AndroidStudio noch nie was zu tun gehabt. Und die Warnings-Auflösenden Tools der IDEs habe ich noch nie benutzt.

eg single reponisibility

Da gebe ich dir recht. Das wollte ich noch machen. Also zumindest vermehrt Funktionen auslagern, wo es möglich ist.
Was ich nicht ganz weiß inwiefern ich alles in der onCreate()-Methode belassen kann/soll. Ich kenne den Lifecycle, aber ich habe ehrlich gesagt keine Ahnung, wann man lieber etwas direkt in onCreate() behandelt oder erst in der onStart().
Der Aufbau der Activity und Init-Sachen in der onCreate() und dann die listener in onStart() ? Macht das Sinn? Oder doch alles in onCreate() belassen?

Was ich auf jeden fall machen werde ist, dass ich die Codeabschnitte bei den Listener auslagere in eigene Funktionen. Die onCreate() wird dadurch etwas schlanker, aber auch nicht immens.

 

[Mart]

Ja, ich muss mich da mal bisschen einarbeiten. Habe mit AndroidStudio noch nie was zu tun gehabt. Und die Warnings-Auflösenden Tools der IDEs habe ich noch nie benutzt.


Da gebe ich dir recht. Das wollte ich noch machen. Also zumindest vermehrt Funktionen auslagern, wo es möglich ist.
Was ich nicht ganz weiß inwiefern ich alles in der onCreate()-Methode belassen kann/soll. Ich kenne den Lifecycle, aber ich habe ehrlich gesagt keine Ahnung, wann man lieber etwas direkt in onCreate() behandelt oder erst in der onStart().
Der Aufbau der Activity und Init-Sachen in der onCreate() und dann die listener in onStart() ? Macht das Sinn? Oder doch alles in onCreate() belassen?

Was ich auf jeden fall machen werde ist, dass ich die Codeabschnitte bei den Listener auslagere in eigene Funktionen. Die onCreate() wird dadurch etwas schlanker, aber auch nicht immens.

du hast im moment ungefähr das ziel zu erreichen

[CODE=java]oncreate ()
{

    this.diesesundjenes()

      

    this.jenesunddieses()

      

    if (isJenesNull())

    {
        dannTuDieses();

    }

voidDiesesUndJEnes(){
    this . dieses und jenes {
      
    }
}
voidJenesUndDieses(){
    this. jenes und dieses {
        ...
            ...
            ...
    }
}
    boolean isJenesNull(){
        return jenes == 0 ;
    }
    void ddann tu dieses (){
        ...
            ...
    }

und schwups hast du in oncreate nur noch wörter und eine sehr verbale Methode da da jeder zuerst hinschaut ... das ist von kneitzel die unterschrift "written prose" ... die hauptmethode ist wie eine Textstelle die man liest und man weis was passiert OHNE die logik jemals gesehen zu haben und dieses kannst du tun ohne dass du 1 zeile der logik veränderst


du hast einfach alle methodne in eine rein gebacken und keine sau weis was passiert

 

[werdas34]

Ich weiß, was du meinst.
Jedenfalls interessant, das die main() quasi ein Text ist, damit man den Ablauf des Codes erkennen kann. Die genaue Implementierung aber erst bei in den Funktionen zu erkennen ist. Ich habe das so noch nie gesehen. Nice.

Ich meine so wie ich das bei den Optionen schon gemacht habe soll die onCreate() auch aussehen.
[CODE lang="java" title="MainActivity"]
@Override
public boolean onOptionsItemSelected(MenuItem item) {
switch (item.getItemId()) {
case R.id.optionDeleteAll:
showDeleteAll();
return true;
case R.id.optionCredits:
showCredits();
return true;
default:
return super.onContextItemSelected(item);
}
}

private void showCredits() {
startActivity(new Intent(MainActivity.this, CreditActivity.class));
}

private void showDeleteAll(){
new AlertDialog.Builder(MainActivity.this)
.setTitle("Löschen")
.setMessage("Möchtest du wirklich alle Einträge löschen?")
.setPositiveButton("Ja", new DialogInterface.OnClickListener() {
public void onClick(DialogInterface dialog, int which) {
try{
int deletedRows = viewModel.deleteAllEntrys();
String str = "";
if(deletedRows == 1) {
str = " Eintrag wurde gelöscht";
}else{
str = " Einträge wurden gelöscht";
}
Toast.makeText(MainActivity.this, deletedRows + str, Toast.LENGTH_SHORT).show();
} catch (ExecutionException e) {
e.printStackTrace();
} catch (InterruptedException e) {
e.printStackTrace();
}
entryList.clear();
entryAdapter.notifyDataSetChanged();
}
})
.setNegativeButton("Nein", null)
.setIcon(android.R.drawable.ic_delete)
.show();
}[/CODE]

Auch muss ich auf DRY achten. Zum Beispiel habe ich den AlertBuilder zweimal, kommt noch ein Dritter da ich auch alle bereits abgehakte Einträge löschen möchte. Und der ist immer gleich bis auf das setMessage und setPositiveButton, da muss ich jedesmal den onClick neu schreiben.

 

[Mart]

da muss ich jedesmal den onClick neu schreiben.

dann nimmst du einfach alles was in onclick drin steht und lagerst es aus

dann steht in onclick

onlick{
    machFancyDinge();
}
machFancyDinge(){
    das was vorher in onclick war
}

was mir aus dem Shcnipsel auffällt

switch (item.getItemId()) {
            case R.id.optionDeleteAll:
                showDeleteAll();
                    return true;
            case R.id.optionCredits:
                showCredits();
                return true;
            default:
                return super.onContextItemSelected(item);
        }
    }

das switch kannst du umbauen in die neuen yield switches ... also die einen return wert haben

 return switch (item.getItemId()) {

            case R.id.optionDeleteAll => {
            
                showDeleteAll();
                yield true;
            }
.....

        }

    }

ausserdem hat R.id eine aussage kraft wie kartoffel puffer

    private void showDeleteAll(){

        new AlertDialog.Builder(MainActivity.this)

                .setTitle("Löschen")

                .setMessage("Möchtest du wirklich alle Einträge löschen?")

                .setPositiveButton("Ja", new DialogInterface.OnClickListener() { clicker -> onClick()

                                                                                })

                .setNegativeButton("Nein", null)

                .setIcon(android.R.drawable.ic_delete)

                .show();

    }
                                   public void onClick(DialogInterface dialog, int which) {

                        try{

                            int deletedRows = viewModel.deleteAllEntrys();

                            String str = "";

                            if(deletedRows == 1) {

                                str = " Eintrag wurde gelöscht";

                            }else{

                                str = " Einträge wurden gelöscht";

                            }

                            Toast.makeText(MainActivity.this, deletedRows + str, Toast.LENGTH_SHORT).show();

                        } catch (ExecutionException e) {

                            e.printStackTrace();

                        } catch (InterruptedException e) {

                            e.printStackTrace();

                        }

                        entryList.clear();

                        entryAdapter.notifyDataSetChanged();

                    }

                }

wird wahrschienlich jetzt eine endlosschleife sein weis ich gar nicht... aber das kriegst schon hin die methode umzubenennen

dies alles was ich bis jetzt gesagt habe ist halt nur clean code speziell, ich habe keine ahnung von android aber guten code zu schreiben sollte ich hinkriegen, javafx ist ja ähnlich zu dem android ding, zumindest in manchen sachen

wahrscheinlich braucht man beim lambda ausdruck 2 werte ... aber du kannst den click listener auch einfach außer halb erstellen
EDIT: was ich jetzt erst sehe... das "immer true" zurück geben kann "vllt" verbessert werden muss aber nicht

 

[Mart]

es ist wesentlich einfacher wenn du schnipsel zeigst die man annörgeln kann ... da niemand lust hat sich durch deine 20 bis 30 klassen oder was es sind durchzugraben

 

[werdas34]

yield gibts in der Android SDK noch nicht.
Stream z.B. gibts auch erst seit der Version 24/25. (aktuell ist die Version 31)

Zu dem AlertBuilder habe ich ne Frage:
Meine Idee ist es, im Vorfeld die Daten, die immer gleich bleiben (Titel, Icon) zu setzen. Und je Alert ändert sich die Message und die Funktion, wenn auf Ja geklickt wird. Die beiden setze ich dann zur Laufzeit.

in der init() passiert auch das:

        this.alertDialog = new AlertDialog.Builder(MainActivity.this)
                .setTitle("Löschen")
                .setNegativeButton("Nein", null)
                .setIcon(android.R.drawable.ic_delete);

Dann hätte ich gerne eine Methode buildDialog: Die soll die beiden Sachen dann entsprechend anpassen.

Ich müsste eine Funktion als Parameter übergeben. In dieser steht dann die Aufgabe, die bei Klick auf den Ja-Button ausgeführt werden soll.
Das ist mit Lambda auch möglich. So wie ich das aber sehe, muss man immer ein ein Interface darumbauen.

Kann man das nicht irgendwie einfacher lösen? Im Idealfall einfach in der selben Klasse eine Methode nutzen? Dann müsste der Typ der methods eben der Klassenname sein..

private void buildDialog(String message, MainActivity methods){
    this.alertDialog
        .setMessage(message)
        .setPositiveButton("Ja", (dialog, which) -> methods);
}

private void löscheAlle(){
    //alles aus der DB löschen
}

// Aufruf
buildDialog("Möchten Sie alles löschen?", löscheAlle);
bzw.
buildDialog("Möchten Sie alles löschen?", p - > löscheAlle);

 

[Mart]

ich habe zwar nicht verstanden was du willst mit dem build aber es hört sich nach builder pattern an bzw factory pattern bzw fluent api

warum machst du nicht einfach etwas über ein lambda ausrduck? du machst es doch selber permanent mit den fancy handlern usw und sofort

interface superToll{
    makeFancy();
}
class ALert{
    superToll methode;
 
    public Alert(superToll methode){
        this.methode = methode
        setActionhandler...{
            this.methode.makeFancy()
        }
    }
}


....
    ....
    mainDings(){
    var Alert = new Alert(new superToll{
        sysout Alles gelöscht;
    })
}

das was du gezeigt hast entspricht keinem builder pattern
das nimmst du her wenn du optionale parameter hast ( die hast du ja ) und deine Factory baut sich dann alles zusammen du hast aber eine private builder methode die bringt dir relativ wenig

eine fluent api könntest du so machen

public interface AlertFluent
{
    interface Creator{
        Generator create(String titel, String name);
    }
    interface Generator{
        Generator setActivity();
        Generator setMessage()
        void build();
    }
 
}

class Alert implements AlertFluent.Creator, AlerFluent.Generator {
}

in deiner main kannst du sie dann so bauen

// die factory ist erfunden.. du musst es irgendwie "anstarten"
var alert = AlertFactory.start()
                .create("Löschen?","Löscher") // hier wird ein Generator zurück geliefert also kannst du die methode nur einmal benutzen während dem bauen
                .setActivity(new Activity (){ ... })
                .setMessage("Alles kaputt")
                .build();

hier kannst du ein ( miserables meines erachtens weils zu lange ist ) beispiel sehen wies gehen kann https://dzone.com/articles/java-fluent-api-design

zu builder und factory pattern findest du massen im internet musst du dir nur suchen


in meiner bibliothek hab ich es so benutzt falls dir das was hilft... hier macht man halt fancy dinge mit dem return wert

public interface RFluentControllerAPI {

  /**
   * The Generator resolves all {@link RapidfxAutoGenerate} Annotated Fields in
   * the Objects.
   */
  public interface Generator {
    Connector generate(Object... components);

    RapidController get();
  }

  /**
   * The Connector. Which will resolve the Given annotations.
   */
  public interface Connector {
    Connector connect(Object component, Object connectOn, Class<? extends Annotation> onAnnotation);

    Connector connectWithController(Object component);

    <CONTROLLERT extends RapidController> CONTROLLERT get();
  }
}

und die klassen schauen dann so aus

public class RFluentController<CONTROLLERT extends RapidController>
    extends RFLuentAbstract<CONTROLLERT>
    implements RFluentControllerAPI.Generator, RFluentControllerAPI.Connector {
    ....
    }

[CODE lang="java" title="Aufruf"]
Rapidfx.create(this)
.generate(view.getLoginBox())
.connectWithController(view.getLoginBox())
.get();
[/CODE]

  public static <CONTROLLERT extends RapidController> RFluentController<CONTROLLERT> create(
      final CONTROLLERT controller) {
    return new RFluentController<>(controller);
  }

das ist mein create was nur einfach einen "builder" oder fluent... oder was auch immer es ist zurück gibt zum "anstoßen"
builder pattern und fluent api ist nahe zusammen so nebenbei erwähnt

 

[Mart]

Außerdem wenn man nach checkstyle geht und konventions blabla... müsstest du bei jeder public mehtode ein javadoc kommentar haben aber ich glaub das sprengt den rahmen nbischl.. auf jeden fall erhöht es den gedankengang "muss das wirklich public sein ich mag nich schon wieder ein kommentar schreiben"