From: Shawn O. Pearce <spearce@spearce.org>
Date: Mon, 06 Oct 2008 22:43:48 GMT
Subject: Re: [EGIT PATCH 3/6] Add a method to get refs by object Id
Message-ID: <20081006224348.GB13687@spearce.org>
In-Reply-To: <200810070037.53841.robin.rosenberg@dewire.com>

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> måndagen den 6 oktober 2008 10.15.54 skrev Shawn O. Pearce:
> > Robin Rosenberg <robin.rosenberg@dewire.com> 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.