5 ms·

If you have a one-line function that is used one place (or even a few places) and it doesn't mean it is easier to understand just because you made it look like

• MaulingMonkey 10y ago
  > If you have a one-line function that is used one place (or even a few places) and it doesn't mean it is easier to understand just because you made it look like English. More likely you just obfuscated it: update_string_with_value(string,val) is NOT easier to understand than string.append(val). We agree your sample has been obfuscated. /// <summary> /// Case Insensitive IRC Name /// </summary> public struct CIIrcName { private string Data; public static implicit operator CIIrcName( string s ) { return new CIIrcName() { Data=s }; } public static implicit operator string( CIIrcName n ) { return n.Data; } public string ToLower() { return (Data??"").ToLowerInvariant().Replace('{','[').Replace('}',']').Replace('|','\\'); } // Silly scandinavians :< public static bool operator==( CIIrcName lhs, CIIrcName rhs ) { return lhs.ToLower() == rhs.ToLower(); } public static bool operator!=( CIIrcName lhs, CIIrcName rhs ) { return lhs.ToLower() != rhs.ToLower(); } public override bool Equals( object obj ) { return obj is CIIrcName && (CIIrcName)obj == this; } public override int GetHashCode() { return ToLower().GetHashCode(); } public override string ToString() { return Data; } } Every single function of this is a one-liner. Are any of them obfuscated? (IRC servers sometimes talk about the same user or channel in multiple cases, this was a quick drop-in to prevent these from being mistaken as different users/channels by my IRC client. The "Silly scandinavians" comment references the fact that, yes, IRC thinks '[' is lowercase '{'. And yes, I've seen that matter in practice.) Most of the changes I'd make here have nothing to do with method length: 1) Conversion (at least back to string) should probably be made explicit at this point (was originally implicit for ease-of-use when replacing string s) 2) ToLower could be made private (only used inside the type) and have the comment replaced with the above blurb (instead of being a comment to myself that only I'll properly decode.) 3) Rename CIIrcName to CaseInsensitiveIrcName? 4) Rename Data to OriginalCaseName? > And you're also coding defensively against your fellow developers. This tends to be a problem in low- to average-skill workplaces. My experience has been just the opposite. Low/average skill tends to mean less asserts, less static analysis, ignored warnings instead of warnings-as-errors, no unit tests, no thread safety annotations... forget my coworkers - I add these things to code defensively against future-me, and I've found it super effective on larger codebases. > This goes all the way back to when they learned it was a good idea to comment their code "next = ++i // increment index then assign". We agree this is bad too. A different perspective though - those are perfectly reasonable comments when you're new enough to programming that you don't remember what "++i" does! But it's something to grow out of and remove.
  • jnordwick 10y ago
    I can't really comment about your specific use case, but I would wonder why you are having to do these conversions and comparisons on the fly so much that you a full class dedicated to them and you don't have the the name normalized ahead of time. But I don't really know. But like I've said in all my comments. There are times and places for all these maxims and rules to be broken. That's why programming is difficult, especially that is why finding the right abstraction so that it looks easy is difficult. The best programmers I know always seem to find that right balance and you look at their code and go "I could do that" when in reality you couldn't.
    • MaulingMonkey 10y ago
      > but I would wonder why you are having to do these conversions and comparisons on the fly I receive a weird mix of normalized and unnormalized input over the network from servers and software I don't control, and want to keep the unnormalized versions for display purposes. I could normalize on construction - might save a tiny bit of performance? - but then I'd need to store and keep in sync two separate strings (one for display, one for comparison.) Not that I have any real mutation going on that would violate that invariant in practice. > so much that you a full class dedicated to them and you don't have the the name normalized ahead of time. The general pattern is: Parse the network message (containing mixed normalization), then immediately lookup (or create) the target channel/user - allowing no duplicates due to mixed normalization. With this I can simply construct e.g. Dictionary<CIIrcName,...>s and not need to remember to normalize every time I want to query by a given key. Only about 5 members that reference this type but there's a good 20-30 key lookups/equality comparisons using those members, handling different network messages among other things. That list will probably expand. I could've wrapped dictionary access instead, but this seemed simpler (as dictionaries aren't the only thing I'm using, and none of the existing code wrapped dictionary access.)
      • jnordwick 10y ago
        I'd normalize on instantiation. It is both a performance win, smaller code, and much simplier. Chanel names don't change so you dont have to keep them in sync in any real sense, and if they do, the setter has a very trival job of setting both the real and normalized name.
