This week we had a chance to review the Informative Workspace's web application: project overviewer which is currently displayed in two of the moniotors in CSDL. It aims at providing a general overview of each of the 4 projects we are doing in ICS 414.
Downloading and setting the project up was very straight forward. At first, verify doesn't pass becuase I am using a different version of PMD and Checkstyle. The team mentioned already mentioned that they are using different versions of some of the QAs so this is not surprising. After modifying the build files to refer to the correct version instead, verify passed.
The team asked us to try and displayed our own project in the project overviewer. This turns out to be a pretty straight forward task too. All that is required is changing a couple line of PageInfoWork.java to provide my hackystat account information and the subversion url.
Possible improvement:
1. Includes a troubleshooting section. I encountered PermGem error very often at first, and I have to shut down and restart tomcat whenever that happen. At the end, I took away other .war or .jar file from the tomcat webapp folder and that stopped happening again. And also problem with deploying and undeploying the war file for multiple times in a session. These problems might be common with user and providing a section to guide user through them would be helpful.
2. A count-down clock would be more useful than just displaying the milestone date in a line.
3. Documentations on how to modify the code to display a particular project should be included. It may not be obvious to all users that all we had to do is to change a couple lines in the PageInfoWork.java to bring up their own projects.
4. Documentation for changing the PMD, checkystat version number in the build.xml should also be included. Otherwises, some users may think that they have to upgrated their QA tools in order to build the project.
5. Even though this is just for review purpose, it would be nice to include everything in all the projects instead of having to try import our own projects in the Info-Work page.
6. The tree takes up too much space and I don't think it is necessary to list out all the files. For example, for our project, the .cs files are of more interests to the users. Just looking at all the .cs files would provide a good-enough overview of our project.
All in all, the project overview had a good start and I look forwards to seeing the final product .
Showing posts with label Code Review. Show all posts
Showing posts with label Code Review. Show all posts
Wednesday, April 2, 2008
Wednesday, March 19, 2008
iHacky code review
I am recently very fond of using facebook, so I was excited to try out the iHacky application which is a facebook application designed for developers to communicate and network among themselves.
Installation was very easy. Since I already have a facebook account, I simply followed the link given in their project page and with a simple click of button, the iHakcy application was added to my profile immediately. After trying it out, I decided to check out the source code and try to set up my own iHacky following the instructions on the Developer's guide. That turns out to be not as smooth as the installation. I faced some problem with setting up the Apache server. In the end, it took me a long time to set it up but I am still having error with the PHP and I understand why the team suggested us that we could skip installing it locally.
I am not familiar with PHP, but from the source code, they look pretty organized with plenty of comments. All in all, this application is still in early development phase and I see a lot of improvement in it.
I installed iHacky on Monday and everything was working fine, but as of right now, the iHacky application is down. Clicking on the tabs would lead me to an error pages, so I will give the suggestions based on what I remember from using iHacky on Monday.
In turn of the application's functionality and interface, I have the following suggestions:
1. Allow user to change the hackystat account detail.
2. Provides visual representation of ranking on the facebook profile page. For example, it could be done in an pyramid form like how "Who has the biggest brain" does. It would encourage people to use iHacky more often in order to get to the top.
3. Provides more informative details on the user's profile page. Right now, it is only displaying "My Last time stamp is".
4. Shows which of your friends are also using the application. Right now, I can do so by going to the iHacky development page. It could be just me but it literally took me a while before I remember that I can actually visit the application's homepage by clicking on the tiny link in the bottom of the page to see who is also using the application. Including the friends list in the iHacky homepage or in another tab would be helpful as it would encourage people to use the application if they know a lot of their friends are also using the applications.
5. Allow people to contact the developers directly. Maybe they could have the developers listed there. Facebook can show "I am online" if the user is logged on. So if user can see one of the developer is online, they can message he directly for questions.
Some other suggestions in term of documentation:
1. It would be nice to have a troubleshooting section. When I test my PHP5 installation for the first time, it didn't work. I followed the instruction to set up a test.php but then it only showed an error page.
2. The Wamp could be listed as one of the required components, with a note saying it is for easy installation of the PHP and other components.
All in all, this is still in early development phase and I really look forwards to seeing the end product. Being a Facebook addict I would surely use it everyday to network with other developers.
Wednesday, March 12, 2008
Visual Studio Sensor Code Review
Our classmates did a review of our project: Visual Studio Sensor for Hackystat. This is the project page and the source package is available here.
In general, I am glad to see that they did not have much problem with installing the sensor. The main problem encountered is failure to build the system due to null reference to NUnit. This problem was addressed promptly by my group partner, Jared by posting a modified release.
Also, I am not surprised to see that one of the biggest problem encountered was creating another instance of Visual Studio. This was also the problem that hindered us from writing test cases for the sensor.
Based on the review, some of the sensor data now collected are apparently not useful and that is exactly what we would like to find out from the review. Apparently, some DevEvents such as CloseFile and OpenFile are not of much interests to them.
In general, I am glad to see that they did not have much problem with installing the sensor. The main problem encountered is failure to build the system due to null reference to NUnit. This problem was addressed promptly by my group partner, Jared by posting a modified release.
Also, I am not surprised to see that one of the biggest problem encountered was creating another instance of Visual Studio. This was also the problem that hindered us from writing test cases for the sensor.
Based on the review, some of the sensor data now collected are apparently not useful and that is exactly what we would like to find out from the review. Apparently, some DevEvents such as CloseFile and OpenFile are not of much interests to them.
Wednesday, March 5, 2008
Ambient Team Code Review
The Ambient Device team gave a presentation on their current release for us to do a review of their code.
By following the installation and developer guide, I was able to check out and install the project. Installation was not difficult as I already have Hackystat installed and have a valid Hackystat account, so I don't have to go through setting up Hackystat again. Everything was fine until I try to run verify. According to the developer guide, it mentions that junit test should fail but the problem is it didn't mention which test should junit fail. So I am not sure if I am just having the error that the ambient team have or if I actually have additional problems that have to be fixed. In the end, I came across three major errors and spent a lot of time just to fix the first two. However, there is still an error: org.apache.xerces.dom.DocumentImpl.getXmlStandalone()Z that I didn't know how to be fix.
For the code itself, it was pretty organized and I don't see much problems with it but in term of documentation, it could have been better. Problems or areas that can be improved:
1. Even though the code is pretty organized, there is a lack of comments. There are comments for each class and methods but comments within methods are rare, which makes it harder for the user to understand the code.
2. The superclasses could have been more comprehensive instead of having only one method.
3. When the jar file is invoked, there is only a statement saying "LastBuild: Active" which is not helpful as it doesn't give the user any information whether it is actually communicating with the orb successfully or not.
4. For documentation, a troubleshooting section would be really helpful. Apparently, many of us are having the same junit errors that makes verify fail such as the Jaxb api problem and the javamail installation problem. So if they can provide solutions to these problems, it would make the developer guide more comprehensive and helps the user.
5. There should be instructions or references instructions to install Hackystat in their developer guide. It would be confusing for users who have no prior experience with Hackystat.
6. Another obvious problem is it takes too long for the orb to change color. This is a problem because during the wait for the orb to change color, the project situation could have been changed completely. For example, it could have gone from a failed build to a successful build again.
It was a fun experience to do this review as it gave me to chance to play with the ambient orb for the first time. The ambient team has surely got a good start with their project. There is big potential to this project. As mentioned above, I think their code is working well and the main problem is with the documentation and the color changing latency, both of which should not hard to fix as the latter could be fixed by acquiring the appropriate hardware. I look forwards to see what the ambient team is going to do with the Nabaztag as well.
By following the installation and developer guide, I was able to check out and install the project. Installation was not difficult as I already have Hackystat installed and have a valid Hackystat account, so I don't have to go through setting up Hackystat again. Everything was fine until I try to run verify. According to the developer guide, it mentions that junit test should fail but the problem is it didn't mention which test should junit fail. So I am not sure if I am just having the error that the ambient team have or if I actually have additional problems that have to be fixed. In the end, I came across three major errors and spent a lot of time just to fix the first two. However, there is still an error: org.apache.xerces.dom.DocumentImpl.getXmlStandalone()Z that I didn't know how to be fix.
For the code itself, it was pretty organized and I don't see much problems with it but in term of documentation, it could have been better. Problems or areas that can be improved:
1. Even though the code is pretty organized, there is a lack of comments. There are comments for each class and methods but comments within methods are rare, which makes it harder for the user to understand the code.
2. The superclasses could have been more comprehensive instead of having only one method.
3. When the jar file is invoked, there is only a statement saying "LastBuild: Active" which is not helpful as it doesn't give the user any information whether it is actually communicating with the orb successfully or not.
4. For documentation, a troubleshooting section would be really helpful. Apparently, many of us are having the same junit errors that makes verify fail such as the Jaxb api problem and the javamail installation problem. So if they can provide solutions to these problems, it would make the developer guide more comprehensive and helps the user.
5. There should be instructions or references instructions to install Hackystat in their developer guide. It would be confusing for users who have no prior experience with Hackystat.
6. Another obvious problem is it takes too long for the orb to change color. This is a problem because during the wait for the orb to change color, the project situation could have been changed completely. For example, it could have gone from a failed build to a successful build again.
It was a fun experience to do this review as it gave me to chance to play with the ambient orb for the first time. The ambient team has surely got a good start with their project. There is big potential to this project. As mentioned above, I think their code is working well and the main problem is with the documentation and the color changing latency, both of which should not hard to fix as the latter could be fixed by acquiring the appropriate hardware. I look forwards to see what the ambient team is going to do with the Nabaztag as well.
Sunday, December 9, 2007
MyIsern-Onyx-2.0 Review
Team reviewed: Onyx
Our team decided to break up the review into four parts and I was given the task to review their web application.
Installation:
Their distribution package is easily found on their hosting site. Downloading and extracting the file was easy and smooth. Installation went well as they have detail instructions on their wiki page and also in the readme file that come in the distribution package. After executing ant -f tomcat.build.xml, I was ready to navigate their site.
Web Design:
They done a good job with the login page. The login page is simple and very informative. Even before logging in, the user can get a comprehensive view of what the page does as a detail description is listed there.
They choose a very nice template which looks professional. The black background stands out nicely. However, as Professor Johnson mentioned during the presentation, the black background and the grey texts in the main body might be hard to see when projected on a projector. Although no
The color scheme was also very good. The white title is catchy against the black background. If the color of the sidebar, which is currently green, can match with that of the main title bar, everything should look even more organized.
Screen Real Estate:
On the whole, team Onyx did a good job in term of screen real estate. The space allocated for the main body text is wide enough. However, when the window is reduced to less than 1/2 of the monitor space, the side bars cannot be show and some of the main body text is also not visible. I have to scroll across the page in order to see all the text.
Quite often, I have to scroll down the page to see some content and I think that can be improved by several ways:
1. Make the title smaller. It is catchy now but it takes up quite a lot of space.
2. Put the Logout Sign on the same level as the title or on the main navigation bar. Right now, it is occupying a separate row at the top.
3. They are too much white space inside the sidebars. Minimizing those white space might be a good idea.
Overall:
I tried their webpage on both Internet Explorer and Firefox and both worked well. Therefore, their website is user friendly regardless of the browser the user is using. Overall, this is a very well designed webpage and as a user, I had an easy time navigating it.
Our team decided to break up the review into four parts and I was given the task to review their web application.
Installation:
Their distribution package is easily found on their hosting site. Downloading and extracting the file was easy and smooth. Installation went well as they have detail instructions on their wiki page and also in the readme file that come in the distribution package. After executing ant -f tomcat.build.xml, I was ready to navigate their site.
Web Design:
They done a good job with the login page. The login page is simple and very informative. Even before logging in, the user can get a comprehensive view of what the page does as a detail description is listed there.
They choose a very nice template which looks professional. The black background stands out nicely. However, as Professor Johnson mentioned during the presentation, the black background and the grey texts in the main body might be hard to see when projected on a projector. Although no
The color scheme was also very good. The white title is catchy against the black background. If the color of the sidebar, which is currently green, can match with that of the main title bar, everything should look even more organized.
Screen Real Estate:
On the whole, team Onyx did a good job in term of screen real estate. The space allocated for the main body text is wide enough. However, when the window is reduced to less than 1/2 of the monitor space, the side bars cannot be show and some of the main body text is also not visible. I have to scroll across the page in order to see all the text.
Quite often, I have to scroll down the page to see some content and I think that can be improved by several ways:
1. Make the title smaller. It is catchy now but it takes up quite a lot of space.
2. Put the Logout Sign on the same level as the title or on the main navigation bar. Right now, it is occupying a separate row at the top.
3. They are too much white space inside the sidebars. Minimizing those white space might be a good idea.
Overall:
I tried their webpage on both Internet Explorer and Firefox and both worked well. Therefore, their website is user friendly regardless of the browser the user is using. Overall, this is a very well designed webpage and as a user, I had an easy time navigating it.
Friday, November 16, 2007
29.MyIsern-1.3-review
Author reviewed: Team Brown
1. Installation Review:
It was very easy to find and download their project as Randy has posted an entry for myIsern-1.3 and the link for download is easily accessible there.
Installation was easy. They have an installation guide and the link to it is also incldued in the blog. By following the instructions there, installation went well. All QA tests passed except for junit.
2. Code format and conventions review:
They did a great job with code format. I couldn't spot any violation only a few minor ones find in the MyIsernActionBean.java which has uncommented prviate members and an irrevalant comment on line 51 from conflicted file during update. There are also some prviate members that are not commented in other files but other than these, everything is good.
3. Test Case Review:
Black box perspective:
Most of the Action Beans are tested throughly, but the model still needs more testings. In particalar, DataIntegrityReportBean is not being tested at all.
White box perspective:
Emma Coverage summary
class: 88% (15/17)
method: 83% (133/161)
block: 70% (1728/2473)
line: 72% (373.8/522)
The coverage is pretty good as almost all classes are tested.
Break da buggah:
The program crush when pressing the save button for the collabotation item. An error always occur there even if no data is changed for that item. Also, by declaring a new organization entry, the xml file would be corrupted. Only the first 4 entries remain while all the rest, including with the new entry created would be erased from the file.
4. User Interface Review:
The user interface was simple and concise. In turns of real estate, they have done a great job! It functions well even when the screen is reduced to a small size. However, it was purely text-based and using more graphic should make it more attractive and professional-looking.
5. Summary and Lessons Learned:
They have achieved a lot with only a week! After reviewing their code, I learned how to implement the login page using Security filter which our group was previously stuck in. Also, the installation guide was a great help to users and our group should include one too.
Wednesday, October 17, 2007
22.MyISERN-1.1.Review
Author reviewed: Ben Karsin of team Ivory
This is the first time we can choose the author to review and so I chose a graduate student’s team to learn from them.
1. Installation Review:
I went to the project page listed on his blog and downloaded the package successfully. Installation was quick and easy. Verify passed indicating JUnit, Checkstyle, PMD, and FindBugs tasks all passed too. The program also enable the creation of a jar file upon typing “ant jar”.
It was very easy to figure out how to use their program as they provided a helpful list of queries upon typing in “java –jar myisern-ivory.jar”. It also listed the link to the user guide in case the user needs more instruction on using the program. I found this queries list very helpful and I always return to it when I was testing the program as it was hard to remember all the different combination of queries. However, there was a typo in one of the queries: “-listCollaboraitons -researcher
2. Code format and conventions review:
They modularized their codes and so there are a number of files in this project. However, after going through all of them, only TestQueryParser.java contains some minor violations. All the codes are also commented well. They have done a good job on code format and conventions.
| File | Lines | Violation | Comments |
| TestQueryParser.java | 59,60,71,* | EJS-7 | Should include white spaces |
3. Test Case Review:
Black box perspective:
The purpose of this program is to print out tables based on the input queries and they have done a lot of testing on the input. However, they have missed out testing the boundary cases for “-listOrganization”. Other than that, they have done a pretty good job on black box testing.
White box perspective:
Emma Coverage summary
class: 100% (12/12)
method: 89% (65/73)
block: 87% (2684/3091)
line: 85% (452.5/532)
One reason why some coverages are not 100% is they forgot to check “-listOrganiztion” as indicated above. The TestXmlQuery.java was supposed to test all the methods in XmlQuery.java but they have left out this one.
Break da buggah:
They have done throughout testing and managed to catch most of my incorrect/invalid queries. However, upon typing “java -jar myisern-ivory.jar -describe -researcher PhilipJohnson” it returns the NullPointerException.
Similar errors also with “-describe –organization < uniqueID>” and “-describe –collaboration < uniqueID>” occur upon typing in anything other than a valid < uniqueID> that exists in the database.
4. Summary and Lessons Learned:
Overall, this program is pretty well-written. One thing I learned from Ben’s team is how to modularize a program. When I first imported the project into Eclipse, I was overwhelmed by the long list of java file there are and didn’t know where to start. It was a big difference from our team’s project which only comprises of two file and all the methods are put within MyIsernXmlLoader.xml. However, once I started going through their project, I was impressed with how organized the codes are. They broke down everything and put them into separate classes which makes it very easy to locate a particular method and to make changes. Given MyIsern is such a big project, I can see how modularization will be especially useful. This is something that my team is going to work on in the next milestone.
Tuesday, October 9, 2007
16. MyIsernReview
Author reviewed: Chiao-Fen Zielinski
1. Installation Review:
I downloaded the package from the link provided in her blog successfully. Installation went well. There was no instruction on how to execute the program but I was able to invoke "ant jar" from the command prompt to produce a jar file. After that, it wasn't difficult to figure out how to run the program as I could execute the program by "java -jar MyISERN-1-Orange.jar". The tables are produced as desired. Verify passed indicating JUnit, Checkstyle, PMD, and FindBugs tasks all passed too. Her team also achieved 100% Emma coverage. Good job!
2. Code format and conventions review:
There are a few violations in the code:
| File | Lines | Violation | Comments |
| MyIsernXmlLoader.java | 48,49,50 | EJS-38 | Document all private memberss |
| MyIsernXmlLoader.java | 206, 207, 208 | EJS-36 | Use one-line comments to explain implementation details |
| TestMyIsernXmlLoader.java | 51 | EJS-7 | Extra blank line between methods |
3. Test Case Review:
Black box perspective:
Since the purpose of this assignment is to print out the tables based on data given in the 3 xml files, they have fulfilled this requirement. However, they seem to have misinterpreted the data in the colaborations.example.xml as there is only one collaboration item but they split up some data such as "collaborating-organizations" in two separate rows. This would cause confusion as users might mistake the second row as a separate item with missing fields. Since this is a mistake based on misinterpretation of data, even writing test cases could not have spotted that. Other than this, as printing out the data correctly is the only objective, I can't think of other test cases.
White box perspective:
They achieve 100% Emma coverage in all four areas, indicating that all codes has been covered at least once. They also have tests to make sure that the lists are returning the correct value which is nice.
Break da buggah:
The methods to print out the tables are entirely tailored to the data given in this set. If the xml files are revised, such as by adding more items to the collaboration file, the table would fail to print out the new data.
4. Summary and Lessons Learned:
Writing test cases are useful for catching a lot of errors but they still have their limits. In this case, the problem is caused by misinterpretation of data which cannot by spotted by tests. This is when reviewing by another person is especially useful. Through this exercise, I learn about how useful it is to have someone review your code as sometimes you are limited by your perspective and cannot spot some trival minor errors. Also, by comparing their codes with my group's, I learn more about ways to test the main methods as I am quite clueless whenever it comes to writing tests for void methods. On the whole, her team did a good job on this assignment.
Monday, October 1, 2007
12. WebSpider review
Reviewed Package Author: JianFei Liao
1. Installation Review:
I downloaded JianFei's code and extracted it with ease. The installation also went well.
Test results:
JUnit: the test runs with 2 failures. The failures are found in testFindTotalLinks and testFIndMostPopular.
Checkstyle: the test runs successfully with no errors.
PMD: the test runs successfully with no errors.
FindBugs: the test runs successfully with no errors.
Jar File Construction and execution: a jar file was built successfully. However, it was named named webspider.jar instead of webspider-jianfei.jar.
I was able to invoke "java -jar webspider.jar -totallinks http://www.hackystat.org 100" and it returns 3257.
2. Code format and conventions review
| File | Lines | Violation | Comments |
| WebSpiderExample.java | 117,118, 254, * | EJS-7 | Unnessary blank lines within a method |
| TestWebSpiderExample.java | 16,49,100 | EJS-7 | Unnessary blank lines within a method |
| TestWebSpiderExample.java | 19,20,21,* | EFS-39 | Document all private members |
3. Test case review
Black box perspective:
All methods and classes in WebSpiderExample were tested in
TestWebSpiderExample.java. However, two of the tests failed as indicated by JUnit.
For the class WebItem, only increaseCounter() is tested while getCounter() and getLink() are only indirectly tested by calling the main method.
No tests for boundary cases are included. It should consider checking the following cases:
- crawling through 0 page
- crawling through -1 page
- crawling through 9999999999999 pages
- entering an invalid URL such as http://hackystat
- entering no URL
- crawling through a page with no links
The best test to do with this program is to create a test site so that we can compare the results.
White box perspective:
Running Emma produces the following coverage summary:
class: 100% (2/2)
method: 100% (9/9)
block: 79% (476/602)
line: 85% (99/116)
This indicates that some blocks and lines are not covered. This can be improved by adding the following test which crawl through more pages:
String[] testArgs7 = { totallinks, testStartUrl, "10", logging };
WebSpiderExample.main(testArgs7);
By employing this, block and line coverages are improved to 81% and 90% respectively.
Break da buggah:
I tried to invoke "java -jar webspider.jar -totallinks http://httpunit.org 9999999999999" but it returns the warning: "Argument for number of pages to crawl is not an integer." It also returns the invalid result "The total number of links discovered while crawling the first 0 pages accessible from http://httpunit.org is: 0".
4. Summary and lessons learned
Overall, this is a well-written program with throughout exception handling. Even though it is hard to determine if the program is returning the correct number of links, I tested it against Randy Cox's test site and it passed the test by returning the correct results. So it does a good job on that.
Exceptions were handled well. I have tried to break the code with a lot of test cases like entering negative or decimal argument for pages to crawl, invalid URL, employ URL, URL with no links etc. The program manages to catch all the exception. However, no appropriate error messages are displayed. This is something that can be improved. I also tried to run the program with my friends' myspace and xanga account, google.com and a few other websites. I was actually surprised that it doesn't crash at all which indicate that he did a good job with exception handling. On the whole, this is a well-done program.
From this exercise, I have learned that testing is important. When I did my assignment, I had no intention of implementing black box and white box perspective in my tests. After implementing them on reviewing other's code, I realize that this would make sure I was not testing randomly and to ensure maximum coverage of my tests.
I also gained more experience with working with JUnit, PMD, Emma and Checkstyle. Even though I am still puzzled at times by how to ensure maximum coverage of Emma, I am at least more familiar with the other QA tools now.
Further more, by comparing JianFei's code with mine. I realize that my code could be improved by removing some redundant codes. He employed a similar algorithm as mine but his was more organized and concise. This is what I have to work on with my code.
Friday, August 31, 2007
CodeRulerReview
I was assigned to review Sonwright Gomex’s code. The code is well-organized and do not contain a lot of violations against the Java standard. It was commented well and was easy for me to follow along. I liked his strategy and he did a good job with the knights. All in all, this is a nicely written code and I had a good time reviewing it.
| File | Lines | Violation | Comments |
| MyRuler.java | 10,11,185,* | EJS-6 | Comment line has more than 100 characters |
| MyRuler.java | 12,17,25,* | ICS-SE-Java-6 | Comments hsould always end with a period |
| MyRuler.java | 16,17,18,* | ICS-SE-Eclipse-2 | Code should be indented 2 spaces instead of 4 |
| MyRuler.java | 16,172 | EJS-5 | The "else-if" should start on a new line |
| MyRuler.java | 193,201,210,* | EJS-76 | The "if" or "else" should always has curly braces |
Subscribe to:
Posts (Atom)