Bad practices that have their roots back to VB - code review



  • A senior consultant was hired to perform some code review on some code I've written. Here is a part of the report:

    6. Additionally as mentioned in String Handling paragraph, relic paradigms deriving from non-OOP languages like Visual Basic should be strongly avoided in your code.

    Bad practices that have their roots back to VB can be found all around the project. The naming of the variables doesn’t follow certain and steady methodology and a lot of practices followed do not promote clean code which developers with C++ & Java background are accustomed to visually handle with.

    WfContactrequest contactRequest = new WfContactrequest
    {
    WfContactrequestId = Guid.NewGuid(),
    WfADLogin = cd.ContactID,
    WfFirstName = cd.FirstName,
    WfLastname = cd.LastName,
    WfEmailaddress1 = cd.Email,
    WfSalutation = cd.Salutation,
    WfJobTitle = cd.JobTitle,
    WfTelephone1 = cd.BusinessPhone,
    WfMobilephone = cd.MobilePhone,
    WfTelephone2 = cd.OtherPhone
    };

    With Blocks like the above, are strongly discouraged in OOP language. Treat class properties as imposed by OOP language semantics and not from relics come from other languages.

    cool, huh? WTF!?



  • What language is it?



  • c# 3.0



  •  ?

    Where's the With block?

    Where are the parens() after new WfContactrequest?

    Why is there a loose statement block there?

    Why do I know little about the peculiarities of C# syntax but am posting anyway?

    How is this even a sentence?:

    a lot of practices followed do not promote clean code which developers with C++ & Java background are accustomed to visually handle with.



  • "relics come from other languages."

    I am supposed to write some comments back for this guy. What do you advise? I am completely speechless.



  • @dhromed said:

    Where are the parens() after new WfContactrequest?

    Probably a typo of the original post.


    Why is there a loose statement block there?

    Why do I know little about the peculiarities of C# syntax but am posting anyway?

     



  • @Alx said:

    What do you advise?
     

    Let him take some courses on proper English.



  • @Alx said:

    "relics come from other languages."

    I am supposed to write some comments back for this guy. What do you advise? I am completely speechless.

     

    Calling new language features of C# 3.0 "relics come from other languages" proves that the consultant has no fucking clue.



  • @ammoQ said:

    http://weblogs.asp.net/dwahlin/archive/2007/09/09/c-3-0-features-object-initializers.aspx
     

    Excellent. Thanks. Good feature, that. I use its equivalent all the time in JS when I have a function with 3+ arguments most of which are optional: it's more explicit to the code reader, and more flexible => more better good.

    "Why do I have to define a type name twice when creating an object?"

    So what's the answer? This is a pet peeve of mine. I don't want to type that much, nor do I want to navigate Intellisense that much. You get things like

    Potato potato = new Potato()

    And where's the benefit in that? It looks like line noise.

     

     



  • @dhromed said:

    Excellent. Thanks. Good feature, that. I use its equivalent all the time in JS when I have a function with 3+ arguments most of which are optional: it's more explicit to the code reader, and more flexible => more better good.
     

    ExtJS , a framework for making GUI-like web applications in JavaScript, uses parameter objects everywhere.



  •  Be afraid - Management will likely assume that this expensive Senior Consultant that they have hired is a complete guru and will not listen to your arguments that he is an idiot.

    Be very afraid - He may be angling for a full-time job.

    Run for your life - He may be related to somebody high-up.



  • var SomeVariable = new SomeClass(); // Let the compiler figure out what var should resolve to. even works for intellisense.

    c# has had a ton of improvements <3



  • Just respond with a link to this forum post :)



  • @Alx said:

    cool, huh? WTF!?
    Is he referring to the fact that you're defining the parameters inside the function call?  I've seen that in a lot of languages, and I wouldn't expect most VB coders to think of it.

    Maybe it's your CamelCase?



  • @Alx said:

    WfContactrequest contactRequest = new WfContactrequest

    {

    WfContactrequestId = Guid.NewGuid(),

    WfADLogin = cd.ContactID,

    WfFirstName = cd.FirstName,

    WfLastname = cd.LastName,

    WfEmailaddress1 = cd.Email,

    WfSalutation = cd.Salutation,

    WfJobTitle = cd.JobTitle,

    WfTelephone1 = cd.BusinessPhone,

    WfMobilephone = cd.MobilePhone,

    WfTelephone2 = cd.OtherPhone

    };

    I know nothing about C#, but let me take a guess here. You're making a new.. instance of the contactRequest class(?) and then setting all the internal properties from some other class called "cd". A little wordy for a simple action, but it is what it is.

    [i] and not from relics come from other languages. [/i]
    If the guru can't even use English correctly....

    My suggestion: Call him. A phone conversation can do alot to clear up simple misunderstandings.



  • There are major differences between using constructor parameters and object initializers. If you do not fully understand the differences, please durn off your computer NOW.



  • Here is the response I would give:

     

    @Alx said:

    6. Additionally as mentioned in String Handling paragraph, relic paradigms deriving from non-OOP languages like Visual Basic should be strongly avoided in your code.

    I appreciate your opinion in what should and shouldn't be avoided, but please allow me to remind you C# has a language has only expanded from these so called "relic" paragidms. Lambda expressions in C# are a direct result of the "relic" paradigm called Functional Programming. Also C# 4.0's largest feature is a direct pull from another "relic" paradigm called Dynamically Typed Programming. While Microsoft has had and will continue to have its issues, I suspect the people who create the very language of the code you are reviewing to have a better understanding of how the language is best used.

    @Alx said:


    Bad practices that have their roots back to VB can be found all around the project. The naming of the variables doesn’t follow certain and steady methodology and a lot of practices followed do not promote clean code which developers with C++ & Java background are accustomed to visually handle with.

    If a C++ or Java developer cannot within a reasonable time discern what the code is saying, they do not have enough knowledge of C# to be working on this code and should direct it to the attention of someone who is more familiar with the language. The naming conventions applied are the same ones used by default from the IDE and are a convention for C# that Microsoft has created. While this convention is not set in stone and there are many possibilities with naming conventions, it should be understood that naming conventions are usually kept at an organization level meaning each company tends to have their own variation of a naming convention. As long as all the code within this organization follows this naming convention, an issue should not arise.

    @Alx said:

     

    With Blocks like the above, are strongly discouraged in OOP language. Treat class properties as imposed by OOP language semantics and not from relics come from other languages.

     

    I agree that "With Blocks" are not among best practices in development but I fail to see where a "With Block" is being used here, especially since C# does not support this functionality. What I see above is called an "Object Initializer", a new feature to C# 3.0. It allows properties to be cleanly set in an objects creation without creating unnecessary constructors and causing a maintenance headache. Following your guidance though, properties should not even be used and should be replaced with a private variable with setter and getter methods. This would make code much easier to follow for those who come from a Java/C++ background, correct?

     

    This guy is a boob and your boss shouldn't be paying him to review your code.



  • @ProggyBS said:

    This guy is a boob and your boss shouldn't be paying him to review your code.

    Sums it up quite nicely.



    Also, VB is an OOP language.



  • @TheCPUWizard said:

    There are major differences between using constructor parameters and object initializers. If you do not fully understand the differences, please durn off your computer NOW.

    Not knowing the subtleties of a newly-added feature in a language most people don't use?  That's a durning.



  • @morbiuswilters said:

    @TheCPUWizard said:

    There are major differences between using constructor parameters and object initializers. If you do not fully understand the differences, please durn off your computer NOW.

    Not knowing the subtleties of a newly-added feature in a language most people don't use?  That's a durning.

     

    Not at all. If a person is going to use a language in a professional environment, then they should have full knowledge of all of the features (unless they are an intern). If the person is going to be performing reviews, then the person should be able to refer back to the language specifications by paragraph as required before making any comments.

     



  • @dhromed said:

    "Why do I have to define a type name twice when creating an object?"

    So what's the answer? This is a pet peeve of mine. I don't want to type that much, nor do I want to navigate Intellisense that much. You get things like

    Potato potato = new Potato()

    And where's the benefit in that? It looks like line noise.

    The answer is simple — use VB!

    [code]Dim potato As New Potato[/code]



  • @morbiuswilters said:

    @TheCPUWizard said:

    There are major differences between using constructor parameters and object initializers. If you do not fully understand the differences, please durn off your computer NOW.

    Not knowing the subtleties of a newly-added feature in a language most people don't use?  That's a durning.

     

    Most people don't use C#?  Hmm, I must have missed the memo.

    Anyway, 3 years ago is not exactly new, and if the guy's doing code reviews then he should probably be held to a higher standard.



  • @Aaron said:

    Most people don't use C#?  Hmm, I must have missed the memo.

    So you're assuming that a majority of programmers use C#?  That seems unlikely.

     

    @Aaron said:

    Anyway, 3 years ago is not exactly new, and if the guy's doing code reviews then he should probably be held to a higher standard.

    Right, but TheCPUWizard didn't say that.  His statement was "If you do not fully understand the differences, please durn off your computer NOW."  He probably meant what you're saying, but his phrasing was broad and riddled with typeos.



  • @morbiuswilters said:

    @Aaron said:

    Most people don't use C#?  Hmm, I must have missed the memo.

    So you're assuming that a majority of programmers use C#?  That seems unlikely.

    Technically, I doubt that you'd be able to say that a majority of programmers use any one language.  It's not like I have statistics on file for this, but on any given programming site, Java tends to be the most well-represented (although that's changing), with .NET (and in particular C#) being the next in line.  PHP is probably third with C++ being next, and after that it's a long tail.

    So I doubt that the market share of any single language is more than 20-30%, and therefore you can't say that "most people use it".  But as far as popularity goes, C# certainly ranks among the highest.

     

    @Aaron said:
    Anyway, 3 years ago is not exactly new, and if the guy's doing code reviews then he should probably be held to a higher standard.

    Right, but TheCPUWizard didn't say that.  His statement was "If you do not fully understand the differences, please durn off your computer NOW."  He probably meant what you're saying, but his phrasing was broad and riddled with typeos.

     

    You mean typoes?



  • @Daid said:

    Also, VB is an OOP language.

    To make your make your head spin a bit more, VB and C# are now produced by the same team! The April 2010 edition of MSDN magazine has an article on page 24 "What's new in Visual Basic 2010". The first section of this is titled "Coevolution" where they talk about how in the past that VB and C# kept leap frogging one another with features. So the solution is:

    ... Microsoft merged the Visual Basic and C# teams, embracing a strategy of coevolution. The intent is to make the languages advance together. When major functionality is introduced in one language, it should appear in the other as well.

  • Discourse touched me in a no-no place

    @Aaron said:

    It's not like I have statistics on file for this
    TIOBE[1] got a mention on Slashdot the other day.



    It has C and Java roughly equal at 18% with C++ and PHP, again, equal, at around 9.7%.




    [1]:
    Tiobe programming community index is an ordered list of programming languages, sorted by the frequency of web search using the name of [the] language as keyword.



  • @OzPeter said:

    [, VB and C# are now produced by the same team!

     WRONG. Still distinct teams just working along the same lines for MAJOR FUNCTIONALLITY. There will continue to remain features which are only available in a given language when that is more appropriate.



  • @ammoQ said:

    @dhromed said:

    Where are the parens() after new WfContactrequest?

    Probably a typo of the original post.

    Not a typo, in C# 3 when using the object initializer syntax you can leave them out if you are calling the default constructor.



  • @PJH said:

    @Aaron said:
    It's not like I have statistics on file for this
    TIOBE[1] got a mention on Slashdot the other day.

    It has C and Java roughly equal at 18% with C++ and PHP, again, equal, at around 9.7%.


    [1]:
    Tiobe programming community index is an ordered list of programming languages, sorted by the frequency of web search using the name of [the] language as keyword.
     

    Seems kind of arbitrary to me:

    The ratings are calculated by counting hits of the most popular search engines. The search query that is used is

    +"<language> programming"

    This seems to heavily favour languages that have been around longer.  I've got nothing against python, but you are not going to try to tell me that there are as many python programmers as .NET programmers.  No way in hell.



  • @Aaron said:

    Technically, I doubt that you'd be able to say that a majority of programmers use any one language.  It's not like I have statistics on file for this, but on any given programming site, Java tends to be the most well-represented (although that's changing), with .NET (and in particular C#) being the next in line.  PHP is probably third with C++ being next, and after that it's a long tail.

    So I doubt that the market share of any single language is more than 20-30%, and therefore you can't say that "most people use it".  But as far as popularity goes, C# certainly ranks among the highest.

    So, in other words, C# is a language most people don't use.  I never said it wasn't popular.

     

    @Aaron said:

    You mean typoes?

    "Typeos" is a meme that originated in #TDWTF IRC.



  • @Aaron said:

    @PJH said:

    @Aaron said:
    It's not like I have statistics on file for this
    TIOBE[1] got a mention on Slashdot the other day.

    It has C and Java roughly equal at 18% with C++ and PHP, again, equal, at around 9.7%.


    [1]:
    Tiobe programming community index is an ordered list of programming languages, sorted by the frequency of web search using the name of [the] language as keyword.
     

    Seems kind of arbitrary to me:

    The ratings are calculated by counting hits of the most popular search engines. The search query that is used is

    +"<language> programming"

    This seems to heavily favour languages that have been around longer.  I've got nothing against python, but you are not going to try to tell me that there are as many python programmers as .NET programmers.  No way in hell.

    It also skews more heavily towards FOSS languages*.  Java and C# probably have more people learning through books, jobs or coursework than Python and Ruby which most people probably learn over the web.

     

    * As in, languages commonly associated with FOSS.



  • @morbiuswilters said:

     

    @Aaron said:

    You mean typoes?

    "Typeos" is a meme that originated in #TDWTF IRC.

    And they need more slat.


  • @TheCPUWizard said:

    @OzPeter said:

    [, VB and C# are now produced by the same team!

     WRONG. Still distinct teams just working along the same lines for MAJOR FUNCTIONALLITY.

    Citation please ... I had one for my post so its only fair you give one too


  • @Aaron said:

    Technically, I doubt that you'd be able to say that a majority of programmers use any one language.  It's not like I have statistics on file for this, but on any given programming site, Java tends to be the most well-represented (although that's changing), with .NET (and in particular C#) being the next in line.  PHP is probably third with C++ being next, and after that it's a long tail.
     

    Depending on how you define "programmer", I bet VBA (sadly) is near the top.



  • @Daid said:

    Also, VB is an OOP language.
     

    VB .NET is OOP, VB was not.

    If his only concrete examples are your variable names, your boss would be foolish to listen to him (although that was just a part of his report).



  • I fail to see how using a with block or an object inicializer could make a code more or less object oriented, when it's a coding detail that it's not visible from outside the methods that use them

     I wonder why every property is prefixed with Wf, that looks very hungarian to me...

    VB6 programmers in my area would prefix every variable of a non primitive type with obj (objPerson), any colections with col or even objCol (objColPersons) and every parameter with x (xName)



  • @monkeypants said:

    @Daid said:

    Also, VB is an OOP language.
     

    VB .NET is OOP, VB was not.

    If his only concrete examples are your variable names, your boss would be foolish to listen to him (although that was just a part of his report).

    VB.Net simply exhibits more OO paradigms than VB6.  VB6 has classes, interfaces, properties, and methods. VB.Net adds inheritance and more sophisticated instance construction.  Whether either is Object Oriented depends on your definition.  Under a liberal definition, both are OO, under a strict definition, neither are.  Only if you draw a line in the sand at inheritance does your statement hold true.  In 2005, James Gosling said that the one thing he would change if he were to start Java over, would be to not have inheritance and only allow interface implementation.  That would make Java's ideal rewrite very similar in object orientedness to VB6.

    As for the consultant's comments on variable names, unless the code is littered with short cryptic names, the consultant was running out of things to write or is talking out his ass.  My personal opinion is that there is almost no code in the professional world that is "big issue free" enough to start spending time renaming variables.  If rule #1 in your coding conventions is a variable naming standard, fire the guy who wrote it and burn it.

    I'm also curious to know what his reference to string handling was.  It almost sounds like he wants you to rewrite

    greeting = "Good " + timeOfDay + " " + userName;

    as

    StringBuilder greetingBuilder = new StringBuilder();
    greetingBuilder.Append("Good ");
    greetingBuilder.Append(timeOfDay);
    greetingBuilder.Append(" ");
    greetingBuilder.Append(userName);
    greetingBuilder = greetingBuilder.ToString();

    The second will compile to identical object code and is harder to read.  StringBuilders are great in loops, but are pointless for simple string concatenation.



  • @Jaime said:

    In 2005, James Gosling said that the one thing he would change if he were to start Java over, would be to not have inheritance and only allow interface implementation.

    This is like Bull Connor saying the only thing he'd do differently is filling the firehoses with crisp, thirst-quenching spring water.



  •  @morbiuswilters said:

    This is like Bull Connor saying the only thing he'd do differently is filling the firehoses with crisp, thirst-quenching spring water.

    Now I'm going to get thirsty every time I hear about civil rights. Thanks Morbius.



  •  Also, I'm pretty sure Microsoft referred to VB as event-driven instead of truly object-oriented, specifically because it lacked inheritance. I have no preference either way; VB was reasonably easy to learn, and you can do stuff with it.

    But I'm technically correct (the best kind of correct).


  • :belt_onion:

    @monkeypants said:

     Also, I'm pretty sure Microsoft referred to VB as event-driven instead of truly object-oriented, specifically because it lacked inheritance. I have no preference either way; VB was reasonably easy to learn, and you can do stuff with it.

    But I'm technically correct (the best kind of correct).

    I always thought of VB6 as being "object based" instead of "object oriented"


  • The funny thing though is that if I was so much out of ideas to complain about things, and decided to pick on these lines, there are a few valid (but not necessarily significant) problems with it that I can see :p



  • @ggf31416 said:

     I wonder why every property is prefixed with Wf, that looks very hungarian to me...

    The story of the Wf prefix is quite interesting...

    WfContactrequest contactRequest = new WfContactrequest
    {
    WfContactrequestId = Guid.NewGuid(),
    WfADLogin = cd.ContactID,
    WfFirstName = cd.FirstName,
    WfLastname = cd.LastName,
    WfEmailaddress1 = cd.Email,
    WfSalutation = cd.Salutation,
    WfJobTitle = cd.JobTitle,
    WfTelephone1 = cd.BusinessPhone,
    WfMobilephone = cd.MobilePhone,
    WfTelephone2 = cd.OtherPhone
    };

    The snippet above prepares some calls to Microsoft Dynamics CRM 4.0 web services, filling in the Contact Request entity. CRM itself has a different naming convention, having by default the "new_" prefix for every custom entity class name. According to this naming convention, you'll and up with a type name as "new_bankaccount", which contradicts the naming conventions for types in C#.

    The consultant who customized the CRM instance changed the default "new_" prefix into "Wf", after his name.

    Also, you can notice the "WfFirstName" and "WfLastname" consistency in his customization.

    You can find the naming conventions I adhere to on the right side of the equal signs.



  • @Alx said:

    The consultant who customized the CRM instance changed the default "new_" prefix into "Wf", after his name.

    Parents who name their child Wf are TRWF.



  • @monkeypants said:

    @Daid said:

    Also, VB is an OOP language.
     

    VB .NET is OOP, VB was not.


    Yes it is. VB6 has classes, objects. Basic needs for OOP, it can also do inheritance (a bit harder the most other OOP languages), and encapsulation. VB6 is just as much an OOP language as C++, PHP and Python. The main difference is that 90% of the projects and examples you see for VB6 don't make use of those features.



  •  

    come from

    Clearly, an INTERCAL expert.



  •  @dhromed said:

    Where are the parens() after new WfContactrequest?

     Redundant constructor call. Inspect your callstack if you disagree and watch how removing the parenthesis will vastly reduce the size of the call stack...



  • @bjolling said:

    I always thought of VB6 as being "object based" instead of "object oriented"
    Have you been reading any Navision documentation lately?



  • @dhromed said:

    Potato potato = new Potato()

    And where's the benefit in that? It looks like line noise.


    Something go you on thinking until one day you write...

    Potato potato = new MarisPiper()

    :)



  • @ThePants999 said:

    Something go you on thinking until one day you write...

    Potato potato = new MarisPiper()

    :)

     

    Yea, but then you get that shitty cast(e) system.


Log in to reply