Hacker News Reader

Comment by leoedin | Hacker News Reader

Have you ever seen a well structured 1000 line function?

I'm sure they exist - maybe some sort of exceedingly complicated data transform or something. But in almost every situation I've seen, a 1000 line function has countless side effects, probably sets a few globals, takes loads of poorly named arguments, each of which is a nested data structure which it reaches deeply into and often has the same for loop copied and pasted 10 times with one character changed.

Often a 1000 line function is actually 5 or 6 20 line functions. I'm sure there are legitimate exceptions, but I've never seen them.

View on HN

https://github.com/github/putty/blob/master/terminal.c#L3281

This function is 1830 lines long. It's reasonably well structured I think. Although the #if 0 are maybe not so good.

A lot of those if-/case-blocks are precisely where I'd put functions :)

If you changed a bunch of those to separate, pure (i.e. side-effect-free) functions it would if nothing else make unit testing a breeze, and then you'd be free to fix bugs in the logic without fear. As it is, if I had a bug in that huge function I'd be really worried about breaking some edge-condition or implied-state 500 lines up etc.

They can’t be side effect free, that’s the point. The switch statements are mutating the input.

There's a lot that can be, take this for instance (line 3336):

  if (c == '\033')
      term->print_state = 1;
  else if (c == (unsigned char)'\233')
      term->print_state = 2;
  else if (c == '[' && term->print_state == 1)
      term->print_state = 2;
  else if (c == '4' && term->print_state == 2)
      term->print_state = 3;
  else if (c == 'i' && term->print_state == 3)
      term->print_state = 4;
  else
      term->print_state = 0;

This could be turned into a pure function that takes c and print_state as input, and returns a new print_state which the outer function assigns. That's 12 lines turned into 1.

  term->print_state = newState(c, term->print_state);

(I am not a C developer so the sytax could be wrong). Just because the outer function is impure doesn't mean it can't in turn call pure functions.

It's a Mealy state machine, it should be encoded as a state machine!

Why some folks will absolutely insist that a series of manually-written case statements in a single thousand line function is the epitome of style, when there's a pure state machine model inside wishing you would free it from the shackles of if else if else if else....

I have some experience with state machines, and I absolutely hate them. It's so easy to get lost, you never know where a function will go to next.

There's a reason stacks are ubiquitous, they're much easier to fit in your head.

You can still have side-effects in your “big” function, based on the return values of pure functions.

I totally agree. And you can always write mutating “functions” (what get called “algorithms” in the C++ world), like C++’s `std::ranges::sort(range)`.

You can always use return values, and allow the caller to decide how to use them (e.g. caller may mutate other values).

Depending on use and language, this may be "expensive" (e.g. you could be allocating for and then copying some huge data structure only to pass it back where is is simply copied over the top of the input), but this is where discretion comes in and decisions are made on what is appropriate (i.e. is performance critical, or is correctness and maintainability more important?)

Oh my God. The sneaky `else` at line 3400.

edit: another one at 3826 with a preprocessor define interleaved.

Well that's horrendous. Sorry, Simon. Each of those big "switch" statements should be broken out.

Why? That would create lots of small functions that each is called only once from only one place in the code and still are extremely coupled to the invoking switch statement, often having multiple inputs and multiple outputs which then the invoking statement needs to write in the proper places to which the function wouldn't have access.

Breaking out functions makes sense when you get either reuse or decoupling, but in this case you don't get any of these.

Because it would objectively and shockingly obviously make the code easier to understand.

You would be able to see what it does at a high level at a glance and then drill down into the functions and sub-functions to focus on a particular part of it.

With this version, to get the high level overview you have to scroll through multiple pages of code and find the comments that say what each section does.

So the problem is being able to drill down? Most decent IDEs allow you to collapse code blocks (i.e brackets) if that's what you want to do, and the comments accomplish the same thing a function name would. Some editors support region comments that start out collapsed and can be named. I don't see why separate functions would be anyones first instinct to reach for here.

I don't see why anyone's first instinct to reach for would be relying on some editor's ability to optionally collapse code blocks.

What happens if someone wants to use a different editor that doesn't have that feature or doesn't display it quite right anymore?

Lol.

I`m not OP, but yes, I saw.

Thats my personal opinion, and nothing more:

Something like complex one time financial/workflow/maintenance operation that includes calling dozens of different smaller functions, but very well structured.

It does not make sense to separate it more into different functions, because execution is generally linear and having to deal with tree of calls where some calls is depends on state of the previous is become cumbersome and makes reading and making changes more complex.

Again, thats my personal feeling, and nothing more.

Reply on news.ycombinator.com