first project - Verbesserungsvorschläge am Code

[fLooojava]

Hallo Leute,

ich möchte euch gerne mein erstes Android App vorstellen. Es ist eine einfach gehaltene Application zur Steuerung von einem RGB Band, RGB Lampe oder änliches. Als Datenübertragung habe ich Bluetooth gewählt. Ich war mir nicht sicher, ob ich das ganze auf einem tcp Socket aufbauen soll oder ob ich Bluetooth wählen werde. Schlussendlich habe ich die Bluetooth Übertragung bevorzugt, da ich hier komplett unabhängig von der Umgebung bin, bei der anderen Variante müsste zusätzlich noch ein Netzwerk vorhanden sein. (n+1 Komponenten)


Warum ich hier schreibe - ich möchte euch gerne meinen Code präsentieren und von euch hören, was absolut garnicht geht, was zu verbessern ist und was man anderst machen könnte. Die sämtlichen if-Anweisungen könnte man wohl switch/case umbauen , habe es nur noch nicht umgeschrieben.

Es würde mich sehr sehr freuen, wenn ich Rückmeldungen von erfahrenen User bekommen könnte, wie dies und jenes besser aussehen würde. Über noch so kleine Antwort würde ich mich sehr freuen, solange sie mir weiterhilft. 🙂 Aber da ich in der Lernphase von Java bin, bin ich über jede Bemerkung dankbar!

Hier der Code:

package com.example.rgbled;


import java.io.IOException;
import java.io.OutputStream;

import java.util.UUID;


import android.os.Bundle;
import android.app.Activity;
import android.bluetooth.BluetoothAdapter;
import android.bluetooth.BluetoothDevice;
import android.bluetooth.BluetoothSocket;
import android.content.Intent;

import android.view.Menu;
import android.view.View;
import android.widget.Button;
import android.widget.TextView;
import android.widget.Toast;

public class MainActivity extends Activity {

   private Button Red,Green,Blue,Off,Fade,White,Dark,Bright,Regular,Orange;
   private TextView test;
   private BluetoothAdapter BA;
   private String output;
   private String status;
   private int	bright;
   String address ="20:13:12:54:07:25";

   //BluetoothDevice device = BA.getRemoteDevice(address);
   BluetoothSocket socket;
   OutputStream mmOutputStream;


