|
Adding attributes to class violates SRP. Data class is responsible of containing data. It is not responsible of telling that I am serializable, I have these validation rules, I have these members with these English field names and I can be used in WCF, this field should not be serialized in JSON, etc. Data class should contain only data. All other things should be placed in other classes. Well, it's easier to say than do in practice because frameworks require you to do things in their way. Sadly.
Microsoft has done many mistakes in .NET design. Many places violates modern principles.
We have had to add [Serializable] into classes because some of them are added to HttpSession (stored in SQL Server). We need to tell that yes, we have thought that this is serializable, and let the default serializer do the job. This is just adding an attribute to class. Why it cannot work without an attribute? If this class is in external library (we have no source code to edit it), we are in trouble.
I am dealing with multilingual systems all the time therefore attribute based techniques with literal text (in English) cannot not be used. Anyway it violates the SRP.
Validation using Data Annotations require attributes. Now you add those attributes to every class. SRP violation. Then you realize that you need some rules that cannot be done with attributes. You decide to do it properly and replace your Data Annotation validation class with FluentValidation and write rules in their own classes. Once you have removed validation rules from data classes validation rules do not violate SRP.
WCF require DataContract and DataMember attributes. If you add those to your domain classes, you violate SRP. You need to violate this, but you should not touch domain classes. You should create separate classes for WCF and add attributes to them. That is keeping internal data structure and external data structure separate.
In practice we have to violate SRP sometimes. It's not our fault.
|
|
|
|
|
Thanks for your comment.
Yes, sometimes we need to violate SRP because we are using those technologies that force us to do so.
But I really hope people start to understand it to avoid such mistakes when creating those technologies (or even their own small and personal frameworks).
|
|
|
|
|
I am not going to post a whole book here. But your missing one main point that derails your entire argument.
Attributes ARE separate classes!
AND
Attributes DO NOT change or affect the class in any way (with the obvious possible exception of Aspect Oriented Programming)
If you notice, class attributes are OUTSIDE a class, they are attached to a class, not included as part of it. Following this logic, any attributes on a class are outside the scope of the class itself, therefore making it impossible for them to violate the SRP of the class.
-Kat
|
|
|
|
|
Attributes are separate classes. True.
Attributes do not change or affect the class in any way. False.
Attributes are outside a class, attached to it. False. Or maybe.
Creating an object of a class really does not need to read the attributes, but they aren't added at run-time. If you use a framework that requires an attribute (I used the [Serializable] as the example) you can't add such attribute to a type without changing the source code, so it is not "attached" considering the run-time. And, if the attribute is mandatory for something to work, it is violating SRP.
But aparently you didn't read the article. You saw the title and downvoted.
|
|
|
|
|
Attributes do not change or affect the class in any way. False. True
attributes can use the class that it is attached to, accessing public properties and so forth, but do not rewrite the class or effect the internal functions of the class.
Attributes are outside a class, attached to it. False. Or maybe. True
So lets take your [Serializable] example... Not having this attribute, does not in any way prevent the class from functioning as written. Anywhere you create the class you can use it and access its public members. Not having the SerializableAttribute on the class prevents A DIFFERENT class from functioning. Wait, doesn't that mean the serialization logic is in another class? Yes it does. And this other class is looking for this EXTERNAL tag to the class to find out if it can handle it? Yes.
This is no way violates the SRP.
Thank you,
Kat
|
|
|
|
|
You are right. Another class is not working. But the code of your class will need to change to include the attribute.
You may argue that it is not executable code. You are right. It is not. But it is code, written by a programmer and it should be in the same unit. It should be "part" of the class, even if you consider it to be attached to the class.
Look at the situation:
public sealed class MyClass
{
public string Name { get; set; }
}
Now, we attach a trait:
AttachTrait(typeof(MyClass), SerializableFlag.Instance);
In this case, MyClass does not need to change.
Now look at the [Serializable] attribute.
Can I say that MyClass is serializable without changing its source code? I am not talking about alternative serializers, so the only way is to go to the unit of that class and change its source code to be:
[Serializable]
public sealed class MyClass
{
public string Name { get; set; }
}
So we needed to change our code for a reason that is not related to the class itself. The attribute is another class? Yes. But the existence of such class is not enough, we need to "attach" that class to our class, but the only way to do it is at compile-time, not at run-time, so we need to change our code. The fact that it is not instruction code is irrelevant as we still need to change the source code.
In fact, I see that the problem here is related to the "attached" problem. To you, attributes are attached to the type, not part of the type, so there is no problem. But to be really attached there should be the support to add them later (and in this case putting them in the main unit will be only a shorthand), but that's not the case. Attributes, like any members, are part of the type description. They don't affect the instances directly, that's true, but any new static method that is never called will not affect any existing instance.
It is another class that will not work if you don't put the attribute, that's also true, but the SRP states that there need to be a single reason to change, and putting the attribute (or attaching the attribute) is a change and, as we can't add it at run-time or in another unit, we should change the source code of that unit.
Imagine that the "MyClass" is already compiled. You need to use it, but you don't have the source. Now you need to "attach" that attribute. Can you?
And that's why I said that attributes should be used as hints. I still use attributes a lot, but they are only one of the ways of telling something. I always let the same thing be told differently. That is, I allow things like Serializer.ConsiderSerializable(typeof(MyClass)) or Description.Set(typeof(MyClass), "This is a test class");
I will naturally put the [Description] attribute in my classes, but if I get a third-party type, it will work, because I don't make them the only way. If they are the only way, then you will need to change the source code of a type to "attach" things.
|
|
|
|
|
You kind of make my argument for me. If you do not know the source of the target class, i.e. someone else wrote it. How can you be sure that it is inherently serializable. That is why the attribute usage here makes since.
The Default Serializer has basically said, "Hey, class author, if your class is inherently serializable, let me know by attaching an instance of this class to your type.
If your not the class author, you cannot know that, and therefore should use other methods to serialize the object if absolutely necessary.
Even if the underlying class is inherently serializable, you should not assume that it will always be the case as the class author is obviously not assuming that as well. He/She could update the class later with something that the default serializer cannot handle and then your application just goes BOOM, whereas if you had used another method to serialize it yourself, you could apply the update most likely without issue.
All of this points specifically that an attribute on the class has absolutely nothing to do with the class' internal and proper function itself, and therefore has nothing to do with whether the class conforms to the SPR or not.
-Kat
|
|
|
|
|
The Serializable attribute is only one example (the description is another one much less problematic). But even in the serializable case, if you are the author of the class and you need to change the code of the class to include an attribute because there is no other way of doing it, then the attribute is violating SRP.
If you don't agree, OK. To you attributes aren't code. To me, they are. If I need to open a .cs to change anything in it, I consider I am changing code.
But you say that attributes have nothing to do with the class itself. And that's exactly why they violate SRP. They are written in the class, but they have nothing to do with the main purpose of the class. Or better, they usually don't have.
If I create a base class that expects some attribute on the child classes, it will be OK to put the attribute as it will be part of the responsibility of the inheriting classes. But if I am putting attributes in my classes to allow other code to work, then I am adding a responsibility.
The serializable attribute is a small responsibility (the author of the class must remember about it, or later he will have to change his code to add it). The entire serialization process is broken, IMO, because we can't say that a 3rd party class is serializable and we can't add a serializer either. But the focus is not on serialization. Is on attributes and code changes.
Any framework that expects an attribute on your code, to do something else with it, is forcing you to change existing classes, even if the change is small. This is a violation of SRP (you are changing code for another reason) and also of OCP (you are changing existing types instead of adding that in new types).
So, to me:
serializer.RegisterSerializer(typeof(MyClass), new MyClassSerializer());
Is OK. I don't know if MyClass has private data that should not be serialized, but I know how to recreate new instances without problems, so I add a new serializer. The Color type is an example. I know it has ARGB traits. How they are stored internally is not important, I can read the ARGB values write them to a byte stream and later I can read them and create a new Color instance. So, problem solved.
Description.Set(typeof(MyClass), "This is a sample class with a description");
It is OK too. The MyClass don't need such information.
But changing the MyClass source code to have this:
[Serializable]
[Description("The designer will not show any help if I don't put the description here"]
Means there is a change on existing code. OCP is violated (as my code was open for modifications). SRP is violated because such code means: I am the responsible of telling that I am serializable and also I am the responsible for giving the right description!
|
|
|
|
|
Herein lies the flaw, a cs file is not a class. (Now I do keep single classes in individual cs files myself, except for rare cases)
Adding an attribute to a class is NOT editing the class its editing the cs file Hell, you could add the attribute in a separate file with a partial class declaration if you REALLY want to, but I wont be doing that.
I'm going to say we must agree to disagree here.
|
|
|
|
|
You can add attributes in another file with partial classes. True.
But the attribute is still part of the class.
We can really discuss where is the limit. In fact, any non-virtual instance method that lives in a class is the same as an static method that lives in the class and receives as first parameter the instance to work on. It will be exactly the same IL code to do the call. Yet we consider that static methods are different than instance methods (and the fact that in C# we call them differently enforces that).
To me that's the same problem with attributes. They are not executable code, so you see them as different. But the problem is that you need to modify the definition of the class. Be it a partial class or not, you modify the class itself to have the attribute. In fact, partial classes are a compiler view. The generated classes aren't split into two classes with the same name. Only a single class is generated.
And that's where all the problem lives. If you need to recompile your type so it can be used by something else, there is a violation on SRP. If you agree that attributes are code or not is not the problem. You need to recompile your type to add that attribute. If you get a compiled library, you can't simply add attributes to those types if the attributes are valid for your application.
|
|
|
|
|
Maybe I should expose the problem differently... more related to OCP than to SRP.
If I have a tested library that works, I will close the entire library for modifications. Modifications will only be done to correct bugs. Nothing else. If the modification is in a class, attribute or whatever, it is in the library, and the entire library is closed.
So, all the components work fine in their main purpose. But then someone wants to use that component with another library. Serialization is an example. All the types are serializable by their structure (they only use primitive types and strings, for example). Yet, the .NET BinaryFormatter will not be able to serialize it. My solution? I have a serialization framework that allows serializers to be added, but that's not the point either. Many people, in that particular situation, will reconsider opening the code to put [Serializable] in all their types. The reasons will usually be: It is a small modification... it is much better than having to do all the serialization by hand (surely, it is, but I never said serialization is bad, I only consider the .NET serialization flawed).
Then imagine other situations. The main application of the company has an "edit workflow" feature, which uses the [Description] attributes of the objects to show a good description. But, hey, your library was created without the idea of being used in such workflow, but your components are compatible with the workflow framework. There is only one thing missing. The descriptions. But again, instead of using a solution that's capable of searching for descriptions elsewhere (and maybe using the attribute as one of the ways) the framework requires the attributes. So you should open your library again and put all the descriptions in the code.
But, hey, people loved attributes, and they are using them for everything. They now are marking objects that can be stored in the database with [POCO] attributes. And again, all your types are OK with the structure, but they don't have such attribute. What you do? You change all the types again to put that [POCO] attribute.
Is that a bad practice? Surely, but the [Serializable] is an example of such bad practice.
So, what really happened?
You modified your code many times because of external factors. If those external factors could be really external (live inside another DLL, be in a text file or whatever) it would be much better.
Is the SRP violated? If you insist that attributes aren't part of the class, then it is not. But you should agree that OCP was violated.
But now look at how IL works. Look at how attributes are get. You don't get all attributes from the assembly and then use a property to know where they were applied. You must enter the type (or any of its members) to look for the attributes. That's because they are part of that type.
In fact, even a constant is part of a type. Surely that when compiling code that uses a constant the compiler "copies" the value directly, but the constant continue to exist on the type, which the sole purpose of letting future code to reference it. If you add a new constant to a type, I consider that you changed that type, even if all the references to that constant are replaced by its actual value.
|
|
|
|
|
Also, to compare what is code and what is not. Is adding a new private constructor changing the class?
I am pretty sure you don't consider constructors as items "attached" to a type.
So, let's use a constructor as example. Imagine that instead of doing:
[Serializable]
public sealed class MyClass
{
public string Name { get; set; }
}
You did:
public sealed class MyClass
{
public MyClass(){}
private MyClass(Serializable ignoredParameter){}
public string Name { get; set; }
}
Let's consider that attributes don't exist, but reflection do. So, instead of searching for an attribute, the framework will search for a private constructor that receives a parameter of type Serializable. But such Serializable type is a class that can never be instantiated.
What I did with that was mark the class with an "I am serializable", but using a constructor that will never be invoked instead of using an attribute. Is a new constructor that is never invoked changing the behavior of the class? No.
Is the existence of such constructor making the code less maintenable? No. Or, maybe. You before used the example: What if I mark a class as serializable but, in the future, the class changes and has a field that can't be serialized?
Well, that means that by having the [Serializable] attribute, if you decide or need to add a field you should also take into account if the type will continue to be serializable. So, such attribute is introducing a difficulty to maintain the code. Or should I say such constructor, as I used the constructor as such flag?
So, why is a constructor that's never invoked different from an attribute? If I need to put the attribute on the type and recompile it, it is the same as adding a private constructor or property that will never be invoked. It will be there only for reflection purposes, to help some unrelated code, but yet it will be part of my type. And if I am required to have such attribute, private constructor or private property that I will never invoke, then I think that my code has more responsibilities than it should. The code that is executed may continue to work correctly with or without those extra info, but when something else does not work, it is my code that is changing.
|
|
|
|
|
I've read through the thread here and it seems like you haven't written enough software. I'm not talking about dragging and dropping boxes onto a web page, I'm talking slogging through hundreds of classes and their code, with nothing visual going on. I'm also amused by the comments that some of these comments are "academic" and therefore not important. Well, that "academic" stuff was developed to prevent real problems in software development. So, if you think it's "academic" that's because the concept works and you've never encountered the problem it was designed to solve.
You are misunderstanding the single responsibility principle. That is a reason for concern. There is nothing in the principle itself which defines the scope of an object's "responsibility" and gives us clues about where to draw the line, but common sense and experience can provide that.
For example, you're confused why some things are members of the numeric type classes, and other things are shuffled into utility classes like "Math" - common sense is the primary reason, but the other reason is because we can define our single responsibility any way we want. The designers chose to define that responsibility in the case of the Integer class to be "anything having to do with Integers" and they didn't put trig functions in that group because trig functions are not about Integers - but basic addition and subtraction ARE. If I choose to define it that way, I can do that.
You would also do yourself a favor by looking up the history of problems behind serializing objects - it has almost always been understood that the common sense way of doing that is to have objects responsible for their own serialization. You should just be happy you don't have to write your own serializers and unserializers - cuz that was fun, dude, lol
So to boil that down... I think you're missing the point because you lack experience and historical perspective.
|
|
|
|
|
It seems that this message was lost here for some time.
But, I did write lots of software. If my examples maybe gave you the wrong idea that doesn't mean I don't understand SRP, Serialization or the responsibility of base types like int. I was using examples that fit my description, but I may forgot to explain every detail.
So, about the Math situation. In the first .Net version we had the primitive types (some other numeric types) and the Math static class, with methods like Abs, Min, Max etc.
But now there is the BigInteger type, and for such type, methods like Abs, Min, Max etc are inside the type itself. The Math class doesn't have methods to work with the BigInteger and, in fact, the creation of the BigInteger type should not affect the Math type.
So, do we have a problem with the single responsibility here?
Well, we have a kind of problem. The BigInteger works differently from the other types. It was created later, but that's no excuse. If all numeric types considered that it was their own responsibility to have Abs, Min, Max etc, that wouldn't happen. But we could also have many different classes (Int32Math, Int64Math) only to avoid putting all of the methods into the same class.
The problem with where starts and where ends the responsibility here is: What will happen if new types are added?
If we consider that methods like Min and Max aren't the responsibility of the primitive type, then it's OK, but at the same type, why is the responsibility of the Math type to deal with Int32, Int64 and others and not with BigInteger?
This problem is even worse if we consider that Min and Max can simple be generic using IComparable<T>. So, Math having Min and Max will be valid, but as a single generic method. The problem of Math having methods for many types is that, someday, a new type will appear and Math can't deal with it. So, Math has too many responsibilities.
If Min and Max were responsibilities of the numeric types, we will have a better rule... but maybe it will be still too many methods to implement to make a numeric-type work.
So, even if I can agree that simple math is the responsibility of the int type and Abs, Min and Max it not, I can't agree that Math has to deal with many types, but not all. Something is broken. If we had many "Math" types it could look ugly, but it will conform better to the principle... so, there is the right and wrong even when we define a responsibility.
About serialization, I never said that it should not exist a default serialization. But if the default does not work, because you have an external type that is not serializable (and it is inside a big graph), what do you do?
If the framework is smart enough, it could find external serializers. If it is not, you have to implement the ISerializable in a type that contains such item, or you need to create an wrapper object... but you are changing the serialized item itself, not the way the serializer finds the serialization algorithm. But if you don't agree with me, look at Silverlight, which simple does not have a [Serializable] attribute.
|
|
|
|
|
Been so long I forgot what the hell I was talking about but I don't think I meant you.
|
|
|
|
|
Hehehe... ok then. I didn't get a message that this message was posted... maybe it is simple misplaced.
|
|
|
|
|
Software development is not about getting the code comply to academia principle that someone decided to be convenient for his/here imaginary project. It is about getting products out of the door in a cheapest, fastest and most errorless way. Recommendation here is to do it inconvenient and an inefficient way for a sake of following the letter of a principle.
|
|
|
|
|
Or to allow things to work even when the attribute is not used... as already said many times, I still use attributes, but they are only one way of doing something, no the only way.
|
|
|
|
|
The problem with software is that it has no resource limitations. When it comes to design, the software guys have their own endless minds. More than often those minds produce series of BLOODY, over-complicated decisions. Your article is a part of the bloody chain of painful over-complication.
Attributes breaks single responsibility? Hell, no. Who, except the type, better knows about its serializability? Who, if not type itself, can give better textual description? Type is responsible for itself. This is an excellent situation. As you can see, this depends on a point of view.
Abstractions are often evil. You know why? Because a lot of people consider them as Idols. That's why I see every single day how some schoolboy builds yet-another-one MVVM framework just to "deliver" the "elegant" and "the only rightful" way of closing WPF windows. Jeeeez. GOF should be exterminated for their Patterns book; Aleksandrescu should be burnt in fire for sick template-based madness in C++. Why? Because there is no benefit from their mediocre works. But there are drawbacks: uneducated minds consume mad ideas and idolize them for the sake of complexity without actually solving real problems.
So, I suggest you to take a responsibility and to think twice before publishing something. It should be excellent or nothing.
Sorry for my a bit aggressive attitude, but I had to say the truth.
A more relevant article would be entitled "Over-complication vs Getting Things Done".
modified 10-Nov-12 9:13am.
|
|
|
|
|
Try to serialize a LINQ expression. I can with my own framework, not with the BinarySerializer which uses the [Serializable] attribute. And as I said, I still use attributes, but they are a default, not a rule. Being forced to put an attribute on a class only to make it serializable, when I know how to serialize it, but it is not part of my code? That's the real problem and it is there when the SRP is violated, as you can't create a class without thinking about what other uses could be done.
|
|
|
|
|
Paulo, you are right and I agree with you on this matter. But it is a mistake to proclaim attributes as SRP violation. It is not. Some CLR/Core classes are not intended to be serializable by design. You try to overcome this limitation and this whole situation looks like SRP violation for you. This is just a war of interests: CLR Team vs You. That's why I strongly recommend to avoid generalization for such kind of conflicts. Kids read Code Project. Just imagine what impact all this can have on their young minds.
|
|
|
|
|
But attributes do violate SRP... in general attributes are there to give extra info on classes, and that extra info is rarely related to their main focus.
I agree that some types are simple not serializable... but others simple forget the attribute and you are simple stuck if you can't change their code. As I said, I still use attributes (in special DisplayName) to give the default naming, but I don't make them the rule. There is always another way to do things (in my frameworks). If there is not, then it is a real problem... and, considering the type was not created to have a DisplayName, adding it is a violation of SRP.
Or, as I prefer to say: Programmers don't need to care and to put things on their classes if it is not necessary for its main purpose. If someone else's find another use, it it is the responsibility of that someone else to add the missing code (and that includes attributes)... but, hey, there's the program. You can't add attributes. So, or you are stuck, or you make the responsibility of the first one to add the extra attributes.
And about children reading codeproject... well, if they understand what I am saying, it will be better!
|
|
|
|
|
I used to think that attributes were really cool, then I discovered the amount of baggage in terms of support that is necessary. For example, the number of attributes necessary for full designer support and serialization can be considerably longer than the code, and this in no way, IMO, should be entangled with the code. I have often thought that the whole concept would be better served with a dictionary that completely decouples the class, its methods and properties, from the secondary ways that the class will be used.
Marc
|
|
|
|
|
In fact that's what I usually do. Now I use attributes only as one of the ways to "tell" something (it is really easier to simple put an attribute over a member), but I don't search for an attribute... I search for an "extension" and the attribute is one of the ways of getting such extension.
But sometimes I really think that it will be even better to avoid attributes completely, but have easier ways to get fieldinfos and property infos (using expressions to get a propertyinfo is ugly and using reflection by name is problematic... I would love to use fieldinfoof(Type.field) or propertyinfoof(Type.Property).
|
|
|
|
|
Unfortunately getting rid of that baggage and implementing the same functionality typically takes more writing (=developers time) than using this "baggage" as is.
|
|
|
|
|