-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 #2 is fine with me.
Marlon On 8/26/11 12:03 PM, Franklin, Matthew B. wrote: > I looked through the rave-portal java code and for the most part, it looks > like everyone is starting to keep consistent coding practices. I think this > is a good sign that the community is beginning to mature. > > There are few things I noticed and commented on in the code, but there was > one thing I thought needed to be brought to the list. Some of the Model > objects define the names of the queries as constant Strings that are then > used in the annotations and the repositories. Others just use inline > strings. I think we need to pick one of the following approaches and make > the necessary changes to keep consistency: > > 1) Use inline Strings > 2) Define constant Strings in the model classes > 3) Create a Constants class that defines all strings used in named queries > > If no one cares which one we use, I will implement option 2. > > -Matt > > >> -----Original Message----- >> From: [email protected] [mailto:[email protected]] >> Sent: Friday, August 26, 2011 11:52 AM >> To: [email protected] >> Subject: svn commit: r1162146 - in /incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal: repository/impl/ service/impl/ >> web/controller/ >> >> Author: mfranklin >> Date: Fri Aug 26 15:52:03 2011 >> New Revision: 1162146 >> >> URL: http://svn.apache.org/viewvc?rev=1162146&view=rev >> Log: >> Added TODO comments and fixed minor inconsistencies >> >> Modified: >> incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/H2OpenJpaDial >> ect.java >> incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/JpaWidgetRep >> ository.java >> incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/TranslatedH2E >> xception.java >> incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/service/impl/DefaultPageServic >> e.java >> incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/web/controller/PageController. >> java >> incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/web/controller/WidgetStoreCo >> ntroller.java >> >> Modified: incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/H2OpenJpaDial >> ect.java >> URL: http://svn.apache.org/viewvc/incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/H2OpenJpaDial >> ect.java?rev=1162146&r1=1162145&r2=1162146&view=diff >> =========================================================== >> =================== >> --- incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/H2OpenJpaDial >> ect.java (original) >> +++ incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/H2OpenJpaDial >> ect.java Fri Aug 26 15:52:03 2011 >> @@ -32,7 +32,10 @@ import org.springframework.orm.jpa.vendo >> * >> * @author CARLUCCI >> */ >> + >> +//TODO: Move this class to commons >> public class H2OpenJpaDialect extends OpenJpaDialect { >> + private static final long serialVersionUID = 1L; >> /** >> * Translates an H2 database error into a Rave application Exception >> * >> >> Modified: incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/JpaWidgetRep >> ository.java >> URL: http://svn.apache.org/viewvc/incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/JpaWidgetRep >> ository.java?rev=1162146&r1=1162145&r2=1162146&view=diff >> =========================================================== >> =================== >> --- incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/JpaWidgetRep >> ository.java (original) >> +++ incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/JpaWidgetRep >> ository.java Fri Aug 26 15:52:03 2011 >> @@ -34,6 +34,8 @@ import org.slf4j.Logger; >> import org.slf4j.LoggerFactory; >> import org.springframework.stereotype.Repository; >> >> +import static org.apache.rave.persistence.jpa.util.JpaUtil.getSingleResult; >> + >> @Repository >> public class JpaWidgetRepository extends AbstractJpaRepository<Widget> >> implements WidgetRepository { >> >> @@ -126,10 +128,7 @@ public class JpaWidgetRepository extends >> // url is a unique field, so no paging needed >> query.setParameter(Widget.PARAM_URL, widgetUrl); >> final List<Widget> resultList = query.getResultList(); >> - if (resultList.isEmpty()) { >> - return null; >> - } >> - return resultList.get(0); >> + return getSingleResult(resultList); >> } >> >> /** >> @@ -140,6 +139,7 @@ public class JpaWidgetRepository extends >> * @param pageSize maximum number of items to be returned >> * @return valid list of widgets, can be empty >> */ >> + //TODO: Make this generic and move to JpaUtil in commons >> protected List<Widget> getPagedResult(TypedQuery<Widget> query, int >> offset, int pageSize) { >> if (pageSize >= LARGE_PAGESIZE) { >> log.warn("Requesting potentially large resultset of Widgets. >> Pagesize is >> {}", pageSize); >> >> Modified: incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/TranslatedH2E >> xception.java >> URL: http://svn.apache.org/viewvc/incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/TranslatedH2E >> xception.java?rev=1162146&r1=1162145&r2=1162146&view=diff >> =========================================================== >> =================== >> --- incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/TranslatedH2E >> xception.java (original) >> +++ incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/repository/impl/TranslatedH2E >> xception.java Fri Aug 26 15:52:03 2011 >> @@ -22,6 +22,7 @@ import org.springframework.dao.DataAcces >> * >> * @author ACARLUCCI >> */ >> +//TODO: Move this class to commons >> public class TranslatedH2Exception extends DataAccessException { >> private int errorCode; >> private String error; >> >> Modified: incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/service/impl/DefaultPageServic >> e.java >> URL: http://svn.apache.org/viewvc/incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/service/impl/DefaultPageServic >> e.java?rev=1162146&r1=1162145&r2=1162146&view=diff >> =========================================================== >> =================== >> --- incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/service/impl/DefaultPageServic >> e.java (original) >> +++ incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/service/impl/DefaultPageServic >> e.java Fri Aug 26 15:52:03 2011 >> @@ -190,31 +190,6 @@ public class DefaultPageService implemen >> target.getRegionWidgets().add(newPosition, widget); >> } >> >> - private static <T> T getFromRepository(long id, Repository<T> repo) { >> - T object = repo.get(id); >> - if (object == null) { >> - throw new IllegalArgumentException("Could not find object of >> given id >> in " + repo.getClass().getSimpleName()); >> - } >> - return object; >> - } >> - >> - private static void updateRenderSequences(List<RegionWidget> >> regionWidgets) { >> - int count = 0; >> - for (RegionWidget widget : regionWidgets) { >> - widget.setRenderOrder(count); >> - count++; >> - } >> - } >> - >> - private static RegionWidget findRegionWidgetById(Long id, >> List<RegionWidget> regionWidgets) { >> - for (RegionWidget widget : regionWidgets) { >> - if (widget.getId().equals(id)) { >> - return widget; >> - } >> - } >> - throw new IllegalArgumentException("Invalid RegionWidget ID"); >> - } >> - >> private Page addNewPage(User user, String pageName, String >> pageLayoutCode) { >> PageLayout pageLayout = >> pageLayoutRepository.getByPageLayoutCode(pageLayoutCode); >> >> @@ -238,7 +213,8 @@ public class DefaultPageService implemen >> >> return page; >> } >> - >> + >> + //TODO: If there is a reason why this is annotated @Transactional when >> the calling public method is @Transactional, note it in comments >> @Transactional(readOnly = false) >> private void updatePageRenderSequences(List<Page> pages) { >> if (pages != null && !pages.isEmpty()) { >> @@ -287,5 +263,30 @@ public class DefaultPageService implemen >> updatePageRenderSequences(pages); >> >> return movingPage; >> - } >> + } >> + >> + private static <T> T getFromRepository(long id, Repository<T> repo) { >> + T object = repo.get(id); >> + if (object == null) { >> + throw new IllegalArgumentException("Could not find object of >> given id >> in " + repo.getClass().getSimpleName()); >> + } >> + return object; >> + } >> + >> + private static void updateRenderSequences(List<RegionWidget> >> regionWidgets) { >> + int count = 0; >> + for (RegionWidget widget : regionWidgets) { >> + widget.setRenderOrder(count); >> + count++; >> + } >> + } >> + >> + private static RegionWidget findRegionWidgetById(Long id, >> List<RegionWidget> regionWidgets) { >> + for (RegionWidget widget : regionWidgets) { >> + if (widget.getId().equals(id)) { >> + return widget; >> + } >> + } >> + throw new IllegalArgumentException("Invalid RegionWidget ID"); >> + } >> } >> \ No newline at end of file >> >> Modified: incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/web/controller/PageController. >> java >> URL: http://svn.apache.org/viewvc/incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/web/controller/PageController. >> java?rev=1162146&r1=1162145&r2=1162146&view=diff >> =========================================================== >> =================== >> --- incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/web/controller/PageController. >> java (original) >> +++ incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/web/controller/PageController. >> java Fri Aug 26 15:52:03 2011 >> @@ -49,6 +49,7 @@ public class PageController { >> private PageService pageService; >> private UserService userService; >> >> + //TODO (RAVE-99) Figure out a better way to register script blocks so >> that >> we don't have to have this cross package dep >> @Autowired >> private OpenSocialEnvironment openSocialEnvironment; >> >> >> Modified: incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/web/controller/WidgetStoreCo >> ntroller.java >> URL: http://svn.apache.org/viewvc/incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/web/controller/WidgetStoreCo >> ntroller.java?rev=1162146&r1=1162145&r2=1162146&view=diff >> =========================================================== >> =================== >> --- incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/web/controller/WidgetStoreCo >> ntroller.java (original) >> +++ incubator/rave/trunk/rave- >> portal/src/main/java/org/apache/rave/portal/web/controller/WidgetStoreCo >> ntroller.java Fri Aug 26 15:52:03 2011 >> @@ -112,6 +112,7 @@ public class WidgetStoreController { >> * @param model {@link Model} >> * @return the view name of the Add new Widget form >> */ >> + //TODO: Change the value of this request mapping so that it follows the >> pattern /store/widget/add >> @RequestMapping(method = RequestMethod.GET, value = "addwidget") >> public String viewAddWidgetForm(Model model) { >> final Widget widget = new Widget(); >> @@ -127,6 +128,9 @@ public class WidgetStoreController { >> * @param model {@link Model} >> * @return if successful the view name of the widget, otherwise the form >> */ >> + /*TODO: Change the value of this request mapping so that it follows the >> pattern /store/widget/add >> + TODO: The value can be the same as the GET action as you are mapping >> based on method & name >> + */ >> @RequestMapping(method = RequestMethod.POST, value = >> "doaddwidget") >> public String viewAddWidgetResult(@ModelAttribute Widget widget, >> BindingResult results, >> Model model) { >> > -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.16 (Darwin) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJOV8dvAAoJEEfVXEODPFIDRDQIAJZJDj4bCEVsbopuhBc1y4qt swvu1JScuy+mUY++xAxaaGmKAo/Ikdy9DQ9HIirpE1Aus4mm3SR8jPAuOECyXPWQ EXt8YiNd2v+erp5x0mblAWwdOc79cJ02tchcJgJFjEoyL5/CVKxD2ZepxJcfdmO/ 6ofUNdDDH3b769+PF71YQ4OWmnBJPqUL6sKG1A3FBmDZWkplRjh1zWrLDYcsegE7 QGL2WUgk0SCxEovF7rXqutvtb2ShvslpDUyv20sSTj6jASUXytcfu02nyraDYgl1 anbxuFMnxQzsRUeh3woiMe1iM1cU8tqS2uL2vdv8mJP1ImOACPvOjkdF4aC1WpA= =rGsN -----END PGP SIGNATURE-----