   @Override
   protected void onCreate(Bundle savedInstancestatus) {
      super.onCreate(savedInstancestatus);
      setContentView(R.layout.activity_main);
      BA = BluetoothAdapter.getDefaultAdapter();
      Red = (Button)findViewById(R.id.btnR);
      Green =(Button)findViewById(R.id.btnG);
      Blue =(Button)findViewById(R.id.btnB);
      Off =(Button)findViewById(R.id.btnOff);
      White =(Button)findViewById(R.id.btnWhite);
      Dark = (Button)findViewById(R.id.btndark);
      Regular = (Button)findViewById(R.id.btnregular);
      Bright = (Button)findViewById(R.id.btnbright);
      Orange = (Button)findViewById(R.id.btnorange);
      
      Fade =(Button)findViewById(R.id.btnF);
      
      if (!BA.isEnabled()) {
          Intent turnOn = new Intent(BluetoothAdapter.ACTION_REQUEST_ENABLE);
          startActivityForResult(turnOn, 0);
          Toast.makeText(getApplicationContext(),"Turned on" 
          ,Toast.LENGTH_LONG).show();
       }
       else{
          Toast.makeText(getApplicationContext(),"Already on",
          Toast.LENGTH_LONG).show();
          }

		

      Red.setOnClickListener(new View.OnClickListener(){
   		public void onClick(View v) {
   			String value = "red";
   			status = "red";
   			sendValue(value);
   			setstatus(status);
   			Toast.makeText(getApplicationContext(), "red r",
    				   Toast.LENGTH_LONG).show();
   		
   		}
       });
      Green.setOnClickListener(new View.OnClickListener(){
   		public void onClick(View v) {
   			String value = "green";
   			status = "green";
   			sendValue(value);
   			setstatus(status);
   			Toast.makeText(getApplicationContext(), "green g",
   				   Toast.LENGTH_LONG).show();
   		}
       });
      Blue.setOnClickListener(new View.OnClickListener(){
   		public void onClick(View v) {
   			String value = "blue";
   			status = "blue";
   			sendValue(value);
   			setstatus(status);
   			Toast.makeText(getApplicationContext(), "blue b",
   				   Toast.LENGTH_LONG).show();
   		}
       });
      White.setOnClickListener(new View.OnClickListener(){
     		public void onClick(View v) {
     			String value = "white";
     			status = "white";
     			sendValue(value);
     			setstatus(status);
     			Toast.makeText(getApplicationContext(), "white w",
      				   Toast.LENGTH_LONG).show();
     		
     		}
         });
      Fade.setOnClickListener(new View.OnClickListener() {
	
    	 
		@Override
		public void onClick(View v) {
			// TODO Auto-generated method stub
			String value = "fade"; 
			sendValue(value);    			
			Toast.makeText(getApplicationContext(), "fade", Toast.LENGTH_LONG).show();}
			
	});
      Off.setOnClickListener(new View.OnClickListener(){
   		public void onClick(View v) {
   			String value = "off";
   			sendValue(value);
   			Toast.makeText(getApplicationContext(), "off o",
   				   Toast.LENGTH_LONG).show();
   		}
       });
      Orange.setOnClickListener(new View.OnClickListener(){
     		public void onClick(View v) {
     			String value = "orange";
     			
     			sendValue(value);
     	
     			Toast.makeText(getApplicationContext(), "orange",
      				   Toast.LENGTH_LONG).show();
     		
     		}
         });

      Dark.setOnClickListener(new View.OnClickListener(){
     		public void onClick(View v) {
     		setBrightness(1);
     		if(getstatus().equals("red")){
            	String value = status+String.valueOf(getBrightnessvalue());
        		//test.setText(status+String.valueOf(getBrightnessvalue()));
            	sendValue(value);
            } else if (getstatus().equals("green")){
            	String value = status+String.valueOf(getBrightnessvalue());
            	sendValue(value);
            } else if (getstatus().equals("blue")){
            	String value = status+String.valueOf(getBrightnessvalue());
            	sendValue(value);
            }else if (getstatus().equals("white")){
            	String value = status+String.valueOf(getBrightnessvalue());
            	sendValue(value);
            }
     		}
         });
      Regular.setOnClickListener(new View.OnClickListener(){
   		public void onClick(View v) {
   		setBrightness(2);
   		if(getstatus().equals("red")){
          	String value = status+String.valueOf(getBrightnessvalue());
      		//test.setText(status+String.valueOf(getBrightnessvalue()));
          	sendValue(value);
          } else if (getstatus().equals("green")){
          	String value = status+String.valueOf(getBrightnessvalue());
          	sendValue(value);
          } else if (getstatus().equals("blue")){
          	String value = status+String.valueOf(getBrightnessvalue());
          	sendValue(value);
          }else if (getstatus().equals("white")){
          	String value = status+String.valueOf(getBrightnessvalue());
          	sendValue(value);
          }
   		}
       });
     	Bright.setOnClickListener(new View.OnClickListener(){
   		public void onClick(View v) {
   		setBrightness(3);
   		if(getstatus().equals("red")){
          	String value = status+String.valueOf(getBrightnessvalue());
      		//test.setText(status+String.valueOf(getBrightnessvalue()));
          	sendValue(value);
          } else if (getstatus().equals("green")){
          	String value = status+String.valueOf(getBrightnessvalue());
          	sendValue(value);
          } else if (getstatus().equals("blue")){
          	String value = status+String.valueOf(getBrightnessvalue());
          	sendValue(value);
          }else if (getstatus().equals("white")){
          	String value = status+String.valueOf(getBrightnessvalue());
          	sendValue(value);
          }
   		}
       });
      
    }

   
    public void on(View view){
    	String address ="20:13:12:54:07:25";
		BluetoothDevice device = BA.getRemoteDevice(address);
      try {
		socket = device.createRfcommSocketToServiceRecord(UUID.fromString("00001101-0000-1000-8000-00805f9b34fb"));
		socket.connect();

      } catch (IOException e) {
		// TODO Auto-generated catch block
		e.printStackTrace();
	}
    }
    
    public void setBrightness(int progress){
 		bright =progress;
 	}
 //getter
 public int getBrightnessvalue(){
  		return bright;
 }
    public void off(View view){

       BA.disable();
       Toast.makeText(getApplicationContext(),"Turned off" ,
       Toast.LENGTH_LONG).show();
    }
  
