Kommentare bei einzeiligen IF-Statements

[kirdie]

IF-Statements mit nur einer Anweisung schreibe ich meist so:

if(userID.startsWith("x")) {grantPriviledges();}

weil mir

if(userID.startsWith("x"))
{
 grantPriviledges;
}

zu lang ist.
Jetzt ist aber mein Problem, wo ich jetzt Kommentare zur Anweisung hinschreibe.

// user is admin?
if(userID.startsWith("x")) {grantPriviledges();}

Gefällt mir nicht wegen dem Fragezeichen aber

if(userID.startsWith("x"))
{
 // user is admin
 grantPriviledges();
}

braucht wieder so viel Platz.

Welche Variante würdet ihr bevorzugen?

P.S.: Mir fällt auch noch diese Variante ein:

 // if the user is admin, grant priviledges
if(userID.startsWith("x")) {grantPriviledges();}

Aber das ist auch nicht so optimal, weil der zweite Teil gar nicht kommentierungsbedürftig ist.

 

[maki]

if(userID.startsWith("x")) {
    grantPriviledges();
}

Aus Gewohnheit, ist ein sehr religiöses Thema wo die Klammern hinkommen.... Kommentare spart sich sich am besten, schreibe lieber sauberen, verständlichen und ausdrucksfähigen Code, zB.:

if(isAdmin(userID)) {
    grantPriviledges();
}
...
private boolean isAdmin(String userId) {
    return userId.startsWith("x");
}

Würde [c]userId[/c] nehmen anstatt [c]userID[/c].

Man könnte natürlich überlegen die isAdmin Methode in die Userklasse (oder wo sie halt hinpasst) direkt zu verschieben.

 

[kirdie]

Für das Beispiel finde ich deinen Ansatz super, aber bei meinem Anwendungsfall gefällt er mir nicht so gut.
Ich habe eine Funktion, die URLs abkürzt, also z.B. aus "https://www.google.com/calendar" "google:calendar" macht und das Gegenstück dazu programmiere ich gerade, die Funktion "expand".

	public static String expand(String shortURI)
	{		
	// already expanded?
	if(shortURI.startsWith("http://")) {return shortURI;}
	int prefixEnd = shortURI.indexOf(':');
	// no prefix found?
	if(prefixEnd==-1) {return shortURI;}
	String prefix = shortURI.substring(0, prefixEnd);
	String baseURI = getURI(prefix);
	if(baseURI==null) {return shortURI;}
	return shortURI.replace(prefix+':', baseURI);
}

Das würde ja dann folgendermaßen aussehen:

public boolean alreadyExpanded(String shortURI)
{
 return shortURI.startsWith("http://");
}

public boolean prefixFound(int prefixPosition)
{
 return (prefixPosition >=0);
}

public static String expand(String shortURI)
{			
	alreadyExpanded(shortURI) {return shortURI;}
	int prefixEnd = shortURI.indexOf(':');
	if(!prefixFound(prefixEnd)) {return shortURI;}
	String prefix = shortURI.substring(0, prefixEnd);
	String baseURI = getURI(prefix);
	if(baseURI==null) {return shortURI;}
	return shortURI.replace(prefix+':', baseURI);
}

Das wäre dann wieder etwas aufgebläht. Vielleicht lasse ich einfach den zweiten Kommentar weg und gehe davon aus, dass der Betrachter sowieso weiß, dass -1 für "keinen Treffer" steht?

P.S.: Mache ich mir über solchen Kleinkrams zu viele Gedanken oder geht euch das auch so? Ich will halt den "perfekten" Code haben

 

[maki]

Das wäre dann wieder etwas aufgebläht.

Stimmt nicht

Die main Methode in deiner Variante ist 9 Zeilen lang, die neue nur 7, sind aber beide schlecht, weil:
2 Anweisungen in 1 Zeile schreiben ist immer schlecht:

if(shortURI.startsWith("http://")) {return shortURI;}

Das geht gar nicht, nach keinem Style Guide.

P.S.: Mache ich mir über solchen Kleinkrams zu viele Gedanken oder geht euch das auch so? Ich will halt den "perfekten" Code haben

Lesetipp: "Clean Code" von Robert "Bob" Martin

 

[despikyxd]

also mal ganz davon abgesehen das bei "einzeiligen IF's" die klammern des blocks weggelassen werden können würde ich sowas hier verwenden

