Is this an acceptable practice?
-
At a first glance, I find this confusing. In particular because the DataMember attribute actually accepts a constructor parameter. - So if I set the constructor parameter, which field does it apply to? - Likewise, if you want to switch out serializers and use something like Protocol Buffers this syntax simply won't translate. You would need to break out each one as the constructor is required. - On top of this all, there's a mixing of types. I'm not a big fan of the "multi-declaration" syntax, especially when it doesn't save any actual lines like this example. But there are some cases where it is useful. That stated, if you're mixing types on the "multi-declarations", it's time split them out. I wouldn't call this "unacceptable" practice, especially in the context of firing someone over this. I would however call this "non-standard" and "confusing" and ask that it be changed.
Gates VP wrote:
That stated, if you're mixing types on the "multi-declarations", it's time split them out.
They are clearly separated by lines, if they were in the same line, then I would agree with you. As I said in my reply, it resembles a lot C++ declaration like:
private:
int x;
double y;
.
.
.The underscore even act as C++ semi-colon. It doesn't hurt readability as you can clearly see the types at the end of each line. I don't see any confusion, except that someone may be in doubt if the attribute applies only to the first variable or all of them.
To alcohol! The cause of, and solution to, all of life's problems - Homer Simpson ---- Our heads are round so our thoughts can change direction - Francis Picabia
-
<System.Runtime.Serialization.DataMember>
Private mLastUpdated As DateTime, _
mLastUpdatedBy As String, _
mClearanceRequired As Int64, _
mClearanceIsRequired As BooleanVisual basic code; class members for serialization - declaring a group of variables as data members.
-
Refrain for abusive comments
Refrain from abusive code.
Did you ever see history portrayed as an old man with a wise brow and pulseless heart, waging all things in the balance of reason? Is not rather the genius of history like an eternal, imploring maiden, full of fire, with a burning heart and flaming soul, humanly warm and humanly beautiful? --Zachris Topelius Training a telescope on one’s own belly button will only reveal lint. You like that? You go right on staring at it. I prefer looking at galaxies. -- Sarah Hoyt
-
I would say it is not acceptable because your intent is not clear, which makes the code difficult to maintain.
The comment above the code in a must inherit class Authorization data members: If Clearance Is Required the user attempting to change the value will have their clearance level validated against the clearance required. Collects who and when this class was last updated by - for data merging such as offline files.
-
Gates VP wrote:
That stated, if you're mixing types on the "multi-declarations", it's time split them out.
They are clearly separated by lines, if they were in the same line, then I would agree with you. As I said in my reply, it resembles a lot C++ declaration like:
private:
int x;
double y;
.
.
.The underscore even act as C++ semi-colon. It doesn't hurt readability as you can clearly see the types at the end of each line. I don't see any confusion, except that someone may be in doubt if the attribute applies only to the first variable or all of them.
To alcohol! The cause of, and solution to, all of life's problems - Homer Simpson ---- Our heads are round so our thoughts can change direction - Francis Picabia
If you have 35 or 40 of these grouped into one segment, they're going to flow off the screen. If you want to mark just one of them as "public", do you copy/paste this to some other part of the file entirely? How does this affect the attributes? Remember, after you copy/paste the private variable to the public section, the original attributes are on a completely different screen. And you're not instinctively moving the attribute along with the variable.
Quote:
I don't see any confusion, except that someone may be in doubt if the attribute applies only to the first variable or all of them.
That's still confusion. And it's not really useful or purposeful confusion. You save a small number of characters and compile to the exact same IL code (hopefully) while introducing uncertainty for developers. This is the equivalent of doing C++ / C# if blocks without braces. You come back to this problem that actually caused real production issues on iOs: https://www.imperialviolet.org/2014/02/22/applebug.html[^]
-
As a general rule, explicit intent is better coding practice than implied intent.
I rather have white space, but agreed.
-
Gates VP wrote:
That stated, if you're mixing types on the "multi-declarations", it's time split them out.
They are clearly separated by lines, if they were in the same line, then I would agree with you. As I said in my reply, it resembles a lot C++ declaration like:
private:
int x;
double y;
.
.
.The underscore even act as C++ semi-colon. It doesn't hurt readability as you can clearly see the types at the end of each line. I don't see any confusion, except that someone may be in doubt if the attribute applies only to the first variable or all of them.
To alcohol! The cause of, and solution to, all of life's problems - Homer Simpson ---- Our heads are round so our thoughts can change direction - Francis Picabia
This is actually one line of code, but that is why I break it into multiple lines using the _ character to cheat a little bit, so I only have to write the serialization tag once.
-
If you have 35 or 40 of these grouped into one segment, they're going to flow off the screen. If you want to mark just one of them as "public", do you copy/paste this to some other part of the file entirely? How does this affect the attributes? Remember, after you copy/paste the private variable to the public section, the original attributes are on a completely different screen. And you're not instinctively moving the attribute along with the variable.
Quote:
I don't see any confusion, except that someone may be in doubt if the attribute applies only to the first variable or all of them.
That's still confusion. And it's not really useful or purposeful confusion. You save a small number of characters and compile to the exact same IL code (hopefully) while introducing uncertainty for developers. This is the equivalent of doing C++ / C# if blocks without braces. You come back to this problem that actually caused real production issues on iOs: https://www.imperialviolet.org/2014/02/22/applebug.html[^]
I write generics my classes are never more then 300 lines of code and if I need more then 4 or so data members I am doing something wrong Also I would never declare a member of a class public, I use properties to expose members. The only reason these are grouped together is so I do not have to write the serialization tag out for each, its cheating a bit.
-
This is what it looked like before
<System.Runtime.Serialization.DataMember>
Private mLastUpdated As DateTime<System.Runtime.Serialization.DataMember> Private mLastUpdatedBy As String <System.Runtime.Serialization.DataMember> Private mClearanceRequired As Int64 <System.Runtime.Serialization.DataMember> Private mClearanceIsRequired As BooleanThe first example I wrote, stands on in the document, where writing datamember so many times with the word private seems to be a waste of space.
So to clarify, you're saying that "saving space" is more important than "providing clarity" and "avoiding bugs"? If you're trying to save space why are you: 1. Using Private at all? That's the default, just take it out. 2. Using that little "m" in the prefix. Why not just lower-case the first letter? ("lastUpdated") 3. Using full words when you could abbreviate everything? ("lstUpd") 4. Not importing the System.Runtime.Serialization on the file? Then you could just type ""! If "saving space" is really important, why aren't you doing it everywhere? And at the end of the day, all of this code is functionally equivalent, right? So it all compiles to the same IL. I'm just not understanding the "save space" argument here.
-
So to clarify, you're saying that "saving space" is more important than "providing clarity" and "avoiding bugs"? If you're trying to save space why are you: 1. Using Private at all? That's the default, just take it out. 2. Using that little "m" in the prefix. Why not just lower-case the first letter? ("lastUpdated") 3. Using full words when you could abbreviate everything? ("lstUpd") 4. Not importing the System.Runtime.Serialization on the file? Then you could just type ""! If "saving space" is really important, why aren't you doing it everywhere? And at the end of the day, all of this code is functionally equivalent, right? So it all compiles to the same IL. I'm just not understanding the "save space" argument here.
The word Private indents the code for white space. The m stands for Member some companies use an underscore _ instead, each data member has a corresponding property that exposes the data member with extra code for validation, each of these properties implement an interface. The intent is white space and readability not saving space. Importing a class file is improper name resolution
-
The word Private indents the code for white space. The m stands for Member some companies use an underscore _ instead, each data member has a corresponding property that exposes the data member with extra code for validation, each of these properties implement an interface. The intent is white space and readability not saving space. Importing a class file is improper name resolution
Quote:
...datamember so many times with the word private seems to be a waste of space.
Your own words literally say that you are worried about "wasting space". Now you've changed it slightly:
Quote:
The intent is white space and readability not saving space.
You're worried about "wasting space" but only for some very specific and narrow scope of "wasting". And in this particular case, you're worried about "small waste" vs. "absolute clarity" which seems like a pretty lame trade-off. The fact that anyone had to question whether or not the attribute applied to all the variables means that you failed the "readability" test. Your goal of "readability" actually resulted in this code being hard to read and possibly being "unclear" to people who were not experts in this very narrow language feature.
-
White space. It's more important to people. Also as a programmer if you do not understand how a compiler is going to handle your code, you might consider another profession.
Yes, it's a matter of knowing how the compiler is going to handle the code, but it's more about maintainability. The format of the code is not to please the compiler. It's there to convey, without a doubt, what you intended. It's better to be explicitly expressive than it is to assume functionality with a lack of expression. Think about what the next guy who has to maintain your code is going to think when he sees it. Again, you're not looking for opinions. You're shooting down everything everyone says about what you wrote in an attempt to justify it.
A guide to posting questions on CodeProject
How to debug small programs
Dave Kreskowiak -
This is what it looked like before
<System.Runtime.Serialization.DataMember>
Private mLastUpdated As DateTime<System.Runtime.Serialization.DataMember> Private mLastUpdatedBy As String <System.Runtime.Serialization.DataMember> Private mClearanceRequired As Int64 <System.Runtime.Serialization.DataMember> Private mClearanceIsRequired As BooleanThe first example I wrote, stands on in the document, where writing datamember so many times with the word private seems to be a waste of space.
If you're worried about space, why are you using one of the most verbose languages there is? :)
A guide to posting questions on CodeProject
How to debug small programs
Dave Kreskowiak -
I see yes the context of how someone takes a sentence is important. The code is flawless, again the question was about coding practices.
When I commented on this branch of the thread, it wasn't addressed to you.
A guide to posting questions on CodeProject
How to debug small programs
Dave Kreskowiak -
If you're worried about space, why are you using one of the most verbose languages there is? :)
A guide to posting questions on CodeProject
How to debug small programs
Dave KreskowiakWhite space and readability It's like taking actual English practices and applying them to coding, so the point reading code is like reading a paragraph - when it's so elegant writing comments are redundant.
-
Quote:
...datamember so many times with the word private seems to be a waste of space.
Your own words literally say that you are worried about "wasting space". Now you've changed it slightly:
Quote:
The intent is white space and readability not saving space.
You're worried about "wasting space" but only for some very specific and narrow scope of "wasting". And in this particular case, you're worried about "small waste" vs. "absolute clarity" which seems like a pretty lame trade-off. The fact that anyone had to question whether or not the attribute applied to all the variables means that you failed the "readability" test. Your goal of "readability" actually resulted in this code being hard to read and possibly being "unclear" to people who were not experts in this very narrow language feature.
you are right I did say waste of space.
-
Yes, it's a matter of knowing how the compiler is going to handle the code, but it's more about maintainability. The format of the code is not to please the compiler. It's there to convey, without a doubt, what you intended. It's better to be explicitly expressive than it is to assume functionality with a lack of expression. Think about what the next guy who has to maintain your code is going to think when he sees it. Again, you're not looking for opinions. You're shooting down everything everyone says about what you wrote in an attempt to justify it.
A guide to posting questions on CodeProject
How to debug small programs
Dave KreskowiakYou are not reading every comment. Maintainability is a good reply.
-
Quote:
...datamember so many times with the word private seems to be a waste of space.
Your own words literally say that you are worried about "wasting space". Now you've changed it slightly:
Quote:
The intent is white space and readability not saving space.
You're worried about "wasting space" but only for some very specific and narrow scope of "wasting". And in this particular case, you're worried about "small waste" vs. "absolute clarity" which seems like a pretty lame trade-off. The fact that anyone had to question whether or not the attribute applied to all the variables means that you failed the "readability" test. Your goal of "readability" actually resulted in this code being hard to read and possibly being "unclear" to people who were not experts in this very narrow language feature.
The reason indeed why I posted the acceptability is to hear what you think. I am way out to left field my own practices are just for myself as I am the owner of my software company. This is the whole thing, I already know how off standard this is.
Namespace Sinks
<System.Runtime.Serialization.DataContract(IsReference:=True)>
Public MustInherit Class Base
Inherits Validate
Implements iInterface<System.Runtime.Serialization.DataMember> Private mLastUpdated As DateTime, \_ mLastUpdatedBy As String, \_ mClearanceRequired As Int64, \_ mClearanceIsRequired As Boolean Public Sub New() MyBase.New() End Sub Public Sub New(Clearance As Int64) MyBase.New() ClearanceRequired = Clearance End Sub Public Property LastUpdated As DateTime Implements iInterface.LastUpdated Get Return mLastUpdated End Get Set(value As DateTime) mLastUpdated = value End Set End Property Public Property LastUpdatedBy As String Implements iInterface.LastUpdatedBy Get Return mLastUpdatedBy End Get Set(value As String) mLastUpdatedBy = value End Set End Property Public Property ClearanceIsRequired As Boolean Implements iInterface.ClearanceIsRequired Get Return mClearanceIsRequired End Get Set(value As Boolean) mClearanceIsRequired = value End Set End Property Public Property ClearanceRequired As Long Implements iInterface.ClearanceRequired Get Return mClearanceRequired End Get Set(value As Long) mClearanceIsRequired = True mClearanceRequired = value End Set End Property Public Overloads ReadOnly Property Value(User As Directory.iUser) As Object Implements iInterface.Value Get Select Case True Case User.Deny : Throw New Exception("User Denied!") Case ClearanceIsRequired And User.ClearanceReadLevel < ClearanceRequired : Throw New Exception("User clearance is lower then required!") Case Else : Return MyBase.Value En -
I DO NOT NEED HELP Quit behaving like you know what the fook is going on. THE QUESTION IS - is this an acceptable practice, please learn to read English before getting on my case.
-
The reason indeed why I posted the acceptability is to hear what you think. I am way out to left field my own practices are just for myself as I am the owner of my software company. This is the whole thing, I already know how off standard this is.
Namespace Sinks
<System.Runtime.Serialization.DataContract(IsReference:=True)>
Public MustInherit Class Base
Inherits Validate
Implements iInterface<System.Runtime.Serialization.DataMember> Private mLastUpdated As DateTime, \_ mLastUpdatedBy As String, \_ mClearanceRequired As Int64, \_ mClearanceIsRequired As Boolean Public Sub New() MyBase.New() End Sub Public Sub New(Clearance As Int64) MyBase.New() ClearanceRequired = Clearance End Sub Public Property LastUpdated As DateTime Implements iInterface.LastUpdated Get Return mLastUpdated End Get Set(value As DateTime) mLastUpdated = value End Set End Property Public Property LastUpdatedBy As String Implements iInterface.LastUpdatedBy Get Return mLastUpdatedBy End Get Set(value As String) mLastUpdatedBy = value End Set End Property Public Property ClearanceIsRequired As Boolean Implements iInterface.ClearanceIsRequired Get Return mClearanceIsRequired End Get Set(value As Boolean) mClearanceIsRequired = value End Set End Property Public Property ClearanceRequired As Long Implements iInterface.ClearanceRequired Get Return mClearanceRequired End Get Set(value As Long) mClearanceIsRequired = True mClearanceRequired = value End Set End Property Public Overloads ReadOnly Property Value(User As Directory.iUser) As Object Implements iInterface.Value Get Select Case True Case User.Deny : Throw New Exception("User Denied!") Case ClearanceIsRequired And User.ClearanceReadLevel < ClearanceRequired : Throw New Exception("User clearance is lower then required!") Case Else : Return MyBase.Value EnI would prefer to write the attribute for each member, if you want to save lines of code you could write them inline.
<System.Runtime.Serialization.DataMember> Private mLastUpdated As DateTime
<System.Runtime.Serialization.DataMember> Private mLastUpdatedBy As String
<System.Runtime.Serialization.DataMember> Private mClearanceRequired As Int64
<System.Runtime.Serialization.DataMember> Private mClearanceIsRequired As BooleanAnd since you're saving lines of code, why could use auto-implemented properties por LastUpdated, LastUpdatedBy and ClearanceRequired.