#145 ✓resolved
Kieran P

Routes.rb is getting harder to maintain / understand

Reported by Kieran P | December 30th, 2008 @ 09:59 AM | in 1.2

Routes.rb should make use of the map.with_options Rails feature, which groups similar pages (rss, searchs etc) into blocks, and removes duplication.

Comments and changes to this ticket

  • Kieran P

    Kieran P December 30th, 2008 @ 12:44 PM

    • State changed from “new” to “to-review”

    I've commit this work to the refinement_lh145_cleaned_up_routes branch.

    Changeset: http://github.com/kete/kete/comm...

    I've manually tests the key points of Kete (items, search, homepages, baskets etc), and it's all working. I've run through all unit, functional and integration tests. They're all passing as well.

  • Walter McGinnis

    Walter McGinnis December 30th, 2008 @ 01:14 PM

    • Milestone set to 1.2

    By and large the commit looks ok to me.

    One thing I would double check is that the old named routes aren't called directly in our codebase. I would grep for each one in app and lib.

    I also see some ugly redundancy in naming of the routes. For example:

    rss.basket_list_rss

    or

    search_all.basket_all

    Is the "rss" or the "all" necessary to be in the second part of the name? What are the resulting automatically generated method names for these routes?

  • Kieran P

    Kieran P December 30th, 2008 @ 02:51 PM

    One thing I would double check is that the old named routes aren't called directly in our codebase. What are the resulting automatically generated method names for these routes?

    Even if they were, the generated routes are the same. The way the routes are made has just been wrapped in with_options blocks to remove duplication.

    Is the "rss" or the "all" necessary to be in the second part of the name?

    Changing that bit would break existing route calls..... (removed original message).

    Edit: I was happy to make those types of changes, but after thought, it probably would be counter productive (see next post for reason).

  • Kieran P

    Kieran P December 30th, 2008 @ 04:10 PM

    On second thought, it might not be a good idea to change the named routes to remove the all/for in the names. The routes file might have search_for, but when in app/lib it won't, so it would be hard to tell a contributed_by_path for searching, from a contributed_by_path for rss.

    What do you think?

  • Walter McGinnis

    Walter McGinnis January 1st, 2009 @ 06:20 PM

    • Tag set to routes
    • State changed from “to-review” to “open”

    Just leave it as is and merge it to master please.

    If you want to get extra spiffy you could add some route testing for all the named routes.

    That said...

    I'm not a huge fan of the variable names you have chosen in your blocks (rss, search_all, search_for, etc). Those are where I would get rid of the duplication (usually it's also a duplicate of the :action or :controller value and also outer block variable you are calling).

    That being said, I don't have better variable names that pop to mind.

  • Kieran P

    Kieran P January 5th, 2009 @ 01:17 PM

    • State changed from “open” to “resolved”

    Work has been merged into master branch. Resolving ticket.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

Kete was developed by Horowhenua Library Trust and Katipo Communications Ltd. to build a digital library of Horowhenua material.

People watching this ticket

Tags

Pages