    // setter
    public void setOutput(String out){
    		output =out;
    	}
    //getter
  public String getOutputvalue(){
	  		return output;
  }
  
  public void setstatus(String statuscolor){
	  status = statuscolor;
  }
  public String getstatus(){
	  return status;
  }
  
    public void sendValue(String value) {
    	if(value.equals("red"))
    	{		
    		setOutput("red\n");
    		getOutputStream_and_sendData();
 
    	}else if(value.equals("green"))
    	{	
    		setOutput("green\n");
    		getOutputStream_and_sendData();
    	
    	}else if(value.equals("blue"))
    	{	
    		setOutput("blue\n"); 
    		getOutputStream_and_sendData();

    	}else if(value.equals("white"))
    	{	
    		setOutput("white\n");
    		getOutputStream_and_sendData();

    	
    	}
    	else if(value.equals("fade"))
    	{	
    		setOutput("fade\n");
    		getOutputStream_and_sendData();

    	}else if(value.equals("off"))
    	{		
    		setOutput("off\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("red1"))
    	{		
    		setOutput("redbrightnessl\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("red2"))
    	{		
    		setOutput("redbrightnessn\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("red3"))
    	{		
    		setOutput("redbrightnessh\n");
    		getOutputStream_and_sendData();
    		
    	}
    	else if(value.equals("green1"))
    	{		
    		setOutput("greenbrightnessl\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("green2"))
    	{		
    		setOutput("greenbrightnessn\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("green3"))
    	{		
    		setOutput("greenbrightnessh\n");
    		getOutputStream_and_sendData();
    		
    	}
    	else if(value.equals("blue1"))
    	{		
    		setOutput("bluebrightnessl\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("blue2"))
    	{		
    		setOutput("bluebrightnessn\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("blue3"))
    	{		
    		setOutput("bluebrightnessh\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("white1"))
    	{		
    		setOutput("whitebrightnessl\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("white2"))
    	{		
    		setOutput("whitebrightnessn\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("white3"))
    	{		
    		setOutput("whitebrightnessh\n");
    		getOutputStream_and_sendData();
    		
    	}else if(value.equals("orange"))
    	{		
    		setOutput("orange\n");
    		getOutputStream_and_sendData();
    	}

    }
	
    public void getOutputStream_and_sendData(){
    	Thread connection = new Thread() {
			public void run() {
  
				try {
		
					OutputStream outputStream = socket.getOutputStream();
					System.out.println(getOutputvalue());
					outputStream.write(getOutputvalue().getBytes());
					outputStream.flush();
					
				} catch (IOException e) {
				} 
			};
		};
	connection.start();
    }
   @Override
   public boolean onCreateOptionsMenu(Menu menu) {
      // Inflate the menu; this adds items to the action bar if it is present.
      getMenuInflater().inflate(R.menu.main, menu);
      return true;
   }
}

Die Oberfläche, wobei die nicht relevant ist- 🙂

Wünsch euch noch nen schönen Abend und würd mich über konstruktive Meldungen sehr freuen. :idea:

flo

 

[VfL_Freak]

Moin,

Warum ich hier schreibe - ich möchte euch gerne meinen Code präsentieren .....

Dann solltest Du auch das richtige Forum nutzen !!
Codeschnipsel u. Projekte

Hier werden nur Anfänger-Themen behandelt - und darum handelt es sich beim Deinem Code sicher nicht!

Gruß
Klaus

 

[Thallius]

Deine Methode sendValue() ist fürchterlich. Mehr als 3 If/Else sollte man niemals verschalchteln.
In Deinem Fall könnte man die Farben auch wunderbar in ein Array packen und die Farbkommandos ebenso. Dann suchst Du dir den passenden Index aus dem Farbarray und nimmst den Eintrag mit dem entsprechenden Index auf dem Farbkommandos-Array.
Noch eleganter wäre es natürlich eine Klasse CommandList zu erstellen, die eine Liste der Farben und Farbkommandos enthält. Weiterhin braucht diese Klasse die Methode getCommandFromColor() die dir dann zu einer Farbe das Commando zurück liefert.

Deine Listener machen fast alle das selbe. Die Listener für die letzten 3 Buttons setzen z.B. nur den Brightness Wert anders. Hier könnte man wunderbar eine eigene Klasse schreiben die von ActionListener erbt und beim Konstruktor einfach den zusätzlichen Parameter brightness bekommt.

Ich würde mal sagen Deinen Code kann man locker auf 1/4 kürzen.


Gruß

Claus

 

[fLooojava]

Weshalb dies keine Anfängerfragen sind ist mir nicht klar, hab schon deutlich komplexeren Code hier gesehen aber mit dem Codeschnipsel/Projekt hast du recht.

@Thallius Vielen Dank für deine Information. Ich werde den Code aufjedenfall nochmal umgestalten, besonders die sendValue Methode ist wirklich fürchterlich. Ich werde hier mal deine genannte Verbesserung mit zwei Arrays ausprogrammieren. Das mit der Brightness klingt gut, aber hier stellt sich die Frage ob eine weitere Klasse für 3 Werte welche übergeben werden sinnvoll ist?

Wünsch euch nen schönen Tag!

flo

 

[Thallius]

Es lohnt sich schon ab dem zweiten mal wo du einen Code wiederverwendest. Es ist nicht nur übersichtlicher es ist auch viel einfacher zu warten. Stell Dir vor du musst jetzt an dem Listener später noch was erweitern oder ändern. Dann musst du höllisch aufpassen, dass du nicht eine Stelle vergisst wo du den Code geschrieben hast. Ist es eine eigene Klasse, musst du nur an einer Stelle ändern und es passt überall in deinem Programm trotzdem.

Gruß

Claus

 

[fLooojava]

Hallo Claus,

ich habe nun ein String Array initialisiert mit sämtlichen bisher möglichen Farbvarianten. Mir ist aber noch nicht klar, wie ich die enorme if-Abfrage in der sendValue Methode beseitigen kann. Es wird immer ein String (red,green,blue,...) als Argument übergeben und nun wird abgefragt um was für ein String es handelt. Jenachdem wird dann folgende weitere Methode, in meinem Fall setOutput("beliebigerwert"); und getOutputStream_and_sendData(); aufgerufen. Wie kann ich denn diese massive if Abfrage umgehen? Das ist mir noch nicht wirklich klar.

Ein zweites Array für die Farbkommandos finde ich nicht sinnvoll, hier kann ich doch immer jeweils ein Indexeintrag vom Color Array hernehmen und hinter diesen String ein +"/n" setzen? Klar, wenn der String deutlich abweicht macht dies kein Sinn, jedoch ist er momentan identisch, nur ohne Umbruch.



flo

 

[Thallius]

Aehm, also wenn der zu sendende String immer gleich dem übergebenen Parameter ist nur mit einem \n dahinter, dann brauchst du ja nicht einmal ein Array oder gar einen If-Abfrage. Dann sieht deine sendValue einfach so aus

public void sendValue(String value) {
            setOutput(value+"\n");
            getOutputStream_and_sendData();
 }

Gruß

Claus

 

[fLooojava]

Wenn's wirklich nur ein "\n" ist ja, sonst muss hald eben noch jeweils eine Zeile mehr rein. Aber das wäre alles unklug. Die komplette if-Abfrage ist somit sinnlos. Da fallen dann gleich schonmal 50 Zeilen Code weg 😉

flo

 

[fLooojava]

@Thallius:

Was haltest du denn von dieser Lösung bezüglich der Brightness?

vorher:

Regular.setOnClickListener(new View.OnClickListener(){
        public void onClick(View v) {
        setBrightness(2);
        if(getstatus().equals("red")){
            String value = status+String.valueOf(getBrightnessvalue());
            //test.setText(status+String.valueOf(getBrightnessvalue()));
            sendValue(value);
          } else if (getstatus().equals("green")){
            String value = status+String.valueOf(getBrightnessvalue());
            sendValue(value);
          } else if (getstatus().equals("blue")){
            String value = status+String.valueOf(getBrightnessvalue());
            sendValue(value);
          }else if (getstatus().equals("white")){
            String value = status+String.valueOf(getBrightnessvalue());
            sendValue(value);
          }
        }
       });

nachher:

Regular.setOnClickListener(new View.OnClickListener(){
   		public void onClick(View v) {
   		setBrightness(2);
 		for (int i=0; i<ColorArray.length; i++) {
 		if(getstatus().equals(ColorArray[i])){
        	sendValue(ColorArray[i]+String.valueOf(getBrightnessvalue()));
        }
        }
   		}
       });