Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

Class header formatting #2

Open
yole opened this issue May 31, 2016 · 29 comments
Open

Class header formatting #2

yole opened this issue May 31, 2016 · 29 comments

Comments

@yole
Copy link
Contributor

yole commented May 31, 2016

If the class header (primary constructor and superclass list) doesn't fit on a single line, put the opening curly brace of the class on a separate line, with no indentation.

class C(val a: String,
        val b: String)
{
}
@cypressious
Copy link

Personally, I use a different style:

class C(
    val a: String,
    val b: String
) : SuperType {
    val d: String  = ""
}

This has the benefit that all properties are horizontally aligned.

@AlexeyTsvetkov
Copy link
Member

AlexeyTsvetkov commented May 31, 2016

Also formatting suggested by @cypressious is more consistent in terms of { } placement.

In addition to what was already mentioned before I think the following examples of formatting should be covered:

  • classes with type parameters (there can be many type parameters, so multiline variant should be covered)
  • classes with many supertypes (multiline variant)

@voddan
Copy link

voddan commented Jun 8, 2016

Same if the list of super types is long:

class MyFavouriteVeryLongClassHolder() 
    : MyLongHolder<MyFavouriteVeryLongClass> 
{
}

@gildor
Copy link

gildor commented Jun 8, 2016

@voddan Do not like curly bracket on the new line

@voddan
Copy link

voddan commented Jun 8, 2016

@gildor same reason why { is on a new line at the header of the topic

@gildor
Copy link

gildor commented Jun 8, 2016

@voddan I prefer style from @cypressious comment.

@voddan
Copy link

voddan commented Jun 8, 2016

@gildor could you please provide an example of a formatting for a class with an empty constructor but with a long list of super types?

@gildor
Copy link

gildor commented Jun 8, 2016

@voddan

class MyFavouriteVeryLongClassHolder() : 
        MyLongHolder<MyFavouriteVeryLongClass>(), MyInterface1, MyInterace2, 
        MyInterface3,  MyInterface4 {
}

Also, I'm not sure about a position of a colon. I think for a colon we should use the same rule as for expression body formatting #1

Because if we move a colon to the next line Idea formats it with this way:

class MyFavouriteVeryLongClassHolder() 
: MyLongHolder<MyFavouriteVeryLongClass>() {
}

@cypressious
Copy link

The formatter can be improved if it is decided that this style looks better.

@voddan
Copy link

voddan commented Jun 8, 2016

Your first example has a flaw that the first line of the class body mixes with the last line of the declaration:

class MyFavouriteVeryLongClassHolder() : 
        MyLongHolder<MyFavouriteVeryLongClass>(), MyInterface1, MyInterace2, 
        MyInterface3,  MyInterface4 {
    println("Hello world!")
}

A solution to that is to add a new line before the println, but AFAIU, the purpose of the header was to avoid that

@gildor
Copy link

gildor commented Jun 8, 2016

@voddan Agree, that it's not perfect, but more compact and consistent with other blocks formating (opening curly bracket on the same line)

class MyFavouriteVeryLongClassHolder()
    : MyLongHolder<MyFavouriteVeryLongClass>()
{
    fun foo() {}
}
class MyFavouriteVeryLongClassHolder() :
        MyLongHolder<MyFavouriteVeryLongClass>() {
    fun foo() {}
}

or with a colon on the next lline (I do not have strong opinion about colon position) :

class MyFavouriteVeryLongClassHolder()
        : MyLongHolder<MyFavouriteVeryLongClass>() {
    fun foo() {}
}

@voddan
Copy link

voddan commented Jun 8, 2016

@gildor the problem I mentioned is very common and a huge PITA.

If you dislike the rule I proposed (it looks meh, but I don't know a better solution) you should actively express it toward the rule at the header of this topic, since I believe the are at sync

@cypressious
Copy link

Small addition for constructor annotations:

class Foo @Inject constructor(
    val prop: String
) {

}

@kevinmost
Copy link

kevinmost commented Jun 14, 2016

Trying to make some crazy class-headers just to see how I can keep it readable when a ton of logic is in the header. I ended up fairly happy with something like:

open class FooActivity
@Inject
@JvmOverloads
protected constructor(
        val foo1: String,
        val foo2: Map<String, Map<Int, String>>,
        val foo3: SomeInterface1? = null
) : Activity(),
        SomeInterface1 by foo3 ?: object: SomeInterface1 {},
        SomeInterface2 {

    // Just foo1 with its length added on at the end
    val foo4 = "${foo1}:${foo1.length}"

    init {
        if (foo1.length > 10) {
            throw IllegalArgumentException("Can't set foo1 to more than 10 char long")
        }
    }

    fun foo() {}

    fun getFoo1Length() = foo1.length

    fun someComplicatedThing() {
        // TODO: Make this do a cool thing
        TODO()
    }
}

@cypressious
Copy link

I wouldn't put the parantheses of the constructor on separate lines.

2016-06-14 20:55 GMT+02:00 Ruckus T-Boom [email protected]:

@kevinmost https://github.com/kevinmost I tried cleaning up your
suggestion a bit:

open class foo_activity
@Inject
@JvmOverloadsprotected constructor
(
val foo_1: String,
val Foo2: Map < String , Map < Int , String > >,
val Foo_Three: some_interface_1? = null
)
: Activity (),
some_interface_1 by Foo_Three ?: object: some_interface_1
{
},
iSomeInterface2
{
init
{
if ( foo1.length > 10 )
{
throw IllegalArgumentException( "Can't set foo1 to more than 10 char long" )
}
}

fun foo ()
{
}

fun get_foo_1_length ()
    = foo1.length

fun some_complicated_thing ()
{
    // TODO: Make this do a cool thing
    TODO ()
}

}


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABIviGHNAx6ANpZASwdaTdgQBg-0xjsfks5qLvkwgaJpZM4Iqw-j
.

@ruckustboom
Copy link

@cypressious I was being facetious and removed my comment as it wasn't helpful. Please do not take it seriously. I would never actually write code like that.

@rocketraman
Copy link

I generally like @kevinmost proposal but I prefer a more compact style that uses more horizontal space (I generally set my editor margin out to 130 chars, which is easily viewable in one screen on modern monitors). The constructor annotations are part of the constructor which is part of the class declaration so I like keeping those together. I also don't like multiple indentation -- I like things to be visually aligned so I keep everything to two:

open class FooActivity @Inject @JvmOverloads protected constructor(
  val foo1: String,
  val foo2: Map<String, Map<Int, String>>,
  val foo3: SomeInterface1? = null
) : Activity(),
  SomeInterface1 by foo3 ?: object: SomeInterface1 {},
  SomeInterface2 {

  // Just foo1 with its length added on at the end
  val foo4 = "${foo1}:${foo1.length}"

  init {
    if (foo1.length > 10) {
      throw IllegalArgumentException("Can't set foo1 to more than 10 char long")
    }
  }

  fun foo() {}

  fun getFoo1Length() = foo1.length

  fun someComplicatedThing() {
    // TODO: Make this do a cool thing
    TODO()
  }
}

@Groostav
Copy link

Groostav commented Jun 29, 2016

@rocketraman 130 characters is pretty wide, and likely to cause you problems if you want to use a command line editor no?

Along those lines, in my experience names get very long, and if you consider access restriction and annotations on the constructor, we get this:

open class CommandAndServiceAndParkadeControllerViewService @Inject @JvmOverloads protected constructor(
  val foo1: String,
  val foo2: Map<String, Map<Int, String>>,
  val foo3: SomeInterface1? = null
) : Activity(),
  SomeInterface2 {

which means I think its more conventional to simply do what the jquery text box on github is already doing for me: put a newline between your first annotation and the end of the class name

open class CommandAndServiceAndParkadeControllerViewService 
@Inject @JvmOverloads protected constructor(
  val foo1: String,
  val foo2: Map<String, Map<Int, String>>,
  val foo3: SomeInterface1? = null
) : Activity(),
  SomeInterface2 {

note I removed your anonymous delegating class, that seems like it would be enormously obnoxious to implement, and I'm still warming up to the idea of inheritance by delegation. The formatting there would be really difficult:

class FooActivity (
    val foo1: String
) : Activity(),
  SomeInterface1 by foo3 ?: object: SomeInterface1 {
    override fun things(): Unit{
      //code
    }
  },

I think in general you're better off delegating the whole thing to a local method if thats legal kotlin:

class FooActivity (
    val foo1: String
) : Activity(),
  SomeInterface1 by asSomeInterface1(foo3){

  //...
  fun asSomeInterface1(arg: Foo3): SomeInterface1{
    return foo3 ?: //...
  }
}

@rocketraman
Copy link

rocketraman commented Jun 29, 2016

@rocketraman 130 characters is pretty wide, and likely to cause you problems if you want to use a command line editor no?

Hmm, I've never had an issue with it and I use the command line all the time. Though rarely to edit code. Occasionally github does not show the entire line on the web. I've never found it a problem.

Even with the name you gave (which honestly, I'd think about renaming to be shorter :-) ), the declaration goes out to 104 chars. No biggie.

@gregsh
Copy link

gregsh commented Nov 22, 2016

The following feels somewhat wrong.

fun someFunHappens(
        place: String,
        time: String,
        dresscode: String
): String {    // suddenly a SAD FACE!
  
}

There're 1204 sad faces and long sad faces in Kotlin sources.
(Searched with ^\)\s*\: pattern)

@MarcinMoskala
Copy link

I'm trying to put it all together. So for short header:

class Person(id: Int, name: String) {
    //...
}

for multiple arguments:

class Person(
    id: Int, 
    name: String,
    surname: String
) {
    //...
}
class Person(
    id: Int, 
    name: String,
    surname: String
): Human() {
    //...
}

Multiple with multiple arguments on extension:

class Person(
    id: Int, 
    name: String,
    surname: String
): Human<Person>(id, name) {
    //...
}

Multiple with multiple arguments on extension and interfaces:

class Person(
    id: Int, 
    name: String,
    surname: String
): Human<Person>(id, name),
    NotADog,
    Somebody {
    //...
}

Is it correct?

@Jeevuz
Copy link

Jeevuz commented Sep 18, 2017

Is there a way to have 4-space indent for the class arguments? Because with the default 8 spaces it looks not so pretty:

class Person(
        id: Int, 
        name: String,
        surname: String
): Human<Person>(id, name),
    NotADog,
    Somebody {
    //...
}

@roschlau
Copy link

@Jeevuz The indent for primary constructor arguments is controlled by the "Continuation Indent" setting in the code style settings for Kotlin. I have that set to the same as the normal indent, but I too would like a separate control for that indent specifically.

@elizarov
Copy link

In the current style guide when the 4-space indentation gets combined with the "opening brace always on the same line" and "every interface on its own line in class header" it ends up looking like this (copied from https://github.com/JetBrains/kotlin-web-site/blob/yole/styleguide/pages/docs/reference/coding-conventions.md):

class Person(
    id: Int,
    name: String,
    surname: String
) : Human(id, name),
    KotlinMaker {
    
    fun memberFunction() { 
        // ...  
    }
}

while an extra empty line does add some separation from class header to its body it is still somewhat confusing to see class header continued into the body on the same indent level. I, personally, would really have preferred an opening brace on a separate line just for the case of multi-line interface list (but in no other case):

class Person(
    id: Int,
    name: String,
    surname: String
) : Human(id, name),
    KotlinMaker 
{    
    fun memberFunction() { 
        // ...  
    }
}

Thumbs up or down?

@damianw
Copy link

damianw commented Sep 25, 2017

I agree that it's a little awkward to have the body and the class header left-aligned, but I don't think that it's worth making an exception for the opening brace on a separate line in this case. When you have actual syntax highlighting, it's really not much of a problem... any members of the body will be highlighted differently:

class Person(
    id: Int,
    name: String,
    surname: String
) : Human(id, name),
    KotlinMaker {
    
    fun memberFunction() { 
        // ...  
    }
}

The real problem is probably that primary constructor parameters coincidentally align with the supertypes.

@Jeevuz
Copy link

Jeevuz commented Oct 17, 2017

So what about the : position in case of an empty constructor?

I think moving it on the new line is better because it looks more consistent with the chosen header style and also looks like the . in a chain (which we is also puts on a new line).

class MyFavouriteVeryLongClassHolder
    : MyLongHolder<MyFavouriteVeryLongClass>(),
      SomeOtherInterface,
      AndAnotherOne {

    fun foo() {}
}

@Jeevuz
Copy link

Jeevuz commented Sep 4, 2018

Hi all!
How will you format this case?

class Something(
    val param: Param,
    foo: Foo,
    bar: Bar,
    andMore: More,
    andOneThatCrossesTheLine
) : SomethingSupertype(
    foo,
    bar,
    andMore,
    andOneThatCrossesTheLine
), Interface1,                                  // this line loocks ugly
    Interface2 {

    fun funnyFun() {
        ...
    }
}

@roschlau
Copy link

roschlau commented Sep 4, 2018

Well, I'd first try finding a solution that doesn't need this much inheritance...

But if needed, I'd probably format it something like this:

class Something(
    val param: Param,
    foo: Foo,
    bar: Bar,
    andMore: More,
    andOneThatCrossesTheLine
) : SomethingSupertype(
        foo,
        bar,
        andMore,
        andOneThatCrossesTheLine
    ),
    Interface1,
    Interface2 { // I'd maybe even consider putting this opening brace on the next line for clarity

    fun funnyFun() {
        ...
    }
}

@Jeevuz
Copy link

Jeevuz commented Sep 4, 2018

Thanks, I like your solution.
Will be great if IDEA will autoformat this way.
(because now it does what I've showed.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests