4
\$\begingroup\$

I'm going through the CoffeeScript book by Trevor Burnham, and I was curious as to what the best style is for a function I was writing.

In the book the author writes the function like so:

strToCoordinates = (input) ->
    halves = input.split(',')
    if halves.length is 2
        x = parseFloat halves[0]
        y = parseFloat halves[1]
        if !isInteger(x) or !isInteger(y)
            console.log "Each coordinate must be an integer."
        else if not inRange x - 1, y - 1
            console.log "Each coordinate must be between 1 and #{GRID_SIZE}."
        else
            {x, y}
    else
        console.log 'Input must be of the form `x, y`.'

I wrote my function like so:

strToCoordinates = (input) ->
    halves = input.split(',')
    if halves.length isnt 2
        console.log 'Please enter coordinates in format: x,y'; return
    [x, y] = (parseFloat s for s in halves)
    unless isInteger(x) and isInteger(y)
        console.log 'The coordinates must be integers.'; return
    if not inRange(x, y)
        console.log "Coordinates must be between 1 and #{GRID_SIZE}."; return
    {x, y}

Is my use of just if statements and using return to stop the flow if the check fails an okay style?

I like the fact that with this style the error messages are right after the check unlike with if/else, and also you don't have to have large sections indented.

\$\endgroup\$
4
\$\begingroup\$

Well I don't now coffeescript, but guess I can read it anyway :D

I think both are good and readable.
I don't know if you can add empty lines to the code. If you can:

strToCoordinates = (input) ->
    halves = input.split(',')
    if halves.length isnt 2
        console.log 'Please enter coordinates in format: x,y'; 
        return

    [x, y] = (parseFloat s for s in halves)
    unless isInteger(x) and isInteger(y)
        console.log 'The coordinates must be integers.'; 
        return

    if not inRange(x, y)
        console.log "Coordinates must be between 1 and #{GRID_SIZE}."; 
        return

    {x, y}

But this really is a minor arguable tweak (some will like it, some don't, I do).
Another point would be the "overuse" of returns. Functions are easier to read, if they do have lss return-statements. But Again, this usually applies to larger functions only.

\$\endgroup\$
3
\$\begingroup\$

I don't really like hiding the returns off the end of the line, so putting them on separate lines (or reverting to the original) would be preferable.

However, I'd also consider actually using postfix if. This would make the code read like this

strToCoordinates = (input) ->
    halves = input.split(',')
    return console.log 'Please enter coordinates in format: [x,y]' if halves.length isnt 2
    [x, y] = (parseFloat s for s in halves)
    return console.log 'The coordinates must be integers.' unless isInteger(x) and isInteger(y)
    return console.log "Coordinates must be between 1 and #{GRID_SIZE}." unless inRange(x, y)
    {x, y}

Under certain circumstances, postfix if can be buried at the end of the line. The presence of code straight after a return draws the eye to the conditions in this case. This would be my favoured way of writing the function as not only is it concise, it would be very easy to extend to extra conditions.

\$\endgroup\$
  • \$\begingroup\$ Don't you think one will need to read the whole console.log message before seeing the condition. It's a pre-condition, hiding if at the end of a line makes it hard to see. Also, isn't it a bit weird to have 3 return statement in a row followed by a statement without the return keyword? Having the possibility of making one liners doesn't mean you have to use them. \$\endgroup\$ – jackdbernier Feb 6 '14 at 2:23
  • \$\begingroup\$ I'd say the very sequence of returns makes it obvious to the reader that postfix ifs are in play. Think of it a different way: under what circumstances do you think postfix if should be used? Nearly every circumstance is harder to spot than putting a return at the front. \$\endgroup\$ – Julian Birch May 5 '14 at 21:36
  • \$\begingroup\$ Do you really need postfix ifs at all ? Lines should be short so that you don't have to move your eyes from left to right. Think of it another way, if you need to search for the ifs statements they should probably be somewhere else. Somewhere you don't need to search for them. Look this answer and tell me if it reads well codereview.stackexchange.com/a/2781/27048. You can clearly see all the paths, the ifs and the returns. Plus I don't need to move my eyes from left to right. \$\endgroup\$ – jackdbernier May 6 '14 at 17:16
1
\$\begingroup\$

I would agree with the author's style just because it's not the absolute case.

If the author wants to build on the example and introduce more cases like a Z coordinate it's an else if statement, but your function you would have to have another inner if statement to handle there being three coordinates.

 if isnt 2 and 3 
      ...
      if 2 
        ...
      elseif 3

In this example you wouldn't be adding to it anyway since it's an excerise, but if it's code you want to use in a project then there's no telling what new requirements you might add in.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.