Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve formatting of method will lot of parameters and with exception #286

Open
murdos opened this issue Oct 20, 2019 · 12 comments · May be fixed by #591
Open

Improve formatting of method will lot of parameters and with exception #286

murdos opened this issue Oct 20, 2019 · 12 comments · May be fixed by #591

Comments

@murdos
Copy link

murdos commented Oct 20, 2019

Source:

    public LoggingConfiguration(@Value("${spring.application.name}") String appName,
        @Value("${server.port}") String serverPort,
        JHipsterProperties jHipsterProperties,
        ObjectMapper mapper)
        throws JsonProcessingException {
        // ...
    }

Formatted with prettier 0.4.0:

    public LoggingConfiguration(
        @Value("${spring.application.name}") String appName,
        @Value("${server.port}") String serverPort,
        JHipsterProperties jHipsterProperties,
        ObjectMapper mapper
    )
        throws JsonProcessingException {
        // ...
    }

I'm not really sure what the result should be, but the position of ) after the last parameter is a bit strange in regards to the exception.
Maybe:

    public LoggingConfiguration(
        @Value("${spring.application.name}") String appName,
        @Value("${server.port}") String serverPort,
        JHipsterProperties jHipsterProperties,
        ObjectMapper mapper
    ) throws JsonProcessingException {
        // ...
    }
@clementdessoude
Copy link
Contributor

I agree with you. I am struggling with one case: how would you format the following ?

public void test() throws Exception1, Exception2, Exception3, Exception4, Exception5, Exception6 {}

Option 1:

public void test() 
    throws Exception1, Exception2, Exception3, Exception4, Exception5, Exception6 {}

Option 2:

public void test() throws 
    Exception1, Exception2, Exception3, Exception4, Exception5, Exception6 {}

Option 3:

public void test() throws Exception1, 
    Exception2, 
    Exception3, 
    Exception4, 
    Exception5, 
    Exception6 {}

Option 4:

public void test() throws 
    Exception1, 
    Exception2, 
    Exception3, 
    Exception4, 
    Exception5, 
    Exception6 {}

Option 5:

public void test() throws Exception1, Exception2, Exception3, Exception4, Exception5, Exception6 {}

@clementdessoude
Copy link
Contributor

Does this formatting looks good ?

class T {
  public void test(String one, String one, String one, String one, String one, String one) {

  }

  public void test(String one, String one, String one, String one, String one, String one) throws Exception, Exception, Exception {

  }

  public void test(String one, String one, String one, String one, String one, String one) throws Exception {

  }

  public void test(String one, String one) throws Exception, Exception, Exception, Exception {

  }

  public void test() throws Exception1, Exception2, Exception3, Exception4, Exception5, Exception6 {

  }

  public void test(String one, String one) throws Exception {

  }
}

into

class T {

  public void test(
    String one,
    String one,
    String one,
    String one,
    String one,
    String one
  ) {}

  public void test(
    String one,
    String one,
    String one,
    String one,
    String one,
    String one
  ) throws Exception, Exception, Exception {}

  public void test(
    String one,
    String one,
    String one,
    String one,
    String one,
    String one
  ) throws Exception {}

  public void test(String one, String one) throws
    Exception,
    Exception,
    Exception,
    Exception {}

  public void test() throws
    Exception1,
    Exception2,
    Exception3,
    Exception4,
    Exception5,
    Exception6 {}

  public void test(String one, String one) throws Exception {}
}

@NoahTheDuke
Copy link

Outside perspective: Black (the Python formatter) does it like the prettier 0.4.0 example, so even if it doesn’t look great, it has precedence in the “code formatting” community.

@clementdessoude clementdessoude self-assigned this Nov 18, 2019
@clementdessoude clementdessoude added the status: PR done The PR to fix this issue is done label Dec 1, 2019
@murdos
Copy link
Author

murdos commented Dec 3, 2019

Sorry @clement26695 for the (very) late reply...

The best option IMO is to keep throws on the same line as the (first) exception(s), as in Option 1, because it's more readable (you don't have to look after the throws keyword that might be "hidden" at the end of the previous line) :

public void test() throws Exception1, Exception2 {}

public void test() 
    throws Exception1, Exception2, Exception3, Exception4, Exception5 {}

public void test(
    String one,
    String one,
    String one,
    String one,
    String one,
    String one
  ) throws Exception1, Exception2, Exception3, Exception4, Exception5 {}

This is consistent with how IntelliJ and Spotless (so probably Eclipse too) wrap the throws keyword:
image

I'm just puzzled on the way to handle a long list of exceptions:

Option A

public void test() 
    throws Exception1, Exception2, Exception3, Exception4, Exception5, Exception6,
    Exception7 {}

