Opinions required on 'code tidying'



  • This is my first pos, so bare with me :-)

     

    My boss and I are at a disagreement with the following code(I have not adjusted spacing/tabulation/newlines or anything)

     

    His code:

                         ((Information) e.Row.FindControl("IncompleteWarning")).State =
                            user.CompleteRecord
                                ? Information.ControlState.green_tick
                                : Information.ControlState.red_cross;

                        ((Information) e.Row.FindControl("IncompleteWarning")).ImageText =
                            user.CompleteRecord
                                ? AppSettings.GetResourceString(
                                      "CompleteUserProfile",
                                      ResourceType.
                                          GlobalString)
                                : AppSettings.
                                      GetResourceString(
                                      "IncompleteUserProfile",
                                      ResourceType.
                                          GlobalString);
     

     My version:

    Information information = (Information)e.Row.FindControl("IncompleteWarning");
    if (user.CompleteRecord)
    {
      information.State = Information.ControlState.green_tick;
      information.ImageText = AppSettings.GetResourceString("CompleteUserProfile", ResourceType.GlobalString);
    }
    else
    {
      information.State = Information.ControlState.red_cross;
      information.ImageText = AppSettings.GetResourceString("IncompleteUserProfile", ResourceType.GlobalString);
    }

     

    I am not averse to the use of the?: conditional operator but sometimes it can make things messy IMO and this mess seems to prove my point.

     And it's not just the use of the ternary operator, it's the general style - who would do that and think it's 'neat'?

    So, what do you guys think? Am I making a mountain out of molehill? Should I just not care about it? I like to write code that can be read by the simplest of souls as long as it doesn't affect performance that is.

     

    ...this isn't the _only_ example of WTFs in this codebase either.

     



  •  Even if you stick to the ternary operator, the second part should be refactored into

     

    ((Information) e.Row.FindControl("IncompleteWarning")).ImageText =
                            AppSettings.GetResourceString(
                                      user.CompleteRecord ? "CompleteUserProfile" : "IncompleteUserProfile",
                                      ResourceType.GlobalString);



  • Yes, I did see that but I wanted to 'test' my boss and ask him directly which he thought was neater. I think it's an ego problem...mine or his, you be the judge!



  • Also, "green_tick" and "red_cross" should be blackboxed behind more generic names like "complete_state" and "incomplete_state", in case the graphic design team changes their minds.




  • You'll probably get a much "better" answer on somewhere like Stack Overflow as asking questions about code is the whole point of that website



  •  I'd definately go with your code. The ternary operator is best used for short one-liners. Beyond that you're decreasing visibilty. And even if that wasn't an issue he's copy-pasting

    ((Information) e.Row.FindControl("IncompleteWarning")).ImageText = user.CompleteRecord<snip>
    all over the place. What is he going to do if he wants to add another change to it? Copy-paste it again?

    If there was only this on one line

    ((Information) e.Row.FindControl("IncompleteWarning")).State = user.CompleteRecord ? Information.ControlState.green_tick : Information.ControlState.red_cross;
    I'd understand. In fact this could be how it started originally. But at the point you are now it's time to refactor.



  • Thanks for the replies, you've made me feel a little better about getting (only slightly) annoyed with my boss.

    Oh well, learning how NOT to code is nearly as useful as learning best coding practises...at least that's what I've been telling myself these past 10 years!



  • I might personally prefer an intermediate approach:

    Information information = (Information) e.Row.FindControl("IncompleteWarning");
    information.State =
      (user.CompleteRecord ? Information.ControlState.green_tick : Information.ControlState.red_cross);
    String resourceKey =
      (user.CompleteRecord ? "CompleteUserProfile" : "IncompleteUserProfile");
    information.ImageText = AppSettings.GetResourceString(resourceKey, ResourceType.GlobalString);
    

    If there were more than two properties to set, I might go with an if statement as in your version (but preferably still keeping the common GetResourceString() call outside them). Actually, my favorite way of handling such situations in loosely typed languages would be something like this (using JavaScript as an example):

    var completeProps = {
      state : Information.ControlState.green_tick,
      reskey : "CompleteUserProfile",
      foobar : 42};
    var incompleteProps = {
      state : Information.ControlState.red_cross,
      reskey : "IncompleteUserProfile",
      foobar : 23};
    

    var props = (user.CompleteRecord ? completeProps : incompleteProps);
    var info = e.Row.FindControl("IncompleteWarning");

    info.State = props.state;
    info.ImageText = AppSettings.GetResourceString(props.reskey, ResourceType.GlobalString);
    info.FooBar = Quux.Xyzzy(props.foobar);

    The advantage here, besides being (IMHO) more readable and less redundant than either of the original versions, is that the data (the properties that differ between the two states) is cleanly separated from the code (applying those properties to the control). You could even read the properties from a config file if you wanted. In fact, that might be the cleanest way to do it, if you're working in a language that doesn't have such convenient syntax for declaring arbitrary associative arrays inline like that.



  • What language is it?

    But I prefer your version. In general, I try to avoid the use of the ternary operator as much as possible.



  • They both seem readable to me. If your manager actually asked you to change your style to his, then you have a perfect right to be annoyed. But if you want your manager to switch to your style, then you are definitely making a mountain out of a molehill. (Has anyone actually seen a molehill? Pictures anyone?)

     



  • @Rick said:

    They both seem readable to me. If your manager actually asked you to change your style to his, then you have a perfect right to be annoyed. But if you want your manager to switch to your style, then you are definitely making a mountain out of a molehill. (Has anyone actually seen a molehill? Pictures anyone?)

     

    He goes through my code and 'tidies' it up, this is just one example (I think he uses an automated tool to do changes like this). I understand that he is the boss and ultimatly it's up to him how the code looks - I don't have  to agree with it though. Or be happy about it.

     

     




  • @miner49er said:

    @Rick said:

    They both seem readable to me. If your manager actually asked you to change your style to his, then you have a perfect right to be annoyed. But if you want your manager to switch to your style, then you are definitely making a mountain out of a molehill. (Has anyone actually seen a molehill? Pictures anyone?)

     

    He goes through my code and 'tidies' it up, this is just one example (I think he uses an automated tool to do changes like this). I understand that he is the boss and ultimatly it's up to him how the code looks - I don't have  to agree with it though. Or be happy about it.

     

    In that case, it would bug the hell out of me. Are you familiar with the http://en.wikipedia.org/wiki/Peter_Principle?



  • your boss' version is wholly unreadable. you ought to gather all the developers and beat some sense in to him.



  • Your version is readable; his is much less so.

    @miner49er said:

    He goes through my code and 'tidies' it up, this is just one example (I think he uses an automated tool to do changes like this). I understand that he is the boss and ultimatly it's up to him how the code looks - I don't have  to agree with it though. Or be happy about it.
    What is his actual position: development lead, project manager, CEO?  And doesn't he have better things to do?  If this is really an issue, it sounds like your group needs some coding standards, rather than having him "clean up" after you.  Otherwise he's just micromanaging.  Your change logs must be a cluttered mess if he does this after every checkin.




  • This is actually a very good example of two functionally equivalent ways to do things with emphasis on different aspects of the problem.

    In the code from your boss, the emphasis is on the control property rather than the condition.  That is, for each of the properties you might want to do something (which in both cases depends on the user.CompleteRecord flag).

    Your code, however, places emphasis on "do something if user.CompleteRecord is true, and do something else is user.CompleteRecord is false".

    These are both valid ways to look at a problem, and I don't think there's actually a programming style that is easily flexible between the two different viewpoints.  I think at the end of the day you just have to be aware of the differences in emphasis.

    You might consider which is more likely to occur in the future: additional control properties (in which case emphasis on the properties makes sense) or if  you're going to add more action conditions (in which case emphasis on the conditions make sense).

    If you have a case where both the conditions and properties change, then I'd suggest picking one that is the most comfortable to all the maintainers.



  • This problem is just crying out for a hash table.  Or maybe LINQ.



  • @miner49er said:

    And it's not just the use of the ternary operator, it's the general style - who would do that and think it's 'neat'?

    Lisp programmers.

    To be fair, I prefer your version, but his seems decipherable. I've seen much, much worse ternary operator abuse.



  •  Your version would seem better to me, because there's two variables on each case.Had it been just one, I'd go with the first (with the improvement in the comment below your original post).

     But I'd use  } else {  on one line, simply because I don't like wasting lines.



  • @mariushm said:

     Your version would seem better to me, because there's two variables on each case.Had it been just one, I'd go with the first (with the improvement in the comment below your original post).

     But I'd use  } else {  on one line, simply because I don't like wasting lines.

     I'm imagining a 1940's era propaganda poster, thanking you for rationing your whitespace for the war effort.



  • @too_many_usernames said:

    This is actually a very good example of two functionally equivalent ways to do things with emphasis on different aspects of the problem.

    In the code from your boss, the emphasis is on the control property rather than the condition.  That is, for each of the properties you might want to do something (which in both cases depends on the user.CompleteRecord flag).

    Your code, however, places emphasis on "do something if user.CompleteRecord is true, and do something else is user.CompleteRecord is false".

    These are both valid ways to look at a problem, and I don't think there's actually a programming style that is easily flexible between the two different viewpoints.  I think at the end of the day you just have to be aware of the differences in emphasis.

    You might consider which is more likely to occur in the future: additional control properties (in which case emphasis on the properties makes sense) or if  you're going to add more action conditions (in which case emphasis on the conditions make sense).

    If you have a case where both the conditions and properties change, then I'd suggest picking one that is the most comfortable to all the maintainers.

     

    Very good answer, thank you.




  • I find the second one a little bit "prettier", but the first isn't much worse.

    The question is, what are you trying to do there? You don't want to alter some state of several elements based on a couple of conditions. You want to display one kind of UI when the record is completed, and another kind of UI if it is not. Looking at it from this point, the second piece definitely conveys this intention much better.



  • @miner49er said:

    So, what do you guys think? Am I making a mountain out of molehill?

    Your boss' code is perfectly fine.

     

    ... if he was writing in Lisp.



  • Yep, my first impression as that your boss' code was Lisp. Only after about 0.7s did I notice it was using the ternary operator.



  • @too_many_usernames said:

    This is actually a very good example of two functionally equivalent ways to do things with emphasis on different aspects of the problem.

    Well, they're not quite functionally equivalent, because the boss's way calls FindControl() twice, and miner49er's only calls it once.

    That aside, the two methods are basically, as you suggest, transposes of each other.  They are indeed both valid ways to look at the problem, but in certain contexts the choice of one way of looking or another can significantly impact the efficiency of the solution.  Compare for example SIMD programming with arrays of structs vs. structs of arrays; very often one approach or the other is a massive improvement in real terms on the other, even though theoretically the two algorithms are entirely isomorphic.




  • @too_many_usernames said:

    This is actually a very good example of two functionally equivalent ways to do things with emphasis on different aspects of the problem.

    In the code from your boss, the emphasis is on the control property rather than the condition.  That is, for each of the properties you might want to do something (which in both cases depends on the user.CompleteRecord flag).

    Your code, however, places emphasis on "do something if user.CompleteRecord is true, and do something else is user.CompleteRecord is false".

    These are both valid ways to look at a problem, and I don't think there's actually a programming style that is easily flexible between the two different viewpoints.  I think at the end of the day you just have to be aware of the differences in emphasis.

    You might consider which is more likely to occur in the future: additional control properties (in which case emphasis on the properties makes sense) or if  you're going to add more action conditions (in which case emphasis on the conditions make sense).

    If you have a case where both the conditions and properties change, then I'd suggest picking one that is the most comfortable to all the maintainers.

    Actually in both cases the first one (the boss's code) would be more difficult to maintain. I would always choose the 2nd example, because it looks clearer (the boss's code just looks like a garbled mess of characters while the 2nd example clear shows what the hell is going) and is easier to maintain in the future, for example if either more properties or more conditions were added.

    What I would like to know, though, is why the hell your boss is fiddling with your code, and thinks he has enough time to correct style issues within the code. Does he think you're not good enough to write code? Is he the one who has to maintain it later, so he just rewrites to suit his style? And if he never has to maintain it, then why does he interfere with it?



  • @pbean said:

    What I would like to know, though, is why the hell your boss is fiddling with your code, and thinks he has enough time to correct style issues within the code. Does he think you're not good enough to write code? Is he the one who has to maintain it later, so he just rewrites to suit his style? And if he never has to maintain it, then why does he interfere with it?

     

     

    When I started about a year ago, there were no coding standards of any kind and the codebase was a lot worse than it is now. There were NO comments in the code AT ALL. All class member variables were capitilised and had a preceding underscore. There was no change control system in place, there was source-control but there was no change-request/bug-tracking system, so tracking why a source-file had been edited was next to impossible.

    So, I came along and suggested we implemented a coding-standard, bug-tracking etc. Some people just don't like being told what they do is sub-optimal (understandable but not good in an engineering discipline). Eventually we now have bug-tracking but still no coding standards - well, the coding standards my boss decided to implement (due to "everyone ignoring coding-standards") was to use an automated tool to re-factor code. I believe this to be unnecesary, just write neat/readable code for god's sake! Plus I don't get a say in how the tool is configured, so we end up with crap like in my original post.

     

    The funny thing is one of the coding-styles that I really didn't like was nameing local objects the same as the class-nam, so we would end up with crap like this:

     

    Information Information = new Information();  //WTF!

     

    When questioned, my boss said "there's nothing wrong with that", so I had to bite my lip and ignore it. Months later when the 'tool' was used, it's default configuration was to dissallow such a style...so he listened to a dumb tool but not to me! Perhaps you can imagine that might just annoy me...just a little bit.

     

    So, to answer the question: he fiddles with the code because he runs that damn automatic tool blindly in an effort to employ a coding-standard!

    Eventually, I'll move on...

     



  • @miner49er said:

    Information Information = new Information();  //WTF!
    Why is this wrong? I need more information..



  • @Zecc said:

    @miner49er said:

    Information Information = new Information();  //WTF!
    Why is this wrong? I need more information..

     

     

    Just in case you're not joking then I will answer. The compiler won't complain as it's syntactly correct, it simply looks wrong IMHO. The standard way of doing it AFAIK is to simply name the object the same as the class but remove the initial captilisation.



  • While I was indeed joking, the truth is it isn't that bad as it is written.

    Without knowing the surrounding code, we can't suggest a better variable name, except maybe "info".



  • @Zecc said:

    While I was indeed joking, the truth is it isn't that bad as it is written.

    Without knowing the surrounding code, we can't suggest a better variable name, except maybe "info".

     

     

    Interesting. So you do not see a problem with having an object named exactly the same as the class it's created from?

    I suppose it's just a different style, it's just a style I do not like.

    I know a lot of coding styles have fallen by the wayside due to IDE technology advancing the way it has done, hungarian notion for example and this example is the same I suppose. A quick look at the colour of the text in the IDE will tell you it's not a Class but an object (see what I did there?)...I still think it's a poor way of naming objects.



  • @miner49er said:

    I still think it's a poor way of naming objects.
     

    It's a very poor way of naming objects and it needs to die.

    That, or explicitly statically typed languages that facilitate this kind of nonsense, but that's not such a great option.



  • @miner49er said:

    Interesting. So you do not see a problem with having an object named exactly the same as the class it's created from?

    Actually, I hadn't realised the capitalization was the same, even despite all the times it's been quoted now.
    @miner49er said:
    The standard way of doing it AFAIK is to simply name the object the same as the class but remove the initial captilisation.
    Yes, of course, you're right. IIRC this is what my IDE did back when I wrote Java.


  • @miner49er said:

    well, the coding standards my boss decided to implement (due to "everyone ignoring coding-standards") was to use an automated tool to re-factor code. I believe this to be unnecesary, just write neat/readable code for god's sake! Plus I don't get a say in how the tool is configured, so we end up with crap like in my original post.

    What the hell automated refactoring tool produces code like that?

    @miner49er said:

    Information Information = new Information();  //WTF!

    When questioned, my boss said "there's nothing wrong with that", so I had to bite my lip and ignore it. Months later when the 'tool' was used, it's default configuration was to dissallow such a style...so he listened to a dumb tool but not to me! Perhaps you can imagine that might just annoy me...just a little bit.

    It's not 1978 anymore, we all have IDEs now. Well, I mean, I'd use a lowercase variable name for a local variable, but other than that I have no problem with it being the same as the class name because it's in the completely different color with completely different tooltips.



  • The boss's version is not terrible. I dislike the ternary operator too, but it's only one level deep here. And I think there are too many line breaks, which break up the objects from their properties.

    Yours is somewhat more readable. Running around changing each others code is a WTF, though.

    And yet...  be grateful. He could be renaming everything to "q1", "q2", "q3(0)", "q3(1)", etc. I've seen code like that.



  • @D-Coder said:

    The boss's version is not terrible. I dislike the ternary operator too, but it's only one level deep here.
    Serious question here: what's with all the ternary operator hate?



  • @Enterprise Architect said:

    @D-Coder said:
    The boss's version is not terrible. I dislike the ternary operator too, but it's only one level deep here.
    Serious question here: what's with all the ternary operator hate?

    Previous discussion here.



  • @miner49er said:

    Information information = (Information)e.Row.FindControl("IncompleteWarning");

    if (user.CompleteRecord)
    {
      information.State = Information.ControlState.green_tick;
      information.ImageText = AppSettings.GetResourceString("CompleteUserProfile", ResourceType.GlobalString);
    }
    else
    {
      information.State = Information.ControlState.red_cross;
      information.ImageText = AppSettings.GetResourceString("IncompleteUserProfile", ResourceType.GlobalString);
    }

     

    What's wrong here is not the actual code, but what you're doing.  The intention is to mark "information" as either complete or incomplete.  So why don't you say that.  What you get is separation of concerns (what, vs how).  A single method call that clearly indicates what the intention is (tell the user that their profile is complete, or not); and a method that encapsulates all the presentation logic (how you tell the user that their profile is complete), making it easier to modify the code if/when the presentation requirements change.

     

    setIncompleteWarningState((Information)e.Row.FindControl("IncompleteWarning"), user)

    ....

    void setIncompleteWarningState(Information info, User user)
    {
    if (user.CompleteRecord)
    {
      information.State = Information.ControlState.green_tick;
      information.ImageText = AppSettings.GetResourceString("CompleteUserProfile", ResourceType.GlobalString);
    }
    else
    {
      information.State = Information.ControlState.red_cross;
      information.ImageText = AppSettings.GetResourceString("IncompleteUserProfile", ResourceType.GlobalString);
    }
    }

       

     

     

     

     



  • @dogbrags said:

    What's wrong here is not the actual code, but what you're doing.  The intention is to mark "information" as either complete or incomplete.  So why don't you say that.  What you get is separation of concerns (what, vs how).  A single method call that clearly indicates what the intention is (tell the user that their profile is complete, or not); and a method that encapsulates all the presentation logic (how you tell the user that their profile is complete), making it easier to modify the code if/when the presentation requirements change.

     

    I fully agree. To be fair though, I used these snippets merely as an example of the kind of refactoring that goes on. In actual fact, I quickly refactored that code into my version and presented it to my boss asking him directly which was neater/easier-to-read. You know his response and I think this is the real problem: Egos. Nobody likes being told that what they have done isn't very good or could be done better.

    I don't mind being informed of an alternative and superior approach to a problem - in fact I'm glad if it actually improves the situation, for a start it could stop me looking like an idiot later but mainly because it improves my knowledge and makes me a better coder.

    But some people take all criticism personally and THIS is what we need to avoid in our line of work.Take ideas on-board, discuss alternatives with colleagues and above al: Dont Be Stubborn.

     



  •  @dogbrags said:

    What's wrong here is not the actual code, but what you're doing.  The intention is to mark "information" as either complete or incomplete.  So why don't you say that.  What you get is separation of concerns (what, vs how).  A single method call that clearly indicates what the intention is (tell the user that their profile is complete, or not); and a method that encapsulates all the presentation logic (how you tell the user that their profile is complete), making it easier to modify the code if/when the presentation requirements change.
    But isn't this all presentation, though? I mean, it looks like it's just setting an image and some text.



  • @Zecc said:

    @miner49er said:

    Information Information = new Information();  //WTF!
    Why is this wrong? I need more information..

    We want information ... information ...

    You won't get it!



  • @DaveK said:

    @Zecc said:

    @miner49er said:

    Information Information = new Information();  //WTF!
    Why is this wrong? I need more information..

    We want information ... information ...

    You won't get it!

    By hook or by crook we will!



  • @miner49er said:

    @Rick said:

    They both seem readable to me. If your manager actually asked you to change your style to his, then you have a perfect right to be annoyed. But if you want your manager to switch to your style, then you are definitely making a mountain out of a molehill. (Has anyone actually seen a molehill? Pictures anyone?)

    He goes through my code and 'tidies' it up, this is just one example (I think he uses an automated tool to do changes like this). I understand that he is the boss and ultimatly it's up to him how the code looks - I don't have  to agree with it though. Or be happy about it.

    I'm curious as far as what tool would automatically convert you original code to ternary expressions?  I used an automated tool to adjust indentation and align braces, but it wouldn't generate ternary expressions.  I think your original code is more readable.



  • @blakeyrat said:

    @DaveK said:

    @Zecc said:

    @miner49er said:

    Information Information = new Information();  //WTF!
    Why is this wrong? I need more information..

    We want information ... information ...

    You won't get it!

    By hook or by crook we will!

    who are you?

    Who are you?

     



  • @DaveK said:

    @blakeyrat said:

    @DaveK said:

    @Zecc said:

    @miner49er said:

    Information Information = new Information();  //WTF!
    Why is this wrong? I need more information..

    We want information ... information ...

    You won't get it!

    By hook or by crook we will!

    who are you?

    Who are you?

     

    The new Number Two.



  • @blakeyrat said:

    @DaveK said:

    @blakeyrat said:

    @DaveK said:

    @Zecc said:

    @miner49er said:

    Information Information = new Information();  //WTF!
    Why is this wrong? I need more information..

    We want information ... information ...

    You won't get it!

    By hook or by crook we will!

    who are you?

    Who are you?

     

    The new Number Two.

    Who is Number One?

    Who is Number One?



  • @DaveK said:

    @blakeyrat said:

    @DaveK said:

    @blakeyrat said:

    @DaveK said:

    @Zecc said:

    @miner49er said:

    Information Information = new Information();  //WTF!
    Why is this wrong? I need more information..

    We want information ... information ...

    You won't get it!

    By hook or by crook we will!

    who are you?

    Who are you?

     

    The new Number Two.

    Who is Number One?

    Who is Number One?

    You are Number Six.



  • @blakeyrat said:

    @DaveK said:

    @blakeyrat said:

    @DaveK said:

    @blakeyrat said:

    @DaveK said:

    @Zecc said:

    @miner49er said:

    Information Information = new Information();  //WTF!
    Why is this wrong? I need more information..

    We want information ... information ...

    You won't get it!

    By hook or by crook we will!

    who are you?

    Who are you?

     

    The new Number Two.

    Who is Number One?

    Who is Number One?

    You are Number Six.

    *shakes fist at sky*

    I am not a number!  I am a free man!



  • C-C-C-COMBO BREAKER!

    @blakeyrat said:

    Previous holy war here.

    FTFY.

    So, in short, we should use it for all it's worth because it confuses people and it might contribute material to this site! CodeSODs have been thin lately.



  • @Enterprise Architect said:

    C-C-C-COMBO BREAKER!

    I think you're too late there, we pretty much wrapped that one up already!




  • @DaveK said:

    @Enterprise Architect said:

    C-C-C-COMBO BREAKER!

    I think you're too late there, we pretty much wrapped that one up already!


    Number Two does laugh. I guess I could have posted a laughing image.


Log in to reply