Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm new in ASP.NET MVC (using MVC 3).
Could you inspect (refactoring) my code, to advise, or anything else? Maybe something can be rewritten in a more compact.
A few details about site: on main page is tabs (today, tomorrow...) when user click on tab then need load films for tab. It's done via JavaScript (jQuery).

This is my views:

<%@ Page Language="C#" MasterPageFile="~/Views/Shared/Site.Master" Inherits="System.Web.Mvc.ViewPage" %>
<asp:Content ID="Content2" ContentPlaceHolderID="MainContent" runat="server">
<script src="../../assets/js/RatingAndGoing.js" type="text/javascript"></script>

<script type="text/javascript">
    $(document).ready(function () {
        loadFilmList(); // by default load today's film

        $(".title").click(function (e) {
            e.preventDefault();
            loadFilmList($(this));
        });

        // loading film depending on selected tab
        function loadFilmList(id, value) {
            var param;
            if (value !== undefined)
                param = value;
            else
                param = $(id).attr("id");

            $('#listInfoBlock').load('/cinema/filmlist?id=' + param, function () {                
                $('.infoBlock').switcher();
                $('div.widgets').switcher(
                    {
                        btn: '.expand',
                        block: '.voice_block',
                        hideifout: true
                        // classActive:'expand'
                    });

                setUpUpdateGoingCount();
                setRating();
            });
            if (id !== undefined) {
                $(".tabs").find("a").removeClass("active");
                $(id).parent().addClass("active");
            }
        };

        $("#film_picker.datepicker").datepicker('option', 'onSelect', function (dataText) {
            loadFilmList($(this), dataText);
        });
    });
</script>

    <div id="content">
        <div class="section">
            <div class="title title_section">Movies</div>
            <div class="wrap_tabs">
                <ul class="tabs">              
                    <li><a class="active"><span id="today" class="title">Today</span></a></li>
                    <li><a><span id="tomorrow" class="title">Tomorrow</span></a></li>
                    <li><a><span id="week" class="title">Current Week</span></a></li>
                    <li><a><span id="month" class="title">Current Month</span></a></li>                    
                    <li class="select_date">                      
                        <form  class="fancy_form">
                          <div class="wrap_input">
                                <i>&nbsp;</i>
                                <input id="film_picker" type="text" class="datepicker"/>
                            </div>
                        </form>
                    </li>
                </ul>
            </div><!-- .wrap_tabs-->

            <div class="filter_block">
                <span>Shows</span>
                <a id="showFullFilmList"><span class="title">full schedule</span></a>
                <a id="showOnlyNameFilmList" class="active"><span class="title">Film's name</span></a>                
            </div><!-- .filter_block-->

            <div id="listInfoBlock" class="list_infoBlock">                       
            </div><!-- .list_infoBlock-->
        </div><!-- .section-->
    </div><!-- #content-->
</asp:Content>

FilmList view:

<%@ Control Language="C#" Inherits="System.Web.Mvc.ViewUserControl<DT.KazBilet.Web.Portal.Models.CinemaViewData>" %>
<% foreach (var film in Model.FilmList)
   { %>
<div class="infoBlock">

    <div class="head_infoBlock">

        <div class="info_btn open"></div>
        <h2><a href="#"><%=film.Name%></a></h2>
        <span><%=film.Description%></span>
        <%=Html.Partial("Controls/RatingForList",film) %>    //bellow      
    </div><!-- .head_infoBlock-->

    <div class="details_info">

        <div class="poster">
            <div class="b_img"><img src="/assets/images/i/i1.jpg"/></div>
            <div class="btn_buy_ticket">
                <div><i></i><span><a href="#">Buy</a></span><em></em></div> //not finished yet
            </div>
        </div><!-- .poster-->

        <div class="object_info">
            <p><%=film.Genre%></p>
            <ul>
                <li><strong class="title">Director: </strong> <span><%=film.FilmDirector%></span></li>
                <li><strong class="title">Actors: </strong> <span><%=film.FilmActorList%></span></li>
                <li><strong class="title"><%=film.Country%>, <%=film.Year%> year, <%=film.Duration%> min.</strong></li>
                </ul>

            <table class="time_table">

                <% foreach (var schedule in film.CinemaSchedules)
                { %>
                <tr>
                    <td><a href="#"><%=schedule.Cinema.BaseEstablishment.Name%></a></td>
                    <td class="time_list">
                        <ul>
                            <li><a href="#"><%=TimeSpan.FromSeconds(schedule.ShowTime.Value) %></a></li>                            
                        </ul>
                    </td>
                </tr>
                <% } %>

            </table><!-- .time_table-->
        </div><!-- .object_info-->
    </div><!-- .details_info-->
</div>  <!-- .infoBlock-->
 <% } %>

And RaitingForList:

<%@ Control Language="C#" Inherits="System.Web.Mvc.ViewUserControl<DT.KazBilet.Objects.IEvent>" %>
<div class="widgets">
    <div class="rating_block"> 
    <span class="iFoot fMin">
            <%= Model.GoingCount %></span>  
        <div class="stars_rating">
            <div class="stars_off">
            </div>
            <div class="stars_on" style="width: <%= Model.AverageRating * 20 %>%">
            </div>
        </div>
        <!-- .stars_rating-->
        <span class="num_rating">(<%= Model.AverageRating%>)</span>
        <div class="expand">                     
        </div>
    </div>
    <!-- .rating_block-->
    <div class="voice_block">
        <div class="going" eventid="<%=Model.OID %>" type="<%=Model.Type %>">
            <a href="">I'm Going!</a>
            <div id="going_too">
                Going too:
                <%= Model.GoingCount%></div>
        </div>
        <!-- .going-->
        <div class="data_for_rating">
            <div>Vote</div>
            <div class="star_for_rating">
                <ul>
                    <li><a href="#">1</a> </li>
                    <li><a href="#">2</a> </li>
                    <li><a href="#">3</a> </li>
                    <li><a href="#">4</a> </li>
                    <li><a href="#">5</a> </li>
                </ul>
            </div>
            <!-- .star_for_rating-->
            <div id="full_num_rating">
                Average:
                <%=Model.AverageRating%>
                (<%=Model.RatingVoteCount%>)</div>
        </div>
        <!-- .data_for_rating-->
    </div>
    <!-- .voice_block-->
</div>
<!-- .widgets-->
share|improve this question

1 Answer

up vote 1 down vote accepted

There's not much going on in there for you to worry about getting anything wrong. You're strongly typing your views to a Domain Model instead of a View Model. Some people think this is a no-no. I think it's fine until you start to notice you're either a) writing to much code in your view to manipulate model properties or b) you're passing in several Models and decide to use ViewData[]. I also would advise switching to razor view engine as the syntax is cleaner and clearer. Is there a reason your js isn't in a js file?

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.