Using viewmodel create comper and edit

Question

I have this really long httppost action results that I add and update 3 db tables I think it's too long and can get shorter and smarter.

public   class AllVm
{
    public AllVm()
    {
        this.wri = new Writer();
        this.sub = new Subject();
        this.rev = new Review();

    }
    public Writer wri { get; set; }
    public Subject sub { get; set; }
    public Review rev { get; set; }

    }
}

    public ActionResult Review()
    {
        return View();
    }
    [HttpPost]
    public async Task<ActionResult> Review(AllVm model)
    {
        var logedInUser = User.Identity.GetUserId();
        var memberDetails = (from m in db.member.Where(a => a.ApplicationUserId == logedInUser) select m).FirstOrDefault();
        //check if subject name allready exist 
        var reviewSubject = from c in db.Subject select c.SubjectName.ToString().ToLower();
        var match = await reviewSubject.FirstOrDefaultAsync(stringToCheck => stringToCheck.Equals(model.sub.SubjectName.ToLower()));

        model.rev.Created = DateTime.Now;
        if (ModelState.IsValid)
        {
            if ((model.sub.SubjectName.ToLower()).Equals(match))

            {
                var ttt = (from c in db.Subject.Where(aa => aa.SubjectName.ToString().ToLower().Equals(match)) select c);
                var GetSubjectId = await (from a in ttt select a.SubjectId).SingleAsync();

                if (model.rev.GBU == "Good")
                {
                    var Good = await (from a in ttt select a.Good).SingleAsync();
                    var bad = await (from a in ttt select a.bad).SingleAsync();
                    var Ugly = await (from a in ttt select a.Ugly).SingleAsync();
                    model.sub.bad = bad;
                    model.sub.Ugly = Ugly;
                    int iGoodRating = Convert.ToInt32(Good);
                    iGoodRating++;
                    model.sub.Good = iGoodRating;
                    model.sub.SubjectId = GetSubjectId;
                    db.Entry(model.sub).State = EntityState.Modified;
                }

                else if (model.rev.GBU == "Bad")
                {
                    var Bad = await (from a in ttt select a.bad).SingleAsync();
                    var Ugly = await (from a in ttt select a.Ugly).SingleAsync();
                    var Good = await (from a in ttt select a.Good).SingleAsync();
                    model.sub.Ugly = Ugly;
                    model.sub.Good = Good;

                    int iBadRating = Convert.ToInt32(Bad);
                    iBadRating++;
                    model.sub.bad = iBadRating;
                    model.sub.SubjectId = GetSubjectId;
                    db.Entry(model.sub).State = EntityState.Modified;
                }

                else if (model.rev.GBU == "Ugly")
                {
                    var Ugly = await (from a in ttt select a.Ugly).SingleAsync();
                    var Bad = await (from a in ttt select a.bad).SingleAsync();
                    var Good = await (from a in ttt select a.Good).SingleAsync();
                    model.sub.Good = Good;
                    model.sub.bad = Bad;
                    int iUglyRating = Convert.ToInt32(Ugly);
                    iUglyRating++;
                    model.sub.Ugly = iUglyRating;
                    model.sub.SubjectId = GetSubjectId;
                    db.Entry(model.sub).State = EntityState.Modified;
                }

                model.rev.SubjectId = GetSubjectId;

            }
            else
            {
                model.sub.Good = 0;
                model.sub.bad = 0;
                model.sub.Ugly = 0;

                if (model.rev.GBU == "Good")
                    model.sub.Good = 1;
                else if (model.rev.GBU == "Bad")
                    model.sub.bad = 1;
                else if (model.rev.GBU == "Ugly")
                    model.sub.Ugly = 1;

                db.Subject.Add(model.sub);
            }
            //writer
            model.wri.WriterName = memberDetails.Name;
            //check if writerId allready in the Db

            var WriterMatch = db.Writer.Where(n => n.WriterName.ToLower().Equals(model.wri.WriterName));
            if (WriterMatch.Count() > 0)
            {
                var getWriterId = await (from a in WriterMatch select a.WriterId).SingleAsync();
                model.rev.WriterId = getWriterId;
            }

            else
            {
                model.wri.WriterName = memberDetails.Name;
                db.Writer.Add(model.wri);
            }
            model.rev.offensive = false;
            db.Review.Add(model.rev);
            await db.SaveChangesAsync();
            return View("index");
        }

        return View(model);
    }
}

Answer 1

