Bot Xmpp - Prośba o Code Review

Hej,

Napisałem ostatnio bota do 4p i so. Jakby mógł ktoś rzucić okiem i powiedzieć czego się powinienem wystrzegać, co robię źle to byłbym wdzięczny :D

Starałem się do tematu podejść w TDD

Mam 32 testy,
.

GH: https://github.com/pixellos/4pBot

Za miesiąc minie rok jak się bawię w programowanie - mniej więcej na jakim poziomie jestem :P?

Jakie wyciągnąłem wnioski?
Nie używać jednej struktury do kilku rzeczy - teraz mam klasę command która mi służy za maskę komend a także za nośnik informacji o niej. Przez to mam utrudnione np zrobienie fajnego helpa.

Nie korzystać z gołych dictionary, tylko jakoś opakować :).

Jakby mógł ktoś rzucić okiem i powiedzieć czego się powinienem wystrzegać

https://github.com/pixellos/4pBot/blob/e9cc6e72691710bfd23b54abbe4d1d5ec3428baa/4pBot/Program.cs

Mieszania tabów i spacji ;)

Wcięcia na githubie wyglądają strasznie.

            clientConnection = new XmppClientConnection()
            {
                AutoPresence = true,
                Password = "123456",
                Username = "bot4p",
                Server = "4programmers.net",
            };

https://github.com/pixellos/4pBot/blob/e9cc6e72691710bfd23b54abbe4d1d5ec3428baa/4pBot/Model/ComunicateService/XmppFree.cs

Podawania hasła plaintextem w kodzie który jest w systemie kontroli wersji.

3. strasznie skomplikowane jest porównywanie komend:

	    public static bool operator ==(Command command1, Command command2)
	    {
            bool isActionNamesEquals =  command2.ActionName.Equals(Any) || command1.ActionName.Equals(Any) || (command1.ActionName.Equals(command2.ActionName,StringComparison.OrdinalIgnoreCase) ) ;

	        bool isNegationEquals = command1.TypeOfCommand.Equals(command2.TypeOfCommand)
                || command1.TypeOfCommand == CommandType.Any
                || command2.TypeOfCommand == CommandType.Any;

	        bool isSenderEquals = command1.Sender.Equals(command2.Sender, StringComparison.OrdinalIgnoreCase) || command1.Sender.Equals(Any) ||
	                              command2.Sender.Equals(Any);

            bool areParametersEqual = true;

	        for (int i = 0; i < command1.Parameters.Length|| i < command2.Parameters.Length; i++)
	        {
                if (command1.Parameters.Length <= i || command2.Parameters.Length <= i)
	            {
                    if (command1.Parameters.Length <= i && command2.Parameters?[i] == Any)
                    {
                        break;
                    }
                    if (command2.Parameters.Length <= i && command1.Parameters?[i] == Any)
                    {
                            break;
                    }
                    areParametersEqual = false;
	                break;
	            }

                if (command1.Parameters[i] == Any || command2.Parameters[i] == Any)
	            {
	                continue;
	            }
                
                areParametersEqual = command1.Parameters[i].Equals(command2.Parameters[i],StringComparison.OrdinalIgnoreCase);
	        }

	        return isSenderEquals && isNegationEquals && isActionNamesEquals && areParametersEqual;
}

Nie da się go prościej zapisać?

I w dodatku podejrzane. Nie czytałem dokładnie, ale to jakiś "fuzzy matching"? W sensie. np. z tego co widzę, jeśli komenda ma parametr "Any" to jest równy każdemu innemu parametrowi.

W takim razie to nie jest operator ==, i zmieniłbym to na jakąś funkcje "Match". Z takim operatorem == daleko nie zajdziesz, bo łamiesz przechodniość (z a == b oraz b == c nie wynika że a == c -> to nie (tylko) matematyczny formalizm i czepianie się definicji, ale też coś na czym podświadomie polegasz (i inni czytający kod polegają) podczas używania operatora "==".

 var escapedTag = command.Parameters[1].Replace("#","%23");

Nie patrzyłem do czego to dokłądnie (coś przed wysłaniem requestu) ale wygląda na hack - może powinieneś zrobić całe urlencode od razu?

return "Sorry, i cant find anything.";

=>

Sorry, I can't find anything :P

		private static string MagicWith4PSubject(string subject)
		{
			return subject
                .Replace(' ', '_')
                .Replace('(', '_')
                .Replace(')', '_')
                .Replace('[', '_')
                .Replace('#', '_')
                .Replace('@', '_')
                .Replace('!', '_')
                .Replace(']', '_')
                .Replace('?','_')
                .Replace('!','_')
                .Replace(".","")
                .Replace('&','_')
                .Replace('^','_')
                .Replace(':','_')
                .Replace("__","_")
                .TrimEnd('_');
		}

Dobre :D.

Ale do czego tego używasz? Pewnie jest lepszy sposób niż reimplementowanie algorytmu normalizacji który może się zmienić w coyote w każdej chwili ;). A nawet jeśli, to pewnie w coyote to jest inaczej zaimplementowane (starczyło zapytać) - np. zmiana wszystkiego spoza \w na _.

Dzięki :P

3. To jest koszt błędnej decyzji projektowej o robieniu wszystkiego na Command - potrzebne mi było maskowanie komend, żeby je dopasować do istniejących w słowniku. W pewnym momencie zabrnąłem w to za daleko :P Będę się tego wystrzegał w przyszłości :)

4. Tak, to jest hack, żeby # było w SO rozpoznawane :P UrlEncode na pewno sprawi, że będzie czytelniejsze.

5. Ano, mogłem zapytać przed pisaniem takiego potworka, używam do konwersji danych z jsona, a dokładniej tematu na adres:P.
   To jak to jest dokładnie z tym algorytmem normalizacji w Coyote ;)?