CSC/ECE 517 Fall 2014/oss E1459 jjr: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
 
(8 intermediate revisions by 2 users not shown)
Line 18: Line 18:


On the first round of refinement the code was simply cleaned up by untabifying the code and using the Rails and Expertiza coding standard of using two spaces for indentation.  Untabifying the code eliminates variations in tab stop configuration in programmers editors. This makes the code more readable to those who view the source code so that blocks don’t look like run together.
On the first round of refinement the code was simply cleaned up by untabifying the code and using the Rails and Expertiza coding standard of using two spaces for indentation.  Untabifying the code eliminates variations in tab stop configuration in programmers editors. This makes the code more readable to those who view the source code so that blocks don’t look like run together.
In the following examples, the original programmers did not employ the [http://en.wikipedia.org/wiki/Don%27t_Repeat_Yourself Don't Repeat Yourself] (DRY) principle to their code. We attempted to correct this by cleaning up duplicate code and removing function calls that provided no additional value or functionality from the call it makes within it.


== Rails Naming Conventions ==
== Rails Naming Conventions ==
Line 23: Line 26:
Some methods were renamed to ones that more closely align with Rails naming conventions.  In the app/controllers/sign_up_sheet_controller.rb the index and add_signup_topics actions were changed to index_signup and index actions respectively.  These two actions did similar functions and were made clearer by merging them.
Some methods were renamed to ones that more closely align with Rails naming conventions.  In the app/controllers/sign_up_sheet_controller.rb the index and add_signup_topics actions were changed to index_signup and index actions respectively.  These two actions did similar functions and were made clearer by merging them.


 
[[File:index_signup1.jpg | Renaming index method]]
{first image here}
 
 


The design of the SignUpSheetController class controls two different behaviors of the SignUpTopics model: creation of topics for an assignment by the instructor, and allowing students to sign up for a topic.  To clearly distinguish between these two different behaviors, all actions performed by the instructor to manage a topic (index/create/update/destroy) were renamed to follow the standard Rails naming convention.  All actions performed by the student (index/create/destroy) to sign up for a topic have the suffix _signup.
The design of the SignUpSheetController class controls two different behaviors of the SignUpTopics model: creation of topics for an assignment by the instructor, and allowing students to sign up for a topic.  To clearly distinguish between these two different behaviors, all actions performed by the instructor to manage a topic (index/create/update/destroy) were renamed to follow the standard Rails naming convention.  All actions performed by the student (index/create/destroy) to sign up for a topic have the suffix _signup.


The issue with these methods were that they didn’t follow Rails naming conventions for displaying a list of items and they were not representative of the the functions that they performed.  The add_signup_topics method displayed a list of topics for the teacher to modify the topics.  The index method although following Rails convention in naming the function name did describe its purpose with respect to the scope of the controller in which it lives.  The index method originally listed the topics for the student to sign up from.  This however is the sign up sheet controller and therefore the index method would have been expected to list topics and perform actions on them at a higher level.  The teacher is concerned with the assignment as a whole.  That is the teacher is concerned with listing and acting on a large number if not all of the topics on the assignment and better aligns with the name of the controller (signup_sheet_controller).  Therefore the add_signup_topics method was renamed to index and the original index method was renamed to index_signup.  The latter was renamed using the index convention but with the _signup suffix.  The original index method displayed the list of topics that the user could sign up for.  In this case the student is interested in acting only one one topic therefore the signup suffix reflects intent.
The issue with these methods were that they didn’t follow Rails naming conventions for displaying a list of items and they were not representative of the the functions that they performed.  The add_signup_topics method displayed a list of topics for the teacher to modify the topics.  The index method although following Rails convention in naming the function name did describe its purpose with respect to the scope of the controller in which it lives.  The index method originally listed the topics for the student to sign up from.  This however is the sign up sheet controller and therefore the index method would have been expected to list topics and perform actions on them at a higher level.  The teacher is concerned with the assignment as a whole.  That is the teacher is concerned with listing and acting on a large number if not all of the topics on the assignment and better aligns with the name of the controller (signup_sheet_controller).  Therefore the add_signup_topics method was renamed to index and the original index method was renamed to index_signup.  The latter was renamed using the index convention but with the _signup suffix.  The original index method displayed the list of topics that the user could sign up for.  In this case the student is interested in acting only one one topic therefore the signup suffix reflects intent.
In the following example from the sign_up_sheet_controller.rb file, the original programmers did not employ the DRY principle to their code. 


== Moving logic code to the Model ==
== Moving logic code to the Model ==
Line 41: Line 39:
There were also several “utility” methods that we believe were added to the controller at some earlier point and are now obfuscated due to the efforts of previous groups of students to improve the code. For instance, the otherConfirmedTopicforUser(assignment_id, creator_id) and slotAvailable?(topic_id) methods did nothing other than call a method in the model and return the result, this sort of complexity is unnecessary. We removed these methods from the controller as they achieved nothing and did not make the code any easier to read.
There were also several “utility” methods that we believe were added to the controller at some earlier point and are now obfuscated due to the efforts of previous groups of students to improve the code. For instance, the otherConfirmedTopicforUser(assignment_id, creator_id) and slotAvailable?(topic_id) methods did nothing other than call a method in the model and return the result, this sort of complexity is unnecessary. We removed these methods from the controller as they achieved nothing and did not make the code any easier to read.


{image 2 here}
[[File: OtherConfirmedTopicsForUser.jpg | OtherConfirmedTopicForUser Cleanup]]


== File change list ==
== File change list ==


The following is a complete list of files that were changed.
The following is a complete list of files that were changed.
<pre>
M      Gemfile.lock
M      app/controllers/sign_up_sheet_controller.rb
M      app/controllers/signup_controller.rb
M      app/models/sign_up_sheet.rb
M      app/models/sign_up_topic.rb
M      app/models/signed_up_user.rb
M      app/models/system_settings.rb
M      app/models/team.rb
M      app/views/assignments/_reserve_topic.html.erb
M      app/views/assignments/edit/_rubrics.html.erb
M      app/views/bookmarks/add_tag_bookmark.erb
M      app/views/bookmarks/view_topic_bookmarks.rhtml
D      app/views/sign_up_sheet/_add_signup_topcis.html.erb
D      app/views/sign_up_sheet/_add_signup_topics.html.erb
A      app/views/sign_up_sheet/_index.html.erb
A      app/views/sign_up_sheet/_index_staggered.html.erb
M      app/views/sign_up_sheet/_reserve_topic.html.erb
D      app/views/sign_up_sheet/add_signup_topics.html.erb
D      app/views/sign_up_sheet/add_signup_topics_staggered.html.erb
M      app/views/sign_up_sheet/edit.html.erb
A      app/views/sign_up_sheet/index.html.erb
A      app/views/sign_up_sheet/index_signup.html.erb
D      app/views/sign_up_sheet/list.html.erb
M      app/views/sign_up_sheet/new.html.erb
M      app/views/student_task/view.html.erb
M      app/views/tree_display/actions/_assignments_actions.html.erb
M      config/routes.rb
M      db/migrate/053_create_nodes.rb
M      db/schema.rb
M      test/fixtures/assignments.yml
M      test/fixtures/invitations.yml
M      test/fixtures/participants.yml
M      test/fixtures/question_types.yml
M      test/fixtures/questionnaires.yml
M      test/fixtures/questions.yml
M      test/fixtures/response_maps.yml
M      test/fixtures/responses.yml
M      test/fixtures/roles.yml
M      test/fixtures/scores.yml
M      test/fixtures/sign_up_topics.yml
M      test/fixtures/signed_up_users.yml
M      test/fixtures/teams.yml
M      test/fixtures/teams_users.yml
M      test/fixtures/topic_deadlines.yml
M      test/fixtures/users.yml
M      test/functional/sign_up_sheet_controller_test.rb
M      test/functional/signup_controller_test.rb
</pre>
== Testing ==
A [https://docs.google.com/document/d/1iU1FyPgfn5LmILBxnmcBW7kOrv3zC_FYmhRgVuj9ct0/edit?usp=sharing README] file has been written explaining how to access features updated in this project.  Much of the content of this file has been repeated here for convenience.
=== Accounts ===
The following accounts have been created on the test environment.
Administrator account:
<ul>
<li>admin - Password: admin</li>
</ul>
<br>
Student accounts:
<ul>
<li>student1</li>
<li>student2</li>
<li>student3</li>
<li>student4</li>
<li>student5</li>
<li>student6</li>
<li>student7</li>
<li>student8</li>
<li>student9</li>
</ul>
To log in as a student first log in as an admin and then use the Manage=>Impersonate User functionality to switch the student account of your choice.
There are two assignments pre-creted:
<ul>
<ul>
<li>M      Gemfile.lock</li>
<li>''due_date_assignment1'': Due date is same for all topics.</li>
<li>M      app/controllers/sign_up_sheet_controller.rb</li>
<li>''staggered_deadline_assignment1'': Due dates for topics are staggered based on a topic heirarchy.</li>
<li>M      app/controllers/signup_controller.rb</li>
<li>M      app/models/sign_up_sheet.rb</li>
<li>M      app/models/sign_up_topic.rb</li>
<li>M      app/models/signed_up_user.rb</li>
<li>M      app/models/system_settings.rb</li>
<li>M      app/models/team.rb</li>
<li>M      app/views/assignments/_reserve_topic.html.erb</li>
<li>M      app/views/assignments/edit/_rubrics.html.erb</li>
<li>M      app/views/bookmarks/add_tag_bookmark.erb</li>
<li>M      app/views/bookmarks/view_topic_bookmarks.rhtml</li>
<li>D      app/views/sign_up_sheet/_add_signup_topcis.html.erb</li>
<li>D      app/views/sign_up_sheet/_add_signup_topics.html.erb</li>
<li>A      app/views/sign_up_sheet/_index.html.erb</li>
<li>A      app/views/sign_up_sheet/_index_staggered.html.erb</li>
<li>M      app/views/sign_up_sheet/_reserve_topic.html.erb</li>
<li>D      app/views/sign_up_sheet/add_signup_topics.html.erb</li>
<li>D      app/views/sign_up_sheet/add_signup_topics_staggered.html.erb</li>
<li>M      app/views/sign_up_sheet/edit.html.erb</li>
<li>A      app/views/sign_up_sheet/index.html.erb</li>
<li>A      app/views/sign_up_sheet/index_signup.html.erb</li>
<li>D      app/views/sign_up_sheet/list.html.erb</li>
<li>M      app/views/sign_up_sheet/new.html.erb</li>
<li>M      app/views/student_task/view.html.erb</li>
<li>M      app/views/tree_display/actions/_assignments_actions.html.erb</li>
<li>M      config/routes.rb</li>
<li>M      db/migrate/053_create_nodes.rb</li>
<li>M      db/schema.rb</li>
<li>M      test/fixtures/assignments.yml</li>
<li>M      test/fixtures/invitations.yml</li>
<li>M      test/fixtures/participants.yml</li>
<li>M      test/fixtures/question_types.yml</li>
<li>M      test/fixtures/questionnaires.yml</li>
<li>M      test/fixtures/questions.yml</li>
<li>M      test/fixtures/response_maps.yml</li>
<li>M      test/fixtures/responses.yml</li>
<li>M      test/fixtures/roles.yml</li>
<li>M      test/fixtures/scores.yml</li>
<li>M      test/fixtures/sign_up_topics.yml</li>
<li>M      test/fixtures/signed_up_users.yml</li>
<li>M      test/fixtures/teams.yml</li>
<li>M      test/fixtures/teams_users.yml</li>
<li>M      test/fixtures/topic_deadlines.yml</li>
<li>M      test/fixtures/users.yml</li>
<li>M      test/functional/sign_up_sheet_controller_test.rb</li>
<li>M      test/functional/signup_controller_test.rb</li>
</ul>
</ul>


== References ==
These two assignments are provided for testing convenience - you are free to add topics and have students sign up/ drop topics.  However, '''PLEASE DO NOT DELETE THESE TWO ASSIGNMENTS'''.  The assignment controller/view has a bug which makes adding assignment deadlines impossible.  Deadlines for these two assignments were manually populated in the database.  You are free to create a new assignment for this purpose.


<references/>


== External Links ==
== Project Links ==
https://github.com/jmkubasc/expertiza<br>
[https://github.com/jmkubasc/expertiza Forked GitHub Repository]<br>
http://152.1.13.96:3000/
[http://152.1.13.96:3000/ VCL Instance] May be invalid after a few weeks<br>
[https://docs.google.com/document/d/1iU1FyPgfn5LmILBxnmcBW7kOrv3zC_FYmhRgVuj9ct0/edit?usp=sharing README]

Latest revision as of 03:51, 5 November 2014

E1459 - Refactoring the signup_controller

Introduction

Expertiza is a web based application used by students and professors to create, select, submit and review assignments. Selection of assignments may be individual assignments or group assignments. The assignment is configured by the professor. Peer reviews can be conducted by fellow students in the class. Additionally the professor can review the assignments submitted by the students. Assignments are what Expertiza calls learning objects. Learning objects consist of such things as articles, websites or web applications, or code.

The Expertiza project is open source with many programmers contributing to the code base. The source code is available at github. Due to the open source nature of this application the code is not always as clear, concise and as standardized as it could be. This project seeks to resolve some of these issues by applying sound object oriented design principles along with standard RESTful naming conventions where possible.


Scope

This project is focused on the signup_controller and its fringe classes. The role of the signup_controller is to process requests from the user to sign up for an assignment. The purpose of the controller is to process actions from the user; however, in many cases the controller is doing functions that should be provided by the model. Any calculations should appear in the model rather than the controller. The code should use RESTful names such as new, create, edit, and delete. In many cases it does not. Methods should be no more than 30-40 lines long. Any longer and the reader needs to rely on their working memory to remember what code that is outside of their view does which is an inefficient and error prone way to read code. Other classes that are involved in this process include the sign_up_sheet_controller a number of views that pertain to assignments, topics and fixtures and functional tests and model classes such as sign_up_sheet.rb and sign_up_topic.rb. The sign_up_sheet_controller handles actions pertaining to creating assignments.


Formatting Changes

On the first round of refinement the code was simply cleaned up by untabifying the code and using the Rails and Expertiza coding standard of using two spaces for indentation. Untabifying the code eliminates variations in tab stop configuration in programmers editors. This makes the code more readable to those who view the source code so that blocks don’t look like run together.

In the following examples, the original programmers did not employ the Don't Repeat Yourself (DRY) principle to their code. We attempted to correct this by cleaning up duplicate code and removing function calls that provided no additional value or functionality from the call it makes within it.


Rails Naming Conventions

Some methods were renamed to ones that more closely align with Rails naming conventions. In the app/controllers/sign_up_sheet_controller.rb the index and add_signup_topics actions were changed to index_signup and index actions respectively. These two actions did similar functions and were made clearer by merging them.

Renaming index method

The design of the SignUpSheetController class controls two different behaviors of the SignUpTopics model: creation of topics for an assignment by the instructor, and allowing students to sign up for a topic. To clearly distinguish between these two different behaviors, all actions performed by the instructor to manage a topic (index/create/update/destroy) were renamed to follow the standard Rails naming convention. All actions performed by the student (index/create/destroy) to sign up for a topic have the suffix _signup.

The issue with these methods were that they didn’t follow Rails naming conventions for displaying a list of items and they were not representative of the the functions that they performed. The add_signup_topics method displayed a list of topics for the teacher to modify the topics. The index method although following Rails convention in naming the function name did describe its purpose with respect to the scope of the controller in which it lives. The index method originally listed the topics for the student to sign up from. This however is the sign up sheet controller and therefore the index method would have been expected to list topics and perform actions on them at a higher level. The teacher is concerned with the assignment as a whole. That is the teacher is concerned with listing and acting on a large number if not all of the topics on the assignment and better aligns with the name of the controller (signup_sheet_controller). Therefore the add_signup_topics method was renamed to index and the original index method was renamed to index_signup. The latter was renamed using the index convention but with the _signup suffix. The original index method displayed the list of topics that the user could sign up for. In this case the student is interested in acting only one one topic therefore the signup suffix reflects intent.

Moving logic code to the Model

In the Model View Controller (MVC) paradigm code pertaining to the business logic of an application belongs in the model layer of the program. This paradigm was broken in several places in sign_up_sheet_controller.rb file. For instance, the confirm_topic method is used in the sign_up_sheet_controller.rb file to assign a topic to a user after performing several model layer checks to make sure that the user can, in fact, be assigned a topic. This code is better moved to the model layer as its performing business logic and not simple validation or control statements. The decision was made to move this method into the SignUpTopic model.

General Cleanup and Deletion

There were also several “utility” methods that we believe were added to the controller at some earlier point and are now obfuscated due to the efforts of previous groups of students to improve the code. For instance, the otherConfirmedTopicforUser(assignment_id, creator_id) and slotAvailable?(topic_id) methods did nothing other than call a method in the model and return the result, this sort of complexity is unnecessary. We removed these methods from the controller as they achieved nothing and did not make the code any easier to read.

OtherConfirmedTopicForUser Cleanup

File change list

The following is a complete list of files that were changed. 

M       Gemfile.lock
M       app/controllers/sign_up_sheet_controller.rb
M       app/controllers/signup_controller.rb
M       app/models/sign_up_sheet.rb
M       app/models/sign_up_topic.rb
M       app/models/signed_up_user.rb
M       app/models/system_settings.rb
M       app/models/team.rb
M       app/views/assignments/_reserve_topic.html.erb
M       app/views/assignments/edit/_rubrics.html.erb
M       app/views/bookmarks/add_tag_bookmark.erb
M       app/views/bookmarks/view_topic_bookmarks.rhtml
D       app/views/sign_up_sheet/_add_signup_topcis.html.erb
D       app/views/sign_up_sheet/_add_signup_topics.html.erb
A       app/views/sign_up_sheet/_index.html.erb
A       app/views/sign_up_sheet/_index_staggered.html.erb
M       app/views/sign_up_sheet/_reserve_topic.html.erb
D       app/views/sign_up_sheet/add_signup_topics.html.erb
D       app/views/sign_up_sheet/add_signup_topics_staggered.html.erb
M       app/views/sign_up_sheet/edit.html.erb
A       app/views/sign_up_sheet/index.html.erb
A       app/views/sign_up_sheet/index_signup.html.erb
D       app/views/sign_up_sheet/list.html.erb
M       app/views/sign_up_sheet/new.html.erb
M       app/views/student_task/view.html.erb
M       app/views/tree_display/actions/_assignments_actions.html.erb
M       config/routes.rb
M       db/migrate/053_create_nodes.rb
M       db/schema.rb
M       test/fixtures/assignments.yml
M       test/fixtures/invitations.yml
M       test/fixtures/participants.yml
M       test/fixtures/question_types.yml
M       test/fixtures/questionnaires.yml
M       test/fixtures/questions.yml
M       test/fixtures/response_maps.yml
M       test/fixtures/responses.yml
M       test/fixtures/roles.yml
M       test/fixtures/scores.yml
M       test/fixtures/sign_up_topics.yml
M       test/fixtures/signed_up_users.yml
M       test/fixtures/teams.yml
M       test/fixtures/teams_users.yml
M       test/fixtures/topic_deadlines.yml
M       test/fixtures/users.yml
M       test/functional/sign_up_sheet_controller_test.rb
M       test/functional/signup_controller_test.rb

Testing

A README file has been written explaining how to access features updated in this project. Much of the content of this file has been repeated here for convenience.

Accounts

The following accounts have been created on the test environment.

Administrator account:

  • admin - Password: admin


Student accounts:

  • student1
  • student2
  • student3
  • student4
  • student5
  • student6
  • student7
  • student8
  • student9

To log in as a student first log in as an admin and then use the Manage=>Impersonate User functionality to switch the student account of your choice.

There are two assignments pre-creted:

  • due_date_assignment1: Due date is same for all topics.
  • staggered_deadline_assignment1: Due dates for topics are staggered based on a topic heirarchy.

These two assignments are provided for testing convenience - you are free to add topics and have students sign up/ drop topics. However, PLEASE DO NOT DELETE THESE TWO ASSIGNMENTS. The assignment controller/view has a bug which makes adding assignment deadlines impossible. Deadlines for these two assignments were manually populated in the database. You are free to create a new assignment for this purpose.


Project Links

Forked GitHub Repository
VCL Instance May be invalid after a few weeks
README

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2014/oss_E1459_jjr&oldid=91420"