• Nested If-Statements: Nesting If-statements tends to lead to code that's harder to follow at times. So when you're able to reduce the amount of nesting that you do by returning early (once again this isn't always possible but it can be helpful to reduce nesting and therefore improve readability).

Since you do nothing after the ModelState.IsValid if-statement you could return early instead by flipping your condition like so:

if (!ModelState.IsValid) 
{ 
    return View(model);
}
// Now everything that was in your if-statement can go here just as it was but indented slightly less.

• Naming & Consistency: Let's start with ttt it took me a minute (two) to figure out where this was being declared. Why did I need to know where it was being declared? Because I didn't know what it was. Name your variables something that describes them. Generally the more descriptive the better. Secondly, be consistent. You have a.Ugly, a.bad, and a.Good. Fix this by making it a.Bad a simple Right-Click -> Refactor -> Rename should make this a breeze.

• Modularity: Make use of functions to make code more reusable and to improve readability. The contents of your if (model.rev.GBU == "Good") if-statement chain is all repeated code. Break that down into a function and just call that function 1-3 times (depending how you break it down). This will not only reduce the amount of code you have but it should make it a bit clearer on what you're doing (to others, you, or you 2 weeks from now).

• ViewModel: Your view model has a few things wrong with it. The Name doesn't describe at all what it is for, I've suggested a probably inaccurate name to try and get my point across. Public class members should be PascalCased, and you should avoid truncating a variable name just to make it shorter. Now this being said, I have been told before that making a variable name the same as the class it represents is bad, but I have yet to have a problem doing so, you're welcome to add more to the member names but I wouldn't recommend truncating Writer to wri, Subject to sub, etc.

Remember: Right-Click -> Refactor -> Rename

    public class WrittenPieceReviewVM
    {
        public Writer Writer { get; set; }
        public Subject Subject { get; set; }
        public Review Review { get; set; }

        public WrittenPieceReviewVM()
        {
            this.Writer = new Writer();
            this.Subject = new Subject();
            this.Review = new Review();
        }
    }

I hope these suggestions have helped you in some way.

Answer 2

You need to work on your naming. You have lots of incorrect casing e.g. local variables in PascalCase but the content of the names isn't as good as it could be either:

var logedInUser = User.Identity.GetUserId();

loged is a typo; it should be logged. It's also not a user - it's an id! The name would be better as loggedInUserId. You should read the official naming guidelines and capitalisation conventions see here.

---

You are mixing query and fluent syntax for your LINQ queries which is resulting a in a lot of code bloat. Let's refactor it one step at a time.

 var memberDetails = (from m in db.member.Where(a => a.ApplicationUserId == logedInUser) select m).FirstOrDefault();

Step 1: remove the pointless query:

var memberDetails = db.member.Where(a => a.ApplicationUserId == loggedInUserId).FirstOrDefault();

Step 2: remove Where and use FirstOrDefault with the predicate.

var memberDetails = db.member.FirstOrDefault(a => a.ApplicationUserId == loggedInUserId);

Step 3: an Id is unique so semantically, you should use SingleOrDefault here:

var memberDetails = db.member.SingleOrDefault(a => a.ApplicationUserId == loggedInUserId);

Step 4: fix the names:

var member = db.Members.SingleOrDefault(member => member.ApplicationUserId == loggedInUserId); 

Step 5: use async like you do elsewhere:

var member = await db.Members.SingleOrDefaultAsync(member => member.ApplicationUserId == loggedInUserId);

---

You have another 2 spelling mistakes/typos here:

//check if subject name allready exist 

The words are "already" and "exists"

// check if the subject name already exists.

---

You should try to be as clear as possible:

var reviewSubject = from c in db.Subject select c.SubjectName.ToString().ToLower();
var match = await reviewSubject.FirstOrDefaultAsync(stringToCheck => stringToCheck.Equals(model.sub.SubjectName.ToLower()));

That should definitely be 1 query not 2.

var match = await db.Subject.Select(s => s.SubjectName).FirstOrDefaultAsync(subjectName => subjectName.Equals(model.sub.SubjectName));

If you have a case sensitive collation on your SQL column you'll need to use a case insensitive comparison but I would guess you don't.

Similarly, you can simplify this:

(model.sub.SubjectName.ToLower()).Equals(match)

to

string.Equals(model.sub.SubjectName, match, StringComparison.InvariantCultureIgnoreCase)

By "simplify" I mean make your intent obvious and avoid creating extra lower case versions of all your strings.

---

This is a bit odd:

(from a in ttt select a.Good).SingleAsync();

Why don't you materialise a from the ttt collection (whatever that is) and then access the properties instead?

var a = await ttt.SingleAsync();
a.Good

---

You need to start splitting out some functionality into well named methods.