Author: nbelaevski Date: 2009-03-25 15:04:18 -0400 (Wed, 25 Mar 2009) New Revision: 13198 Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Album.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Comment.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Image.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/MetaTag.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Shelf.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/User.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/AlbumAction.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/Constants.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/IAlbumAction.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/IImageAction.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/ImageAction.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/SearchAction.java trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/ShelfAction.java Log: Code review results committed Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Album.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Album.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Album.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -73,6 +73,7 @@ @NotNull @NotEmpty @Length(min = 3) + //TODO nick - add maxLength validation here and everywhere? private String name; @Column(length = 1024) @@ -80,6 +81,7 @@ @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "ALBUM_USER_ID", referencedColumnName = "USER_ID") + //TODO nick - add @NotNull? private User owner; @OneToMany(cascade = CascadeType.ALL, mappedBy = "album") @@ -99,6 +101,7 @@ private List<Image> unvisitedImages = new ArrayList<Image>(); @Transient + //TODO nick == unvisitedImages.size private Long countUnvisitedImages; @OneToOne(cascade = CascadeType.ALL, optional = true) @@ -175,6 +178,7 @@ throw new IllegalArgumentException("Null image!"); } if (image.getAlbum() != null && !this.equals(image.getAlbum())) { + //TODO nick - use removeImage() image.getAlbum().getImages().remove(image); } image.setAlbum(this); @@ -191,6 +195,7 @@ if (image == null) { throw new IllegalArgumentException("Null image"); } + //TODO nick - check if image is in the current album image.setAlbum(null); images.remove(image); } @@ -218,6 +223,8 @@ } public int getIndex(Image image) { + //TODO nick - use indexOf + //TODO nick - check images == null? for (int i = 0; i < this.images.size(); i++) { if (this.images.get(i).equals(image)) { return i; @@ -231,6 +238,7 @@ return coveringImage; } if (images != null && images.size() > 0) { + //TODO nick - i suggest we use random images coveringImage = images.get(0); return images.get(0); } else { Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Comment.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Comment.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Comment.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -56,6 +56,7 @@ @ManyToOne @JoinColumn(name = "IMAGE_COMMENT_ID", referencedColumnName = "IMAGE_ID") + //TODO nick - add @NotNull private Image image; @ManyToOne(fetch = FetchType.LAZY) Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Image.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Image.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Image.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -88,6 +88,7 @@ @Length(min = 3) private String name; @Transient + //TODO nick - remove this private Boolean covering; @Column(length = 1024, nullable = false) @@ -118,6 +119,7 @@ private int height; + //TODO nick - should be changed to int private double size; private int width; @@ -234,6 +236,7 @@ if (comment == null) { throw new IllegalArgumentException("Null comment"); } + //TODO nick - check that comments belongs to "this" image comment.setImage(null); comments.remove(comment); } @@ -256,7 +259,13 @@ s.append(tag.getTag()).append(", "); } - return s.substring(0, s.length() - 2); + //TODO nick - changed by nick + if (s.length() >= 2) { + s.delete(s.length() - 2, s.length()); + } + + String string = s.toString(); + return string; } public void setMeta(String meta) { Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/MetaTag.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/MetaTag.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/MetaTag.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -52,6 +52,7 @@ @NotNull @NotEmpty @Length(min = 3) + //TODO nick - meta tags should not contain ',', add constraint private String tag; @ManyToMany(mappedBy = "imageTags") Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Shelf.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Shelf.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/Shelf.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -30,6 +30,8 @@ @Name("shelf") @Scope(ScopeType.CONVERSATION) @Table(name = "shelfs") +//TODO nick - rename table to "shelves" +//TODO nick - should we store this in conversation? public class Shelf implements Serializable { private static final long serialVersionUID = -7042878411608396483L; @@ -64,6 +66,7 @@ private Date created; @Transient + //TODO nick - == unvisitedImages.size() private Long countUnvisitedImages; /** @@ -129,6 +132,7 @@ if (album == null) throw new IllegalArgumentException("Null child!"); if (album.getShelf() != null) + //TODO nick - use removeChildAlbum() album.getShelf().getAlbums().remove(album); album.setShelf(this); albums.add(album); @@ -137,6 +141,7 @@ public void removeChildAlbum(Album album) { if (album == null) throw new IllegalArgumentException("Null child!"); + //TODO nick - check that shelf is "this" album.setShelf(null); albums.remove(album); } @@ -202,6 +207,7 @@ } public Album getFirstAlbum() { + //TODO nick - use random album for cover? if (this.albums.size() > 0) { return this.albums.get(0); } Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/User.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/User.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/domain/User.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -99,6 +99,7 @@ @Column(length = 255, nullable = false) @NotNull @NotEmpty + //TODO nick - use e-mail validator @Pattern(regex=".+@.+\\.[a-z]+", message="Not valid e-mail") private String email; @@ -120,6 +121,7 @@ @NotNull private Sex sex; + //TODO nick - this should be persistent, right? private Boolean doNotShowMail; private Boolean informAboutNews; @@ -199,10 +201,12 @@ //---------------------------Business methods + //TODO nick - do we really need this? we can just aggregate data from all albums public void addAlbum(Album album) { if (album == null) { throw new IllegalArgumentException("Null album!"); - } + } + //TODO nick - remove album from the previous user? album.setOwner(this); albums.add(album); } @@ -211,6 +215,7 @@ if (album == null) { throw new IllegalArgumentException("Null album"); } + //TODO nick - check if owner is current user? album.setOwner(null); albums.remove(album); } @@ -219,6 +224,7 @@ if (shelf == null) { throw new IllegalArgumentException("Null shelf!"); } + //TODO nick - remove shelf from the previous user? shelf.setOwner(this); shelfs.add(shelf); } @@ -227,7 +233,9 @@ if (shelf == null) { throw new IllegalArgumentException("Null shelf"); } + //TODO nick - check if owner is current user? shelf.setOwner(null); + //TODO nick - why to remove albums? for(Album a : shelf.getAlbums()){ removeAlbum(a); } Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/AlbumAction.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/AlbumAction.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/AlbumAction.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -43,6 +43,8 @@ @In(value="entityManager") EntityManager em; + + //TODO nick - do we need @Out here? @In @Out private User user; @@ -79,16 +81,20 @@ } public Long getCountUnvisitedImages(Album album){ + //TODO nick - use Shelf/Album.unvisitedImages return (Long)em.createQuery("select count(i) from Image i where i.album=:album and i.created >= :date").setParameter("album", album).setParameter("date", getDate()).getSingleResult(); } public List<Image> getUnvisitedImages(Album album){ + //TODO nick - use Shelf/Album.unvisitedImages return (List<Image>)em.createQuery("from Image i where i.album=:album and i.created >= :date").setParameter("album", album).setParameter("date", getDate()).getResultList(); } private Date getDate() { + //TODO nick - use Calendar.getInstance() Calendar c = new GregorianCalendar(); c.add(Calendar.DAY_OF_YEAR, -15); + //TODO nick - return c.getTime() return c.getInstance().getTime(); } } Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/Constants.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/Constants.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/Constants.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -20,7 +20,8 @@ */ package org.richfaces.realworld.service; - +//TODO nick - review constants and remove old ones +//TODO nick - separate view and service layer constants public class Constants { public static final String ERROR_ID = "mainform:error"; Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/IAlbumAction.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/IAlbumAction.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/IAlbumAction.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -36,6 +36,7 @@ public void editAlbum(Album album, boolean isFlushNeeded); + //TODO nick - explicit flush doesn't seem like a good solution public abstract void flush(); public Long getCountUnvisitedImages(Album album); Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/IImageAction.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/IImageAction.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/IImageAction.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -31,6 +31,8 @@ @Local public interface IImageAction { + //TODO nick - remove isFlushNeeded + public abstract void deleteImage(Image image, boolean isFlushNeeded); public abstract void editImage(Image image, boolean isFlushNeeded); @@ -43,6 +45,7 @@ public abstract MetaTag getTagByName(String tag); + //TODO nick - rename to getPopularTags() public abstract List<MetaTag> popularTags(); public abstract List<MetaTag> getTagsLikeString(String suggest); Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/ImageAction.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/ImageAction.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/ImageAction.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -65,6 +65,7 @@ if (!image.containTag(s)) { MetaTag tag = getTagByName(s); if (tag == null) { + //TODO nick - how about concurrent creation of meta tags with the same name? tag = new MetaTag(); tag.setTag(s); } Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/SearchAction.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/SearchAction.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/SearchAction.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -18,6 +18,7 @@ @Stateless @AutoCreate @SuppressWarnings("unchecked") +//TODO add JavaDocs public class SearchAction implements ISearchAction { @In(value="entityManager") Modified: trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/ShelfAction.java =================================================================== --- trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/ShelfAction.java 2009-03-25 17:53:03 UTC (rev 13197) +++ trunk/test-applications/realworld2/ejb/src/main/java/org/richfaces/realworld/service/ShelfAction.java 2009-03-25 19:04:18 UTC (rev 13198) @@ -72,6 +72,7 @@ } public List<Shelf> getShelfs(User user) { + //TODO nick - preserve shelves ordering? Maybe use TreeSet with Comparator? Set<Shelf> sh = new HashSet<Shelf>(); List<Shelf> shelfs = em.createQuery("from Shelf s where s.shared=true") .getResultList(); @@ -83,14 +84,17 @@ } public Long getCountUnvisitedImages(Shelf shelf){ + //TODO nick - use data from user return (Long)em.createQuery("select count(i) from Image i where i.album.shelf=:shelf and i.uploaded > :date").setParameter("shelf", shelf).setParameter("date", getDate()).getSingleResult(); } public List<Image> getUnvisitedImages(Shelf shelf){ + //TODO nick - use data from user return (List<Image>)em.createQuery("from Image i where i.album.shelf=:shelf and i.uploaded > :date").setParameter("shelf", shelf).setParameter("date", getDate()).getResultList(); } private Date getDate() { + //TODO nick - copy-pasted from AlbumAction, see TODOs there Calendar c = new GregorianCalendar(); c.add(Calendar.DAY_OF_YEAR, -15); return c.getInstance().getTime();