• dllthomas 10y ago
  "Made it look like English" probably isn't an improvement. "Made it so a future modification is made in one place instead of four (possibly missing some)" probably is.
• InclinedPlane 10y ago
  I feel like you're arguing against something else. It should be obvious that I'm not saying "ALWAYS USE 1-LINE FUNCTIONS". My point is that a phobia against small functions is both extremely common and typically harmful to code quality, for all the reasons I described. If it were easy enough to give someone a simple checklist of guidelines and have them produce high quality code, coding would be a lot easier, but it doesn't work that way. Coding well requires years of experience that produces the wisdom and perspective to know when and to use different techniques. It's what tells you "it's unnecessary to comment an i++ line", it's what tells you when and how to use one data structure over another, and so on. And no, I'm not "refactoring ahead of the refactoring", I'm refactoring where it makes sense, even down to a single line. That's the point. To properly compose things, and to avoid the bias against small functions. Not to atomize a program into infinite function calls, of course. Most of the code I've come across that is below the quality I'd like tends to have excessively large function definitions, precisely because there is such a widespread antipathy against splitting things up "for no reason" and against small functions. When it improves readability, when it improves the composition and modularity of the system, when it makes it easier to update, modify, improve logging and error handling, etc. those are all good reasons for extracting methods. The rare times I've seen code that required excessive jumping around to figure out what anything did it wasn't because of too many small methods, it was because of overzealous application of bad design patterns (e.g. call a factor method to produce a configuration object then instatiate some other factory and pass the configuration object to it to get a factory that produces what you want, that kind of nonsense). That's indicative of a failure of judgment. My point is, don't be afraid of small functions when they make sense. There are too many people who are afraid of small functions because, like you, they have these strong biases against them based on prejudice. How do you know when a small function is a good idea? Judgment and experience, of course, same as always.
• jghn 10y ago
  "is NOT easier to undestand than string.append(val)." Disagree 100%. But of course as everything else in our industry this is super subjective. I'd much rather a line of combinators which read like English than a collection of function calls with wacky parameter calls. YMMV on this though and I understand that. I also want tiny functions and those functions to be moved far away. I want the "english" names as close to the Real Work as possible. If I really care what the activity is I can use the IDE to drill in. But again, I understand YMMV on this one. People who argue this topic as if there's a ground truth are the true problems. Everyone has they're own take.
  • InclinedPlane 10y ago
    > "People who argue this topic as if there's a ground truth are the true problems. Everyone has they're own take." Exactly. It's like design patterns, algorithms, and data structures. There are lots of techniques useful in programming. The trick is having the experience and judgment to know when to use the right one.
• hackinthebochs 10y ago
  >is NOT easier to understand than string.append(val). But this depends entirely on the context of the code. I like to think of this in terms of "abstraction levels". Mixing abstraction levels harms readability and so if your code is doing some low level operation then string.append(val) should be obvious from the context and the name of the function it belongs in. If however the code is still at an abstract enough level then you want to wrap your implementation detail in a name that is appropriate for that level of abstraction. As a rough example, in a function that's popping from some data source and pushing into a local structure as a part of some larger unit of work, wrapping your collection.append(val) would be harming readability. But if you're "enrolling" a "student" into a "class" you should be doing class.enroll(student) even if the implementation is just class.students.append(student_id)
  • flukus 10y ago
    > As a rough example, in a function that's popping from some data source and pushing into a local structure as a part of some larger unit of work, wrapping your collection.append(val) would be harming readability. But if you're "enrolling" a "student" into a "class" you should be doing class.enroll(student) even if the implementation is just class.students.append(student_id) After calling class.enrol(student), I would expect the student to be enrolled, not just added to a list of students.
    • DSMan195276 10y ago
      > After calling class.enrol(student), I would expect the student to be enrolled, not just added to a list of students. Why do they have to be different? That's really his point. In this situation, enrolling just requires adding their student id to the list of students in the class. But while doing 'class.students.append(student_id)' accomplishes that, it is not clear reading that line that it actually enrolls the student in the class, rather then just being one step of a larger process, or something else all-together.
      • flukus 10y ago
        If they are not different then you're half way to going full Active Record (which I'd consider an anti-pattern). There is likely much more to enrolling a student than adding them to a class, and most of that logic should not go in class.