Coding Styles Horror

Ich habe mal wieder einen netten Codeschnippsel in einem Projekt, das ich weiterführen soll gefunden.

// initialize view for current user (always)
if(true || User.IsLoggedIn)
{
    // ... 
}
else
{
    // ...
}

Eben in einem MSDN Forum gefunden:

private void button2_Click(object sender, EventArgs e)
        {
            timer1.Interval = Convert.ToInt32(1000);
        }

Video auf Youtube: Vortrag "C# zum Gruseln" (1:05:41) 😄

ansehen

Zusatz: gerade den Tweet gesehen unter Tweed zu "Ausgabe 4.2014 des Entwickler Magazins!"
Mal schauen, ob der Beitrag zu C# ("C# Universaltalent mit Fehlerpotenzial") noch mehr "Futter" bietet...

Edit(h): tut er, auch wenn "nur" 3 Fehler abgehandelt werden

hmm.... 😕

public string EncodeFilename(string oldString)
        {
            string[] arr = oldString.Split('.');
            int count = arr.Length - 1;

            string newString = HttpContext.Current.Server.HtmlDecode(oldString).ToLower().
                       Replace(arr[count], "").Replace("ä", "ae").Replace("ö", "oe").Replace("ü", "ue").Replace("ß", "ss").
                       Replace(".", "").Replace("&", "").Replace("&", "").Replace("!", "").Replace("'", "").Replace("§", "").
                       Replace("²", "").Replace("³", "").Replace("/", "").Replace(";", "").Replace("*", "").Replace("#", "").
                       Replace(" ", "").Replace("+", "-").Replace("¨", "-").Replace("-", "_") + "." + arr[count].ToString();

            //Ausgabe
            return newString;
        }

Ich kann zwar keine exakten Codeschnippsel geben, aber lustig sind auch aktuell SQL-Statements über ein ganzes Projekt hingezogen, die sich weder an unsere Coding-Guidelines halten noch einen geringen Umfang haben (teilweise 60 Zeilen) und nicht mal das zurück liefern, was sie sollen. Und immer und immer wieder das selbe.

Ich habe vor kurzem ein (scheinbar authorisiertes) Video von der BASTA Conference 2014 entdeckt. Der Titel ist "C# zum Gruseln"

http://www.youtube.com/watch?v=4reR1PCHSIA

Hierbei werden recht unterhaltsam einige krasse No-Go's aufgezeigt.

Anmerken möchte ich das mir für einige vorgestellte Fälle im 2. Teil des Videos durchaus plausible Szenarien einfallen die den vorgestellten Fall rechtfertigen. (Es ist relativ unfair ein paar Zeilen Code aus dem Zusammenhang zu reissen ohne das der Entwickler dazu Stellung nehmen kann)

Ausserdem beanstandet der Tutor einen hier auf mySharp.de vielgebenen Hinweis:
Er behauptet:

if(null == x)

ist Slut, vielmehr ist;

if(x==null)

dann eben doch besser.

🤔

Die Variablen Namen hab ich abgeändert.

                int result = GetValue();
                int value = 0;
                switch (result)
                {
                    case 100:
                        value = 100;
                        break;
                    case 85:
                        value = 85;
                        break;
                    case 50:
                        value = 50;
                        break;
                    case 25:
                        value = 25;
                        break;
                    case 10:
                        value = 10;
                        break;
                    case 0:
                        value = 0;
                        break;
                }

Auch schön:

	private void Timer1Tick(object sender, System.EventArgs e)
		{
			this._nTimerZaehler1++;
			Application.DoEvents();
		}
		private void Timer2Tick(object sender, System.EventArgs e)
		{
			this._nTimerZaehler2++;
			Application.DoEvents();
		}
		private void NamegändertTick(object sender, System.EventArgs e)
		{
			this._nNamegändertTick++;
			if (this._nNamegändertTick == 1)
			{
				if (this._oNamegändertTick != null)
				{
					this._oNamegändertTick.DisconnectAll();
					this._tRelaisTimer.Stop();
					this._tRelaisTimer.Enabled = false;
				}
			}
			Application.DoEvents();
		}

