willowtreeapps / swift-style-guide

The official Swift style guide for WillowTree, Inc.
MIT License
16 stars 3 forks source link

[RFC] Improve long parameter, argument, and element list organization #38

Closed willtellis closed 4 years ago

willtellis commented 7 years ago

I think these coding style changes improve organization and readability for long function parameter lists, long function argument lists, and long collection element lists. I don't think these style changes should be mandatory, but they ought to be part of the suggested style. They are especially helpful for complex view model declarations, as I think my example shows. If you agree, please help me improve the language and examples.

ianterrell commented 7 years ago

I'm somewhat in favor, but halfway opposed.

I think that vertical real estate implies importance and complexity, and this style gives the appearance of complexity for what is often trivial code (initialization, method calls). I am more in favor of spreading these types of calls across many lines at the call site when parameters are non-trivial (e.g. involve computation), and less in favor of it when they're simple (e.g. constants).

Compare, for instance, the suggested preferred struct init with my preferred style for that example:

let foo = CellViewModel(title: "Foo", detail: "Reticulated", showDisclosureIndicator: true)
let bar = CellViewModel(title: "Bar",detail: "Unreticulated", showDisclosureIndicator: false)
let secondSection = SectionViewModel(title: "Splines", dataSource: [foo, bar])

We've gone from 15 lines to 3. I don't think creating a little data is 15 lines of important.

To generalize it, you risk code like this:

simple
simple
simple
simple
simple
simple

complex
complex
complex

simple
simple
simple
simple

Which I would find less readable at a glance, as the "meat" is obscured.

ianterrell commented 7 years ago

That said, I do this for method declarations, especially with generics or other line-bloating things. Here's an example from live code:

private func getData<T: ResultBasedDocument>(
    _ request: URLRequest,
    cacheFile: FileWriter?,
    parser: XMLPushParser<T>,
    completion: @escaping (Result<T.Result, MetadataFetchError>) -> Void
) -> Cancellable {
    ...
}
willtellis commented 7 years ago

@ianterrell I get what you're saying about the implied importance of vertical real estate. I've been dealing with a lot of declarative code, and I think that's where this style can be used to increase clarity, as opposed to more imperative or algorithmic code. Much like JSON or HTML, the structure of the data is the complex part of the code. At the risk of undermining my toy example, I think I would actually write that bit of code like so:

let firstSection = SectionViewModel(
    title: "Splines",
    dataSource: [
        CellViewModel(title: "Foo", detail: "Reticulated", showDisclosureIndicator: true),
        CellViewModel(title: "Bar", detail: "Unreticulated", showDisclosureIndicator: false)
    ]
)

The point being that the hierarchical structure of the SectionViewModel and its relationship to the CellViewModels are made more apparent by the nested declaration, which itself is made clearer by the vertical and horizontal spacing I'm proposing. In more algorithmic/imperative code, declaring the objects before the place where they're used makes more sense. Doing so reduces nesting, so you may not need to use this kind of spacing.

willtellis commented 7 years ago

Ultimately, I'm really just proposing a consistent, generalizable way to break things up across multiple lines when it's necessary to improve clarity. In any particular case, refactoring the code may be a better solution, but prescribing a strategy for breaking up long lines should not imply the preclusion of that option.