OpenJPA
  1. OpenJPA
  2. OPENJPA-780

code review for DistributedStoreManager

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2.0
    • Fix Version/s: 1.3.0, 2.0.0-M2
    • Component/s: slice
    • Labels:
      None

      Description

      I am currently reviewing code, and this one piece of code stood out. It might not be a bad thing, but it just has a "funny smell". This is in DistributedStoreManager. There it gets a String[], which it then iterates over it to find an appropriate slice. But it looks like it either finds a slice and returns it, or finds a null and throws an exception, all on the first step of the for loop. So really, there is no for loop at all. This might be on purpose, but the code is just not as legible..

      ORIGINAL
      /**

      • Selects child StoreManager(s) where the given instance resides.
        */
        private StoreManager selectStore(OpenJPAStateManager sm, Object edata)
        Unknown macro: { String[] targets = findSliceNames(sm, edata); for (String target }

      expecting more like:

      String[] targets = ....
      if ( targets == null || targets.length == 0 ) {
      return null;
      }
      SliceStoreManager slice = lookup(targets[0]);
      if (slice == null) {
      throw new InternalException(_loc.get("wrong-slice", target, sm));
      }
      return slice;

        Activity

        Hide
        Michael Dick added a comment -

        Thanks very much for the patch Fernando!

        Show
        Michael Dick added a comment - Thanks very much for the patch Fernando!
        Hide
        Michael Dick added a comment -

        resolved the wrong issue, sorry.

        Show
        Michael Dick added a comment - resolved the wrong issue, sorry.
        Hide
        Michael Dick added a comment -

        Pinaki is a better candidate to determine whether the current behavior is correct. I'm not an expert on the slice code, but it looks like we should iterate on each target and return the first non-null slice.

        Show
        Michael Dick added a comment - Pinaki is a better candidate to determine whether the current behavior is correct. I'm not an expert on the slice code, but it looks like we should iterate on each target and return the first non-null slice.
        Hide
        Fernando Padilla added a comment -

        shrug... then maybe you should just close this bug now that slices is getting more attention.

        Show
        Fernando Padilla added a comment - shrug... then maybe you should just close this bug now that slices is getting more attention.
        Hide
        Fernando Padilla added a comment -

        I think we can close this one.

        Show
        Fernando Padilla added a comment - I think we can close this one.

          People

          • Assignee:
            Pinaki Poddar
            Reporter:
            Fernando Padilla
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development