public void test(
    String one,
    String one,
    String one,
    String one,
    String one,
    String one
  ) throws Exception1, Exception2, Exception3, Exception4, Exception5, Exception6,
    Exception7 {}

or

Option B

public void test() 
    throws Exception1, 
    Exception2, 
    Exception3, 
    Exception4, 
    Exception5, 
    Exception6,
    Exception7 {}

public void test(
    String one,
    String one,
    String one,
    String one,
    String one,
    String one
  ) throws Exception1, 
    Exception2, 
    Exception3, 
    Exception4, 
    Exception5, 
    Exception6,
    Exception7 {}

Option B is probably more consistent with the Prettier style.

@benjavalero
Copy link

Just to confirm. The throws would have a double indent? If not, we would have the body of the method at the same indent label, for instance (with 0.4.0):

public OAuth1AccessToken getAccessToken(OAuth1RequestToken requestToken, String oauthVerifier)
    throws AuthenticationException {
    try {
        [...]
    }
}

@murdos
Copy link
Author

murdos commented Dec 8, 2019

Just to confirm. The throws would have a double indent? If not, we would have the body of the method at the same indent label, for instance (with 0.4.0):

In my proposal, it would have a simple indent. I don't think it's a problem if we the same indent level for body and throws

@jhaber
Copy link
Contributor

jhaber commented Dec 12, 2019

👍 the Option B style proposed here looks good to me #286 (comment)

@digorydoo
Copy link

How about this:

public void one() throws Exc {
    doIt();
}

public void two(
    int someLongishArgument,
    int someOtherArgument
) throws Exc1, Exc2 {
    doIt();
}

public void three()
throws
    SomeWeirdExcpetion,
    SomeOtherWeirdException,
    SomeRatherRareException,
    SomeObscureException
{
    doIt();
}

public void four(
    int someLongishArgument,
    int someOtherArgument
) throws
    SomeWeirdExcpetion,
    SomeOtherWeirdException,
    SomeRatherRareException,
    SomeObscureException
{
    doIt();
}

@clementdessoude
Copy link
Contributor

Hi @digorydoo, thank you for your feedbacks !

I definitively need to look at this. I'll try to work on this next week

@MichielBugherJelli
Copy link

MichielBugherJelli commented May 6, 2020

I'm sure this discussion has happened before, but I'm not finding it anywhere.
I'm unclear as to why it puts every single element on a new line once it reaches the print width.

Given this:

public void test() throws Exception1, Exception2, Exception3, Exception4, Exception5, Exception6 {}

Why can't this be done?

public void test() throws Exception1, Exception2, Exception3, 
    Exception4, Exception5, Exception6 {

    //code here
}

OR

public void test() 
    throws Exception1, Exception2, Exception3, Exception4, 
        Exception5, Exception6 
{
    //code here
}

Same thing with method parameters.

public void methodName(int longArgument1,
    int longArgument2, int longArgument3,
    int longArgument4, int longArgument5
)

OR

public void methodName(
    int longArgument1, int longArgument2, 
    int longArgument3, int longArgument4, 
    int longArgument5
)

Essentially this: prettier/prettier#6573 but for Java

@sanjayrawat1
Copy link

@MichielBugherJelli suggestion looks pretty good to me. Why keep every element to next line once reached print width. A smart wrap respecting print width value would be perfect.

@jhaber
Copy link
Contributor

jhaber commented Nov 13, 2020

Prettier is an opinionated formatter, and it seems like one of those opinions is that either all elements in a group should line break, or none should (and prettier-java inherits this behavior from prettier since it's a prettier plugin). The example above might look tidy because all of the arguments are the same type and happen to align nicely, but it quickly becomes text soup when that's not the case, for example:

public void doRemoteFetch(RemoteDataFetcher remoteDataFetcher,
    JsonDeserializer jsonDeserializer, CustomHeadersProvider customHeadersProvider,
    ResponseSignatureVerifier responseSignatureVerifier, ErrorHandlingStrategy errorHandlingStrategy
)

vs.

public void doRemoteFetch(
    RemoteDataFetcher remoteDataFetcher,
    JsonDeserializer jsonDeserializer, 
    CustomHeadersProvider customHeadersProvider,
    ResponseSignatureVerifier responseSignatureVerifier, 
    ErrorHandlingStrategy errorHandlingStrategy
)

But like I said before, this style is just an opinion so there's not much use in debating which way is objectively "right". However, this opinion is pretty much at the core of prettier and unlikely to change. If you don't like prettier's opinions, google-java-format might be of interest, it's another formatter that's a bit less opinionated and also has IDE plugins available.

@jtkiesel jtkiesel linked a pull request Jan 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants