Senior Project Release: 02/10/2024

[ArabellaJi]

This PR is to code review and release the senior project branch.

[ArabellaJi]

Here is the API code for the Housing Lottery Senior Project. In this project, we created five tables and referenced them mainly in the HousingController and HousingService files.
I may remove the Housing.Year table and make some changes to the Housing.Applicant table in the near future, but we'd love to start our code review process on other parts of the code so that we have a better chance to launch our project on time. Thank you very much!!

[EjPlatzer]

Hi Arabella,

The code looks correct from a behavioral standpoint, although I don't know your requirements. Great work!

I have left some initial comments about changes to improve the expressiveness, performance, and readability of your code. Let me know if you have questions on any of those comments.

I know you said you're planning to re-design the database layout somewhat, which I think is good. I didn't leave any comments about that for now, although I do have some thoughts about how you could improve it.

I do have some concerns about design, especially in terms of database design and the Type of data the API is working with. I think it would be worthwhile to meet together and discuss those design aspects if you would like to.

[EjPlatzer]

Instead of BadRequest, you should return Unauthorized here. Also, you can reduce the nesting here and make the successful path more prominent by inverting the condition. In my opinion, this makes the logic easier to understand and follow:

Suggested change

	if (viewerGroups.Contains(AuthGroup.HousingAdmin))
	{
	await _housingService.UpdateDueDateAsync(dueDate);
	return Ok();
	}
	return BadRequest();
	if (!viewerGroups.Contains(AuthGroup.HousingAdmin))
	{
	return Unauthorized();
	}
	await _housingService.UpdateDueDateAsync(dueDate);
	return Ok();

[bennettforkner]

IMO this should be done in StateYourBusiness.cs instead of in the controller. Is that possible here?

[EjPlatzer]

We can simplify the logic for getting the max class year of all applicants, and also make it much more performant, by getting all rows from the Student table in one query:

Suggested change

	List<string> yearList = new List<string>();
	foreach (string e in emailList)
	{
	if (e != "")
	{
	var newApplicant = new Applicant
	{
	ApplicationID = applicantion_id,
	Applicant1 = e
	}; ;
	await _context.Applicant.AddAsync(newApplicant);
	var student = _context.Student.FirstOrDefault(x => x.Email == e);
	if (student != null)
	{
	yearList.Add(student.Class);
	}
	}
	}
	var newMaxYear = new Year
	{
	ApplicationID = applicantion_id,
	Year1 = yearList.Max()
	}; ;
	await _context.Year.AddAsync(newMaxYear);
	var maxYear = _context.Student.Where(s => emailList.Contains(s.Email)).Select(s => s.Class).Max();
	await _context.Year.AddAsync(new Year
	{
	ApplicationID = applicantion_id,
	Year1 = maxYear
	});

[EjPlatzer]

Similarly, we can use a bit of LINQ to simplify the Applicant code:

Suggested change

	List<string> yearList = new List<string>();
	foreach (string e in emailList)
	{
	if (e != "")
	{
	var newApplicant = new Applicant
	{
	ApplicationID = applicantion_id,
	Applicant1 = e
	}; ;
	await _context.Applicant.AddAsync(newApplicant);
	var student = _context.Student.FirstOrDefault(x => x.Email == e);
	if (student != null)
	{
	yearList.Add(student.Class);
	}
	}
	}
	var newMaxYear = new Year
	{
	ApplicationID = applicantion_id,
	Year1 = yearList.Max()
	}; ;
	await _context.Year.AddAsync(newMaxYear);
	var newApplicants = emailList
	.Where(e => String.IsNullOrWhiteSpace(e))
	.Select(email => new Applicant
	{
	ApplicationID = applicantion_id,
	Applicant1 = email
	});
	_context.Applicant.AddRange(newApplicants);

[bennettforkner]

IMO this should be done in StateYourBusiness.cs instead of in the controller. Is that possible here?

[bennettforkner]

Please use update logic instead of delete and readd.

[bennettforkner]

Aren't these Class Years? School years are a different thing.