Those who know, know.

  • Mia
    6•5 months ago

    Yeah that was essentially what I was referring to (referring to your edit).

    I generally dislike stuff like (crappy example incoming):

    void do_stuff(int count, bool cond) {
	function1(count);
	function2(b);
	function3();
}

void function1(int count) {
	for (var i = 0; i < count; i++) {
		...
	}
}

void function2(bool cond) {
	if (cond) { ... }
	else { ... }
}

void function3() {
	...
}

    I’m not a fan of this kind of code fragmentation.
    If all those actions were related and it could have been just one thing, retaining a lot more context, then it should be one function imo.
    If by not splitting it it became massive with various disconnected code blocks, sure, but otherwise I’d much prefer being able to read everything together.

    If splitting the functions required producing side effects to maintain the same functionality, then that’s even worse.

    • @[email protected]
      4•5 months ago

      Huh, I really like code like that. Having a multi-step process split up into sections like that is amazing to reason about actual dependencies of the individual sections. Granted, that only applies if the individual steps are kinda independently meaningful

      To adapt your example to what I mean:

      Baz do_stuff(int count, boolean cond) {
	Foo part1 = function1(count);
	Bar part2 = function2(cond);
	return function3(part1, part2);
}

      This allows you to immediately see that part1 and part2 are independently calculated, and what goes into calculating them.

      There are several benefits, e.g.:

      1. if there is a problem, you can more easily narrow down where it is (e.g. if part2 calculates as expected and part1 doesn’t, the problem is probably in function1, not function2 or function3). If you have to understand the whole do_stuff before you can effectively debug it, you waste time.
      2. if the function needs to be optimized, you know immediately that function1 and function 2 can probably run in parallel, and even if you don’t want to do that, the slow part will show up in a flame graph.
      • Mia
        5•5 months ago

        It really depends on the context frankly. I did say it was a crappy example ;)
        Try to read this snippet I stole from Clean Code and tell me if it’s readable without having to uselessly jump everywhere to understand what’s going on:

        public class SetupTeardownIncluder {
  private PageData pageData;
  private boolean isSuite;
  private WikiPage testPage;
  private StringBuffer newPageContent;
  private PageCrawler pageCrawler;


  public static String render(PageData pageData) throws Exception {
    return render(pageData, false);
  }

  public static String render(PageData pageData, boolean isSuite) throws Exception {
    return new SetupTeardownIncluder(pageData).render(isSuite);
  }

  private SetupTeardownIncluder(PageData pageData) {
    this.pageData = pageData;
    testPage = pageData.getWikiPage();
    pageCrawler = testPage.getPageCrawler();
    newPageContent = new StringBuffer();
  }

  private String render(boolean isSuite) throws Exception {
     this.isSuite = isSuite;
    if (isTestPage())
      includeSetupAndTeardownPages();
    return pageData.getHtml();
  }

  private boolean isTestPage() throws Exception {
    return pageData.hasAttribute("Test");
  }

  private void includeSetupAndTeardownPages() throws Exception {
    includeSetupPages();
    includePageContent();
    includeTeardownPages();
    updatePageContent();
  }


  private void includeSetupPages() throws Exception {
    if (isSuite)
      includeSuiteSetupPage();
    includeSetupPage();
  }

  private void includeSuiteSetupPage() throws Exception {
    include(SuiteResponder.SUITE_SETUP_NAME, "-setup");
  }

  private void includeSetupPage() throws Exception {
    include("SetUp", "-setup");
  }

  private void includePageContent() throws Exception {
    newPageContent.append(pageData.getContent());
  }

  private void includeTeardownPages() throws Exception {
    includeTeardownPage();
    if (isSuite)
      includeSuiteTeardownPage();
  }

  private void includeTeardownPage() throws Exception {
    include("TearDown", "-teardown");
  }

  private void includeSuiteTeardownPage() throws Exception {
    include(SuiteResponder.SUITE_TEARDOWN_NAME, "-teardown");
  }

  private void updatePageContent() throws Exception {
    pageData.setContent(newPageContent.toString());
  }

  private void include(String pageName, String arg) throws Exception {
    WikiPage inheritedPage = findInheritedPage(pageName);
    if (inheritedPage != null) {
      String pagePathName = getPathNameForPage(inheritedPage);
      buildIncludeDirective(pagePathName, arg);
    }
  }

  private WikiPage findInheritedPage(String pageName) throws Exception {
    return PageCrawlerImpl.getInheritedPage(pageName, testPage);
  }

  private String getPathNameForPage(WikiPage page) throws Exception {
    WikiPagePath pagePath = pageCrawler.getFullPath(page);
    return PathParser.render(pagePath);
  }

  private void buildIncludeDirective(String pagePathName, String arg) {
    newPageContent
      .append("\n!include ")
      .append(arg)
      .append(" .")
      .append(pagePathName)
      .append("\n");
  }
}

        That’s what I was talking about.