Warum kompliziert:

 private bool isIn( char ch ) {
			switch ( ch ) {
				case 'ö':
					return true;
				case 'Ö':
					return true;
				case 'ü':
					return true;
				case 'Ü':
					return true;
				case 'ä':
					return true;
				case 'Ä':
					return true;
				case 'ß':
					return true;
				case '§':
					return true;
				case '²':
					return true;
				case '³':
					return true;
				case 'µ':
					return true;
				default:
					break;
			}
			return false;
		}

wenns auch einfach gehen würde:

private bool isForbidden( char ch )
        {
            string forbiddenChars = "öÖüÜäÄߧ²³µ"; //oder besser noch als Konstante auf Klassenebene
            return forbiddenChars.Contains(ch);
        }

Bring ich mich doch auch mal hier ein:

           
if (nAnzahlGeräte > 1)
{
   lMehrereUnterschiedlicheGeraete = true;
}

if (!lMehrereUnterschiedlicheGeraete == false)
{
   [..]
}

Ich muss das refactoren und habe mir einen Wolf gesucht, da ich das ! überlesen hatte...

Falls jemand glaubt, hier waere nur nichts mehr los, weil es keinen schlechten Code mehr gibt:

What do you mean, bad style?

LaTino (beim auditing)

@LaTino Beeindruckend. Das toppt sogar meines..

Für einen Kunden musst wir mal eines unserer Java Frameworks nach C# konvertieren.

Leider blieb diese sinnvolle Hierarchie erhalten -> erleichtert ja die Schreibarbeit der armen Applikationsentwickler 😉

Statt "VAGenStringUtils.Methodenname" sollen sie ja nur "Methodenname" schreiben müssen.

Gerade in einem Projekt von 2004 das ich auf den aktuellen Stand bringen soll gefunden (Beispielhaft):

public class Config : ExecutorConfig
{
     public override void Copy(object source)
     {
           base.Copy(source);
           this.InputPath = new Document();
           this.InputPath.Path = source.InputPath.Path;
           // und so weiter..           
     }

     public Document InputPath { get; set; }

     public Document OutputPath { get; set; }
}

public class Executor
{
      public void Execute()
      {
            string inputPath = this._config.InputPath.Path;
            // Daten ermitteln
            // Daten aufbereiten
            ths.WriteOutput(this._config.OutputPath.Path, daten);
      }
}

War so frei und habe InputPath und OutputPath gegen Strings ersetzt. 👍

Bin neulich in einer Klasse über eine Exception gestolpert, die einen Tippfehler in der Nachricht hatte, die sie bekam. Hab den Typo, da ich schonmal dabei war, korrigiert.

Plötzlich schlägt ein Test in einem anderen Projekt fehl. Nachgeschaut, ob ich das war...und finde folgendes Stück Code in einer im Test verwendeten Klasse:

try 
{
    //dosomething
}
catch(SomeException ex)
{
    if(ex.Message == "Nachricht mit Tippfeler") DoSomethingElse();
}

Mal völlig abgesehen davon, dass man sich an der Message-Property orientiert - ich bin ja auch faul. Aber lieber auf die Nachricht mit Tippfehler prüfen, statt den zu korrigieren - das schlägt meine Faulheit nochmal 😄.

LaTino

Ein Klassiker 😉

        public static double GetCacheSize()
        {
            return 0;
        }

Ob sich die CacheSize mal ändert - ich bin gepsannt =)

Hallo,

von mir selbst gecodet...

if ((_count == 0 && index == 0) || (index == _count))

Geht doch glatt einfacher:

if (index == _count)

😁

mfG Gü

In C++ ist folgendes möglich:

++i = ++i + ++i;

Was würde die Console für i anzeigen?

int i=0; 
++i = ++i + ++i;
cout<<"i = "<<i;

Bei mir wurde der Wert 6 angezeigt, erst wurde i dreimal inkrementiert, dann wurde 3 + 3 addiert.

Im C++ -Forum wurde das als Undefined Behavior eingestuft und mir als UB-Test empfohlen:

int i=0, a, b, c; 
a = ++i = (b=++i) + (c=++i); 
cout<<"a = "<<a<<endl; 
cout<<"b = "<<b<<endl; 
cout<<"c = "<<c<<endl; 
cout<<"i = "<<i<<endl;

        public static double GetCacheSize()
        {
            return 0;
        }

...ähm ja, Klassiker 🙂

