Using case objects in place of case classes having no members #11

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@nurkiewicz
Contributor

nurkiewicz commented Apr 3, 2013

Idiomatic Scala uses case objects in favor of empty case class.

@hekonsek

This comment has been minimized.

Show comment Hide comment
@hekonsek

hekonsek Apr 3, 2013

Contributor

+1 for Tomek's pull request

Contributor

hekonsek commented Apr 3, 2013

+1 for Tomek's pull request

@hekonsek

This comment has been minimized.

Show comment Hide comment
@hekonsek

hekonsek Apr 3, 2013

Contributor

I'm just not sure whether do we really need to bind the variable in the following line in TransactionSynchronizationManagerTests:

case e@AfterCommitEvent => afterEvent = e

What is is purpose of this change?

Contributor

hekonsek commented Apr 3, 2013

I'm just not sure whether do we really need to bind the variable in the following line in TransactionSynchronizationManagerTests:

case e@AfterCommitEvent => afterEvent = e

What is is purpose of this change?

@nurkiewicz

This comment has been minimized.

Show comment Hide comment
@nurkiewicz

nurkiewicz Apr 3, 2013

Contributor

Do you suggest:

case AfterCommitEvent => afterEvent = AfterCommitEvent

Indeed, I was misled by previous case. I'll commit this to pull branch if you agree.

Contributor

nurkiewicz commented Apr 3, 2013

Do you suggest:

case AfterCommitEvent => afterEvent = AfterCommitEvent

Indeed, I was misled by previous case. I'll commit this to pull branch if you agree.

@hekonsek

This comment has been minimized.

Show comment Hide comment
@hekonsek

hekonsek Apr 3, 2013

Contributor

Yeah, this binding is a little bit confusing in this context :) .

Actually with your case object under the hood we could even consider using boolean variables in this test. For example:

test("Should match multiple events.") {
    var beforeEventReceived: Boolean = false
    var afterEventReceived: Boolean = false
    transactional() {
      status => {
        TransactionSynchronizationManager.registerSynchronization {
          case BeforeCommitEvent => beforeEventReceived = true
          case AfterCommitEvent => afterEventReceived = true
        }
      }
    }
    beforeEventReceived should be (true)
    afterEventReceived should be (true)
}

What do you think?

Contributor

hekonsek commented Apr 3, 2013

Yeah, this binding is a little bit confusing in this context :) .

Actually with your case object under the hood we could even consider using boolean variables in this test. For example:

test("Should match multiple events.") {
    var beforeEventReceived: Boolean = false
    var afterEventReceived: Boolean = false
    transactional() {
      status => {
        TransactionSynchronizationManager.registerSynchronization {
          case BeforeCommitEvent => beforeEventReceived = true
          case AfterCommitEvent => afterEventReceived = true
        }
      }
    }
    beforeEventReceived should be (true)
    afterEventReceived should be (true)
}

What do you think?

@nurkiewicz

This comment has been minimized.

Show comment Hide comment
@nurkiewicz

nurkiewicz Apr 3, 2013

Contributor

Looks cleaner, however BeforeCommitEvent actually conveys some information (readOnly flag), so maybe:

  test("Should match multiple events.") {
    var beforeEvent: Option[BeforeCommitEvent] = None
    var afterEventReceived = false
    transactional() {
      status => {
        TransactionSynchronizationManager.registerSynchronization {
          case e: BeforeCommitEvent => beforeEvent = Some(e)
          case AfterCommitEvent => afterEventReceived = true
        }
      }
    }
    beforeEvent should be (Some(BeforeCommitEvent(false)))
    afterEventReceived should be (true)
  }

What I don't like is the lack of symmetry - your suggestion looks better from that perspective.

Contributor

nurkiewicz commented Apr 3, 2013

Looks cleaner, however BeforeCommitEvent actually conveys some information (readOnly flag), so maybe:

  test("Should match multiple events.") {
    var beforeEvent: Option[BeforeCommitEvent] = None
    var afterEventReceived = false
    transactional() {
      status => {
        TransactionSynchronizationManager.registerSynchronization {
          case e: BeforeCommitEvent => beforeEvent = Some(e)
          case AfterCommitEvent => afterEventReceived = true
        }
      }
    }
    beforeEvent should be (Some(BeforeCommitEvent(false)))
    afterEventReceived should be (true)
  }

What I don't like is the lack of symmetry - your suggestion looks better from that perspective.

@hekonsek

This comment has been minimized.

Show comment Hide comment
@hekonsek

hekonsek Apr 3, 2013

Contributor

The purpose of this partocular test is to validate that multiple events can be caught by single registration, so I personally would prefer clean boolean variables over strict testing here.

Contributor

hekonsek commented Apr 3, 2013

The purpose of this partocular test is to validate that multiple events can be caught by single registration, so I personally would prefer clean boolean variables over strict testing here.

@nurkiewicz

This comment has been minimized.

Show comment Hide comment
@nurkiewicz

nurkiewicz Apr 4, 2013

Contributor

If the test only makes sure given events were triggered (moreover order is not important), I agree, Boolean is enough and more expressive. I'll rewrite it later today.

Contributor

nurkiewicz commented Apr 4, 2013

If the test only makes sure given events were triggered (moreover order is not important), I agree, Boolean is enough and more expressive. I'll rewrite it later today.

@poutsma

This comment has been minimized.

Show comment Hide comment
@poutsma

poutsma Apr 4, 2013

Contributor

Looks good to me. Thanks for contributing! I will merge as soon as you're done with it.

Contributor

poutsma commented Apr 4, 2013

Looks good to me. Thanks for contributing! I will merge as soon as you're done with it.

@nurkiewicz

This comment has been minimized.

Show comment Hide comment
@nurkiewicz

nurkiewicz Apr 4, 2013

Contributor

Done. Unfortunately case classes and case objects are handled slightly different in pattern matching:

case _: BeforeCommitEvent => beforeEventReceived = true
case AfterCommitEvent => afterEventReceived = true
Contributor

nurkiewicz commented Apr 4, 2013

Done. Unfortunately case classes and case objects are handled slightly different in pattern matching:

case _: BeforeCommitEvent => beforeEventReceived = true
case AfterCommitEvent => afterEventReceived = true
@hekonsek

This comment has been minimized.

Show comment Hide comment
@hekonsek

hekonsek Apr 4, 2013

Contributor

I personally prefer the following syntax for matching all classes.

case BeforeCommitEvent(_) => beforeEventReceived = true

But its up to the spring-scala users to decide how they would like to use the library.

I think that pull request is ready to merge in the current state :) .

Contributor

hekonsek commented Apr 4, 2013

I personally prefer the following syntax for matching all classes.

case BeforeCommitEvent(_) => beforeEventReceived = true

But its up to the spring-scala users to decide how they would like to use the library.

I think that pull request is ready to merge in the current state :) .

poutsma pushed a commit that referenced this pull request Apr 5, 2013

Arjen Poutsma
Merged pull request #11 from nurkiewicz/case-object
* case-object:
  Use case objects instead of case classes
@poutsma

This comment has been minimized.

Show comment Hide comment
@poutsma

poutsma Apr 5, 2013

Contributor

Merged. Thanks for contributing!

Contributor

poutsma commented Apr 5, 2013

Merged. Thanks for contributing!

@poutsma poutsma closed this Apr 5, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment