Microsoft z każdą wersją wprowadza pewne nowe elementy do języka, które teoretycznie powinny poprawić możliwości języka, jak komfort pracy programisty. Od wersji .net 4.0 wprowadzono nowy typ – [mark]dynamic[/mark]. Jego użycie pozwala na wykonanie czynności, które będą dopiero znane w momencie wykonania aplikacji. W dużym skrócie oznacza to, że kompilator pozwoli na wywołanie dowolnej akcji na obiekcie bez zgłoszenia błędu. Kod zostanie bez problemu skompilowany, nawet, jeśli nie istnieje metoda do której się odwołujemy. Błąd wystąpi dopiero w momencie wykonania kodu, jeśli nie będzie tej metody.
W tytule jest code review… Myślałem, że sporo rzeczy już widziałem w kodzie, ale okazuje się, że kreatywność programistów nie zna granic. Ostatnio natrafiłem na kod napisany zgodnie z poniższą ideą.
W kodzie był zdefiniowany interfejs, który umożliwiał tylko odczytać właściwości obiektu:
interface IReadOnlyFoo
{
int SomeValue { get; }
}
Była również definiowana metoda, która używała obiektów implementujących ten interfejs jako argumentu:
public void DoSomething(IReadOnlyFoo obj)
{
...
}
Na razie nic ciekawego. Pisząc kod pewnie nawet nie zajrzałbym do metody DoSomething, tylko po prostu bym ją użył. Tym razem coś mnie podkusiło. Wspomniana metoda wyglądała w następujący sposób:
public void DoSomething(IReadOnlyFoo obj)
{
((dynamic)obj).SomeValue = 10;
}
Hmmmm….. Lekkie zdziwienie. W języku z silną kontrolą typów, w sygnaturze metody twierdzę, że nic nie będę robił z obiektem, a w samej metodzie dobieram się do obiektu w bardzo brutalny sposób, który jedyne co gwarantuje to fakt, że kompilator do tego kodu się nie przyczepi. Dodatkowo istnieje duże ryzyko, że jakikolwiek refaktoring interfejsu IReadOnlyFoo spowoduje błąd w naszej aplikacji, który ujawni się dopiero w momencie działania programu.
Co można z tym zrobić… Zastanawiam się, czy Microsoft nie mógłby wprowadzić w nowszej wersji języka mechanizmu, który pozwalałby zdefiniować słowa kluczowe, których nie powinno się używać. Każde użycie takiego słowa powinno wymuszać akceptację tego kodu przez pozostałych członków zespołu.
A może macie jakiś inny pomysł na radzenie sobie z tym problemem?
W idealnym przypadku taki kod powinien zostać wychwycony podczas code review przez innego/innych programistów. Prawie na pewno ten kod nie musiał powstać, a wartość ’10’ być może udałoby się ustawić gdzieś wyżej podczas tworzenia obiektu IReadOnlyFoo, a jeśli jego konstrukcja lub metody (prywatne) na to nie pozwalają to jest coś nie tak w samym design skoro widać że istnieje potrzeba wykonania takiej operacji. Ale skoro nie było czasu na code review także pewnie i programistę czas naglił, że takie coś stworzyć raczył 🙂 Co do proponowanych zmian to nie jest to kwestia zmian w samym języku (specyfikacji), ewentualnie kompilatora czy innych narzędzi. Np w jednej z przyszłych wersji C# lub RoslynCTP (obecnie nie ma jeszcze obsługi dynamic ale być ten konkretny przypadek da się zrobić inaczej) będzie można napisać coś co wyrzuci warning w takim przypadku (error chyba się nie da), a jeśli sam warning nie odstrasza wystarczająco to build process powinien zablokować taki commit. Nie wiem czy jakieś istniejące narzędzia do statycznej analizy kodu już czegoś podobnego nie umożliwiają. Co do samych dynamic to oprócz celów do jakich zostały stworzone są nader często wykorzystywane właśnie przy różnego typu obejściach i wcale nie musi to być złe. Np ostatnio w projekcie w przypadku dość dużego zasobu zaszłego kodu, w którym performance nie jest priorytetem a reflection jest na porządku dziennym zdecydowałem się na użycie dynamic w parametrze metody. Inaczej musiałbym zryć architekturę kilkoma dodatkowymi interfejsami wprowadzonymi tylko w tym celu. Ale nawet i tego nie mogłem zrobić bo na tyle to mąciło design że wprowadzało circular reference. Użycie dynamic w tym przypadku dodatkowo zabezpieczone zostało kilkoma unit testami. Natomiast workarround z Twojego przypadku na który się natknąłeś jest rzeczywiście przegięciem:)
jakby nie było dynamic, to pewnie kreatywny programista wykorzystał by reflection. A samo dynamic jest z powodzeniem wykorzystywane w ASP do np. ViewBag.
Jest jeszcze jedna opcja -- rzutowanie. Może być ono zrobione na inną klasę lub interfejs. Taka konstrukcja też będzie dziwna, ale przynajmniej będzie zapewniona kontrola typów.
The code you have provided shows that:
1. Code was not tested
2. Code was not covered with unit tests
3. There was no code review by a more experienced person
Dynamic keyword is a powerful tool but you have to know how and when to use it.
The example you provided isn’t caused by the presence of that keyword.
You can write many stupid things that will break only at run-time even with old-school .NET
Language isn’t a problem here. People are the problem.
The people – I completely agree that this is the main reason of this issue.
All three reasons that you have pointed are true for the project from which is this example.
I think that most important solution for finding that strange constructions is code review. And in many times there is no budget for such activities…