hallo, ich finde es nicht lustig - es ist zum Heulen ...

if ((list.Count(p=>p.IsSelected) -1) == 0 )

=> Liste leer.
??

!! => Fix: (eingecheckt, Wochen später ...)

if ((list.Count(p=>p.IsSelected) -1) < 0 )

=> Liste leer.

wenn ich jetzt sage, dass die "-1" überflüssig ist, befürchte ich bleibt

if ((list.Count(p=>p.IsSelected)) < 0 )

=> Liste leer.
??

stehen und der Fehler ist wieder drin ....... es sei denn, in der Liste ist
ein "Blind"-Objekt enthalten, was immer da drin ist. Leider weit und breit
kein Kommentar und der Rest vom Code sieht genauso aus ...

was das IsSelected überhaupt soll - ich weiß es nicht.

Moin

Ich muss gestehen, meinen Einstand im Lesterthread zu geben, fühlt sich etwas komisch an, aber was solls. ^^

Ursprünglich war das mal VB, habs leicht abgeändert um zu anonymisieren.

private void Button_Click(object sender, RoutedEventArgs e)
{
	Button btn = sender as Button;
	if (btn != null) {
		MessageBox.Show(((Button)sender).Name);
	} else {
		MessageBox.Show("Error: Es wurde kein Button geklickt!!");
	}
}

Wenn Ihr immer schon einen "FancyRandomNameGenerator" haben wolltet, hier das Snippet for free:

var randomString = new Random(DateTime.Now.Second).ToString();

Bitte aber im UnitTest nur einmal aufrufen... wird schon laufen.

Ich hab mal wieder was bei einem Review gefunden, nachdem ich erst mal nen Kaffee brauchte:

public const String TRUE = "true";
public const String FALSE = "false";

Und dazu

if (obj.Value.ToString().ToUpper() == TRUE)

Folgendes habe ich mal beim Review gesehen und ging mir seitdem nicht mehr aus dem Kopf:

string xyz = textBox1.Text.ToString();

Grüße
leafar

Ich bin ja schon immer ein großer Fan davon gewesen, Konvertierungen zwischen Datentypen durchzuführen, indem man zuerst den Urprungswert in einen String konvertiert, und dann den String in den Zieltyp parst, so z.B.:

short currentYear = short.Parse(DateTime.Now.Year.ToString());

Das hat eigentlich nur Vorteile:

• Es ist unperformant,
• Es ist unlesbar und
• Es ist eine potentielle Fehlerquelle

Wenn man das nur konsequent durchzieht, dann sieht das z.B. so aus:

short currentYear = short.Parse(DateTime.Now.Year.ToString());
short currentMonth = short.Parse(DateTime.Now.Month.ToString());

if (selectedYear == 0 || (selectedYear < model.StartYear || selectedYear > model.EndYear))
{
	selectedYear = short.Parse(DateTime.Now.Year.ToString());
}

if (selectedYear != short.Parse(DateTime.Now.Year.ToString()))
{
	currentMonth = 0;
}

Das ist jetzt kein Code von einem Praktikanten, sondern von einer großen internationaler Firma, deren Hauptgeschäft darin besteht, Software für andere große internationale Firmen zu entwickeln. Das Problem ist hier wahrscheinlich, daß man Leuten, die sich mit der Geschäftslogik auskennen, den Auftrag gibt: "Setzt das mal in C# um, ihr wißt ja am besten wie das funktioniert, und programmiert habt doch auch schonmal."

Mal etwas von den hochgeschätzten Webfrickelkollegen:

Bin gerade über ein react-Projekt gestolpert, das im Ordner für die statischen Inhalte einen Unterordner für die Icons und dort in -zig Unterordnern für verschiedene Bildgrößen (Schema "8x8", "16x24", "16x16" usw.) ausschließlich .svg-Icons gesammelt haben. Hm - wofür stand das s in svg gleich wieder?

X(

LaTino

Static. Eindeutig Static... 😁

Ich belebe das Mal wieder neu mit diesem kleinen Snippet:

private string _ConnectionString = @"data source=1.2.3.4\My;initial catalog=MyDB;Application Name=MyApp;user id=sa;password=;persist security info=False;packet size=4096;";

private async Task OnBerechnen()
{
	return await Task.Factory.StartNew(() =>
	{
		using (System.Data.SqlClient.SqlConnection conn = new System.Data.SqlClient.SqlConnection(_ConnectionString))
		{
			conn.Open();
			
			// ...
		}
	});
}

Das stand so im Code, am ConnectionString habe ich nur IP und Name geändert, UserId und Passwort standen genau so im Code.
Mich macht sowas sprachlos ...

Natürlich ist das nicht mehr der Fall und den Server gibt's auch nicht mehr, aber das lief jahrelang so.

Natürlich ist das nicht mehr der Fall und den Server gibt's auch nicht mehr, aber das lief jahrelang so.

Böse gesagt - alles richtig gemacht wenn es jahrelang so lief, oder? 😃 Wir machen uns eindeutig zu viele Gedanken über Sicherheit und Kennwortstärke...

Aber es ist doch so einfach, Mal eben einen simplen User anzulegen, der nur Lese-Rechte hat 😕
Ich verstehe nicht, warum man das damals nicht getan hat, ich kann nur froh sein, dass es bisher keinen Angreifer gab, der auf die Idee kam, die Anwendung zu decompilen und nach ConnectionStrings zu suchen, immerhin läuft das Programm auf DAU-Rechnern.

Und ich hab die ersten 30 Sekunden darüber nachgedacht was daran so schlimm sein könnte eine Connection in nem eigenen Task aufzumachen 😄

public static Nullable<T> GetEnum<T>(int? EnumInt) where T : struct {
    foreach (var item in Enum.GetValues(typeof(T))) {
        if ((int)item == EnumInt) {
            return (T?)item;
        }
    }
    return null;
}

Okay, soweit, so...ungewöhnlich, aber gut, wenn einer das lieber selber macht als die .NET-Methoden zu nehmen, des Menschen Wille ist sein Himmelreich.

Aber dann. Die Methode wird zweimal verwendet. Beide Male so:

(MyEnumType) Utils.GetEnum<MyEnumType>((int) data.Property)

Die Punchline? data.Property ist vom Typ MyEnumType.
Wir nehmen das Enum, casten nach int, werfen das int in eine Methode, die einen Nullable von dem Enum baut, von dem wir kommen, und casten dieses Nullable dann hart in den Ursprungstyp.

*mikedrop*

LaTino

Sieht nach einem "2 Uhr Morgens" Code aus und macht auch genau soviel Sinn 😃

T-Virus

Der Urheber ist nicht mehr bei meinem AG beschäftigt. Seinem Code nach zu urteilen, hat er aber sehr oft "nachts gearbeitet". Der ist jetzt übrigens (wieder) in der Lehre beschäftigt - lehrt OOP. Manche Geschichten kann man sich nicht ausdenken.

LaTino

Sieht für mich wie Code aus, den man über einen längeren Zeitraum von Int-Flags auf ein Enum umstelen musste und dabei die eine oder andere Zwischenlösung vergessen hat.
Hab ich so in der Art schon öffter gesehen 😄

Ich habe am 29.02.2020 lernen müssen, dass dieser Codeschnipsel wahrscheinlich nicht der geschickteste ist.
Zum Glück war das ein Samstag...
Autor war ich leider selbst.

Due = DateTime.Now;
From = new DateTime(Due.Year - 1, Due.Month, Due.Day);

Urza

if (true) // wg. Scope innerhalb switch-case
{
    ...
}

Hallo,

private static string GetXyzInstanceString()	// Xyz changed from actual name
{
	// We are going to hardcode the value here to 3 (a random number) so that we don't have to
	// ...
	int id = 3;
	return id.ToString(CultureInfo.InvariantCulture);
}

wurde geändert zu

private static string GetXyzInstanceString()	// Xyz changed from actual name
{
	// We are going to hardcode the value here to 3 (a random number) so that we don't have to
	// ...
	return "3";
}

Immer KISS berücksichtigen.

mfG Gü

Hallo,

ein Bug sollte sich im Idealfall durch (Unit-) Tests reproduzieren lassen und falls nicht, so sollte dafür ein Test erstellt werden. Auch damit es später einmal zu keiner Regression kommen kann.

Gibt es jedoch bereits Tests mit passenden Test-Argumenten und der Bug lässt sich dennoch nicht reproduzieren, so mag es u.U. auch am Test-Code selbst liegen.

actual.Should().BeSameAs(actual);

Notiz aus dieser Geschichte:
In solch einem Fall durch bewusste Fehl-Eingaben für den Test, so dass dieser zwangsweise fehlschlagen muss, eine Validierung vom Test-Code vornehmen.
Schlägt der Test nämlich nicht fehl, so ist der Test-Code faul.

Weitere Infos dazu:
Testgetriebene Entwicklung
What is red/green testing?

mfG Gü

Ich habe neulich auch was interessantes entdeckt. Ist zwar kein C# aber naja 😉
Vielleicht kennt der ein oder andere das Computerspiel Airline Tycoon aus den früheren 2000ern. Da ist mittlerweile der Quelltext verfügbar.

Hier eine Funktion ob ein spezielles Feature vorhanden ist bzw. ob die Bots bestimmte Features nutzen dürfen:

//--------------------------------------------------------------------------------------------
//Unterstützen die Roboter im aktuellen die Level ein bestimmtes Feature:question:
//--------------------------------------------------------------------------------------------
bool RobotUse (SLONG FeatureId)
{
   SLONG Level;

   //Die verschiedenen Levelstrukturen in eine Reihe bringen:
   if (Sim.Difficulty>=DIFF_TUTORIAL && Sim.Difficulty<=DIFF_FINAL) Level = Sim.Difficulty;
   else if (Sim.Difficulty==DIFF_FREEGAME || Sim.Difficulty==DIFF_FREEGAMEMAP) Level = 6;
   else if (Sim.Difficulty>=DIFF_ADDON01 && Sim.Difficulty<=DIFF_ADDON10) Level = Sim.Difficulty-DIFF_ADDON01+7;
   else if (Sim.Difficulty>=DIFF_ATFS01 && Sim.Difficulty<=DIFF_ATFS10) Level = Sim.Difficulty-DIFF_ATFS01+7+10;

   //Tabelle ermitteln für welche Level ein Feature ermittelt wird:
   const char *pFeatureDesc;
   switch (FeatureId)
   {
      //0-5 : Level im Basisspiel
      // F  : Freies Spiel
      //0-9 : Level in Addon / FlighSecurity
      //
      //"-." : Disabled
      //"xX" : Enabled
      //"!"  : Enabled, but wasn't enabled in AT 1.0
      //"?"  : Enabled, but isn't enabled in AT First Class
      //                                            Basisspiel       Addon        FlightSecu      
      //                                                012345   F   0123456789   0123456789 
      case ROBOT_USE_SABOTAGE         : pFeatureDesc = "-XXXXX" "X" "XXXXXXXXXX" "-XXXXXXXXX"; break;
      case ROBOT_USE_FRACHT           : pFeatureDesc = "------" "X" "-XXXXXXXXX" "XXXXXXXXXX"; break;
      case ROBOT_USE_WERBUNG          : pFeatureDesc = "---XXX" "!" "XXXXXXXXXX" "-XX-X-XXXX"; break;
      case ROBOT_USE_AUFSICHT         : pFeatureDesc = "----XX" "!" "----------" "----------"; break;
      case ROBOT_USE_NASA             : pFeatureDesc = "-----X" "." "---------X" "----------"; break;
....
      case ROBOT_USE_MAX20PERCENT     : pFeatureDesc = "XXXXXX" "!" "XXXXXXXXXX" "XXXXX-XXXX"; break;
      case ROBOT_USE_TANKS            : pFeatureDesc = "------" "." "----------" "--------XX"; break;
      case ROBOT_USE_DESIGNER         : pFeatureDesc = "------" "?" "----------" "---XX-XX--"; break;
      case ROBOT_USE_DESIGNER_BUY     : pFeatureDesc = "------" "." "----------" "----X--X--"; break;

      default:
         TeakLibW_Exception (FNL, ExcNever);
   }

   return (pFeatureDesc[Level]!='-' && pFeatureDesc[Level]!='.') && (!bFirstClass || pFeatureDesc[Level]!='?');
}

Die Lösung fande ich irgendwie sehr interessant 😁 Generell ist da der halb deutsche Quelltext sehr lustig zu lesen.

Siehe:
AirlineTycoon/src/Player.cpp