if(prefixEnd==-1)
	return shortURI;

wie gesagt ... so lange der block *also das was dann ausgeführt werden soll wenn if true ist* kann so lange es nur eine anweisung ist auch ohne die lästigen klammern geschrieben werden ...
der compiler übersetz es so wie er es soll .. nicht wie er es will

if(prefixEnd!=null)
	if(prefixEnd==-1)
		//TO-DO
	else
		if(prefixEnd==0)
			//TO-DO
		else
			//TO-DO

und genau so wie es logisch aussieht wird es auch übersetz ...
das funktioniert auch bei for(; und while() und glaub auch noch bei irgendwas anderen ^^
darum würde ich es bei EINE-ANWEISUNG-fällen komplett ohne klammern sondern nur mit entsprechender einrückung machen ... der compiler setz dann die klammern automatisch ..
was allerdings NICHT geht sowas hier

if(whatEver.startsWith("x"))
	anweisung(); nochNeAnweisung();

logisch gesehen würde es so übersetz werden ... der compiler holt aber das nochNeAnweisung() in die nächste zeile was dazu führt es IMMER ausgeführt wird .. egal ob if true oder false
beweis

public class test
{
	public static void main(String[] args)
	{
		if(true)
			System.out.println("1"); System.out.println("2");
		if(false)
			System.out.println("3"); System.out.println("4");
	}
}

sollte laut logik nur 1 und 2 ausgeben ... das ergebnis ist aber 1 2 4 ... zeigt also das obwohl if false ist die folgende anweisung nicht mehr zum if gehört und normal ausgeführt wird ...
bei verschachtelung MÜSSEN klammern gesetzt werden

public class test
{
	public static void main(String[] args)
	{
		if(true)
			System.out.println("1"); System.out.println("2");
		if(false)
			System.out.println("3"); System.out.println("4");
		if(true)
			if(true)
				System.out.println("5"); System.out.println("6");
			else
				System.out.println("7"); System.out.println("8");
		if(true)
			if(false)
				System.out.println("9"); System.out.println("10");
			else
				System.out.println("11"); System.out.println("12");
	}
}

führt zum compiler-error : ELSE WITHOUT IF ...
die verschachtelung geht auch wieder nur soweit wie es eine anweisung bleibt ... wenn eine weitere anweisung folgt wird diese wieder als nächster block compiled ...
erst wenn man sich wirklich an die EINE-ANWEISUNG-regel hält oder halt klammert funktioniert es

public class test
{
	public static void main(String[] args)
	{
		if(true)
			System.out.println("1"); System.out.println("2");
		if(false)
			System.out.println("3"); System.out.println("4");
		if(true)
			if(true)
				System.out.println("5"); //System.out.println("6");
			else
				System.out.println("7"); //System.out.println("8");
		if(true)
			if(false)
				System.out.println("9"); //System.out.println("10");
			else
				System.out.println("11"); //System.out.println("12");
	}
}

ergibt die gewollte ausgabe 1 2 4 5 11
da auf die EINE if anweisung DIEREKT das else folgt zählt diese auch noch dazu ... wird das durch eine weitere anweisung im if-block unterbrochen fliegt das ganze im übertragenen sinn aus ein ander und führt zum obigen compile-error ...

wollte euch das nur noch mal so mitteilen wenn man sich die klammern ganz sparen will

 

[maki]

Wenn man sich die Klammern spart, hat man auch gleichzeitig viele Gelegenheiten für blöde Fehler

 

[kirdie]

Die Klammern hab ich früher auch weggelassen aber das ist bei uns die Konvention und mittlerweile finde ich das auch sinnvoll.

 

[Haave]

Sowas wie

if(irgendwas()) {return true;}

find ich ehrlich gesagt potthässlich ^^

Wenn man sich die Klammern spart, hat man auch gleichzeitig viele Gelegenheiten für blöde Fehler

Aus dem Grund nehme ich einen Mittelweg und schreibe kurze if-Statements nicht so:

if(irgendwas())
    return true;

sondern so:

if(irgendwas()) return true;

Wenn ich dann nämlich merke, dass noch ne Zeile reinmuss, bin ich gezwungen, Klammern zu nehmen (und werde wohl nicht nach dem Semikolon von return true weiterschreiben ). Kommentare dann eben einfach dahinter.
So schreibe ich es am liebsten.

 

[despikyxd]

nun ja .. ich programmiere mit java nur in meiner freizeit nebenbei ...
es zum beruf machen ... hmm .. habe schon drüber nachgedacht ... aber ich bin selbst nach n paar jahren immer noch n anfänger ...
kleine GUI-spiele mit multiplayer-funktion ... datenbank-administration ... content- und application-server *basierend auf eigenen protcolen* ... und n paar kleine spielereien ... aber auch ich lerne immer noch was dazu
grade was den graphischen bereich angeht bin ich so gut wie planlos was man alles so machen und wie man es produktiv entwikelt ...
ich mein ... ich arbeite nicht mit einer IDE sondern nur mit notepad ... *oder auf unix mit vim* ... so richtig große anspruchsvolle projekte mit mehreren tausend zeilen code oder jeder menge klassen ... nee ... alleine wird sowas auch sehr zeitaufwändig
und das bisschen was ich dann mal puliziere wird dann auch mit vollständigem source ausgeliefert ...
und da viele die dann die entsprechenden apps genutz haben eher auf die funktionalität der app fixiert waren anstatt sich das ding mal anzukuggn und mal n bissl dran rumschrauben um die leistung zu steigern ... viele davon haben von programmieren sowieso keine ahnung
von daher ist es eigentlich ziemlich egal wie ich programmiere ...
erlich gesagt : ich schreibe auch immer die klammern mit um fehler zu vermeiden .. wollte nur aufzeigen das es auch ohne funktioniert

 

[gee_20_4_7]

ich schreibe auch immer die klammern mit um fehler zu vermeiden .. wollte nur aufzeigen das es auch ohne funktioniert

Ein schöner Fehler zu diesem Thema ist das "dangling else". Passt hier ganz gut

if(a)
    if(b)
        m();
else
    n();

Hier trügt der Schein, der Compiler bezieht hier das else auf das innere if...

Da kann man doch drauf reinfallen finde ich...

 

[tommysenf]

public boolean alreadyExpanded(String shortURI) {
   return shortURI.startsWith("http://");
}
 
public boolean noPrefixFound(String shortURI) {
   return shortURI.indexOf(':') < 0;
}
 
public static String expand(String shortURI) {   
    if(alreadyExpanded(shortURI) || noPrefixFound()){
        return shortURI;
    }
    int prefixEnd = shortURI.indexOf(':')
    String prefix = shortURI.substring(0, prefixEnd);
    String baseURI = getURI(prefix);
    return baseURI==null ? shortURI : shortURI.replace(prefix+':', baseURI);
}

Wenn du es gern kompakt magst kannst du natürlich auch folgendes schreiben:

public static String expand(String shortURI) {   
    return shortURI.startsWith("http://") || shortURI.indexOf(':') < 0 ? shortURI : getURI(shortURI.substring(0, shortURI.indexOf(':'))) == null ? shortURI : shortURI.replace(shortURI.substring(0, shortURI.indexOf(':'))+':', getURI(hortURI.substring(0, hortURI.indexOf(':'))));
}

Aber ob das wirklich perfekter Code ist?

 

[Cola_Colin]

Die Formatierung, wie sie von Eclipse oder Netbeans per Automatik erzeugt wird plus einige extra Leerzeilen ist eigentlich recht brauchbar, d.h.:

    public static boolean alreadyExpanded(String shortURI) {
        return shortURI.startsWith("http://");
    }

    public static boolean prefixFound(int prefixPosition) {
        return (prefixPosition >= 0);
    }

    public static String expand(String shortURI) {
        if (alreadyExpanded(shortURI)) {
            return shortURI;
        }

        int prefixEnd = shortURI.indexOf(':');

        if (!prefixFound(prefixEnd)) {
            return shortURI;
        }

        String prefix = shortURI.substring(0, prefixEnd);
        String baseURI = getURI(prefix);

        if (baseURI == null) {
            return shortURI;
        }

        return shortURI.replace(prefix + ':', baseURI);
    }

Ich sehe keinen Grund, wieso Code nicht ein paar Zeilen mehr brauchen sollte.
Lesbarkeit ist das entscheidende, nicht kürze.
Wobei mir das runtersetzen der Klammer aus Gewohnheit dann doch gegen den Strich geht.
Religiöses Thema eben