8

Kind of a long title, but that is generally the question.

I want to know if you think its a good idea to do the following.

Instead of:

public void buyItem(int itemId, int buyerId) {
    if (itemId <= 0) {

        throw new IlleglArgumentException("itemId must be positive");
    }
    if (buyerId <= 0) {

        throw new IlleglArgumentException("buyerId must be positive");
    }
    // buy logic
}

I want to have something like:

@Defensive("isPositive(#itemId, #buyerId)")
public void buyItem(int itemId, int buyerId) {
    // buy logic
}

Do you think this is good/terrible/too fancy/too slow ? If you actually think its good I was thinking of using SpEL to implement it, does anyone have something better/lighter/faster in mind ?

Thanks,

Simeon
  • 7,582
  • 15
  • 64
  • 101

5 Answers5

3

It's not necessarily a bad thing however your simple case can well be solved with exception statements (rather than assertions which I initially mentioned).

It seems you're introducing your own set of annotations which other developers in your project must accept and adapt to. However, once you've introduced annotations, it seems like a regular Proxy approach would be a better candidate to solve the issue.

To sum up: The problem you described can easily be solved using standard java alternatives, although in a more complex case it might be justified (consider e.g., @Secured in spring-security), especially if you're developing your own framework.

Community
  • 1
  • 1
Johan Sjöberg
  • 47,929
  • 21
  • 130
  • 148
  • And it introduces another language inside the annotations. – Hauke Ingmar Schmidt Feb 17 '12 at 13:14
  • Actually this question got in my head exactly as a side effect of using `@Secured`. But since we already use `@Secured` for method level security it would be confusing to use it for defencive checks IMO, that's why I'm asking for ideas on doing it and since `@Secured` and `@PreAuthorize` use SpEL I thought thats the obvious choice. – Simeon Feb 17 '12 at 13:22
  • 1
    It's generally considered inappropriate to use `assert` for checking arguments. But you could add a method, say, `legalID` or `checkID` to do the checking concisely. – Tom Hawtin - tackline Feb 17 '12 at 13:26
  • 1
    @Simon, if you're developing a framewok used by other applications, it can be worth it. If used in this one project it probably adds more confusion and tends to be overengineering. Still, if you want to learn, have the devs on your side, and the time to implement it, sure, try it out. – Johan Sjöberg Feb 17 '12 at 13:27
  • 1
    @TomHawtin-tackline, yes I think you're right. I've updated the post and referenced an SO post debating the assert/exception issue. – Johan Sjöberg Feb 17 '12 at 13:29
2

I think it's

  • overengineering
  • breaking encapsulation, by relying on an external aspect (that could or couldn't be there) to maintain invariants of the object
  • confusing
  • not efficient
  • not adding any value

I like using Guava's Preconditions class for such checks. Moreover, such checks are part of the business logic, IMHO.

JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
  • Whether it is business logic or not is subjective (probably ... I'm not sure if there is a decisive definition), but still its inside the method :) and inside the method I feel better (maybe that's just me) if I don't read 10/20 lines of checks, which is normal if you're trying to be 100% safe. – Simeon Feb 17 '12 at 13:25
  • 1
    Guava's preconditions would reduce the number of lines, because it avoids the if blocks. If these checks are so big that they're disturbing, then put them in a private method and call this private method. That reduces the checks to just one line. And if you want to be 100% safe, then don't use assertions (which can be disabled - and are disabled by default IIRC), and don't use an aspect, which can be disabled, or incorrectly installed. – JB Nizet Feb 17 '12 at 13:29
  • 1
    @downvoter: you might disagree with my answer, but that doesn't make it not useful. Why the downvote? – JB Nizet Feb 17 '12 at 13:36
2

I think you meant annotations, not aspects. Aspects are normally external to the code they advice. In any case, you can accomplish the sort of validations you highlighted in your post with JSR 303. Case in point:

public void buyItem(@Min(value = 1) int itemId, @Min(value = 1) int buyerId) {
    // buy logic
}

There are currently two well known implementations of JSR 303:

Perception
  • 79,279
  • 19
  • 185
  • 195
1

It looks like validation. FYI, several frameworks already exist, check out this question and OVal for example.

I don't know about performances, but to me it is not overengineering: you keep only the logic code. I love working with that.

Community
  • 1
  • 1
Matthieu Napoli
  • 48,448
  • 45
  • 173
  • 261
0

I'd prefer the Java assertion approach.

  1. it's a standard approach so all (most?) Java programmers will understand it. Automated tooling etc. will be able to parse it as well.
  2. you don't have to develop anything yourself. I know it looks trivial, but these things have a habit of escalating.

Unless you can demonstrate that your approach is manifestly better or doing something different, I would use the tooling and APIs that Java provides as standard.

Matthieu Napoli
  • 48,448
  • 45
  • 173
  • 261
Brian Agnew
  • 268,207
  • 37
  • 334
  • 440
  • This is OK as long as you are sure it is [turned on](http://docs.oracle.com/javase/1.4.2/docs/guide/lang/assert.html#enable-disable) when you need it to be. – McDowell Feb 17 '12 at 15:53