Robin Rosenberg <robin.rosenberg@xxxxxxxxxx> wrote:
> måndagen den 6 oktober 2008 10.15.54 skrev Shawn O. Pearce:
> > Robin Rosenberg <robin.rosenberg@xxxxxxxxxx> wrote:
> > >
> > > /**
> > > + * @return a map with all objects referenced by a peeled ref.
> > > + */
> > > + public Map<AnyObjectId, List<Ref>> getAllRefsByPeeledObjectId() {
> >
> > Do we really want to promise List here? Can we make it just
> > Collection instead?
>
> Sure. Our promise is actually slightly better, it is Set, but Java doesn't have a suitable class for that.
java.util.Set? java.util.HashSet?
What am I missing?
> > > + for (Map.Entry<String,Ref> e : allRefs.entrySet()) {
> > > + Ref ref = e.getValue();
> >
> > I think this is cleaner:
> >
> > for (Ref ref : allRefs.values()) {
> >
> > as you never use the key.
>
> Yes. I was thinking it might be less efficient, but the JDK implementation looks quite well optimized in 1.6 at least
> so values() is slightly faster.
I wasn't concerned about performance, I was going for readability.
This loop is probably not performance critical as its done once
early when the PlotWalk starts. I just found that grabbing the
entrySet when all you care about is the values was odd.
> > List<Ref> nl = Collections.singletonList(ref);
> > List<Ref> ol = ret.put(target, nl);
> > if (ol != null) {
> > if (ol.size() == 1) {
> > nl = new ArrayList<Ref>(2);
> > nl.add(ol.get(0));
> > nl.add(ref);
> > ret.put(target, nl);
> > } else {
> > ol.add(ref)
> > ret.put(target, ol);
> > }
> > }
>
> ok, I guess one just has has to include the comment on why for the casual reader.
I'm probably too used to this pattern of "put, then test" because
I use it a lot when the odds of put returning null are very high.
Its an odd idiom. I think most developers wouldn't write it.
So I guess a comment of some sort is probably a good idea if you
chose to use this mess. ;-)
--
Shawn.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
