1
\$\begingroup\$

I have the following code which uses EntityFramework to reach out to a DB and retrieve data to populate into a Winform application gridview.

I am still quite new to C# and especially abstracting code out of code behind/writing loosely coupled code.

Could the following code be reviewed and feedback given on the methods I am using to construct the application in a methodical way?

MainScreenForm

public partial class MainScreenForm : MainScreenController
{
    private static readonly ILog log = LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType);

    public MainScreenForm()
    {
        InitializeComponent();
    }

    private void MainScreenForm_Load(object sender, EventArgs e)
    {
        log.Debug("Main Screen Launched");
        PopulateGrid();
        AdjustGridviewVisuals();
    }

    private void AdjustGridviewVisuals()
    {
        metroGrid1.AutoSizeColumnsMode = DataGridViewAutoSizeColumnsMode.Fill;
        metroGrid1.AllowUserToAddRows = false;
        metroGrid1.RowHeadersVisible = false;

        this.metroGrid1.Columns["Id"].Visible = false;
        this.metroGrid1.Columns["ExamId"].Visible = false;
        this.metroGrid1.Columns["Comments"].Visible = false;
        this.metroGrid1.Columns["CreationDate"].Visible = false;
        this.metroGrid1.Columns["LastModified"].Visible = false;
        this.metroGrid1.Columns["PatientState"].Visible = false;
    }

    private void PopulateGrid()
    {
        metroGrid1.DataSource = GetListOfPatients();
    }
}

MainScreen Controller

public class MainScreenController : MetroForm
{
    private static readonly ILog log = LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType);

    private TrollyPatientDbContext db = new TrollyPatientDbContext();

    public List<Patient> GetListOfPatients()
    {
        log.Debug("Attempting to retreve a list of patients from SQL");
        var patients = db.Patients;
        return patients.ToList();
    }
}

DbContext

public class TrollyPatientDbContext : DbContext
{
    public TrollyPatientDbContext() : base("PrimaryConnection")
    {
    }
    public DbSet<Exam> Exams { get; set; }
    public DbSet<Patient> Patients { get; set; }
}

The models are just standard "POCO" (I think that's what it is called).

Is the way I have extended the controller to allow access to methods within it a good way to do this work or should I being using static methods instead?

Is this considered a way of constructing to an MVP practice?

\$\endgroup\$
  • \$\begingroup\$ Any reason you're using WinForms? WPF is much better. \$\endgroup\$ – BCdotWEB Apr 30 '15 at 13:11
  • \$\begingroup\$ Is there a reason you are using System.Reflection.MethodBase.GetCurrentMethod().DeclaringType instead of typeof(MainScreenController)? \$\endgroup\$ – Dan Lyons Apr 30 '15 at 17:57
2
\$\begingroup\$
    this.metroGrid1.Columns["Id"].Visible = false;
    this.metroGrid1.Columns["ExamId"].Visible = false;
    this.metroGrid1.Columns["Comments"].Visible = false;
    this.metroGrid1.Columns["CreationDate"].Visible = false;
    this.metroGrid1.Columns["LastModified"].Visible = false;
    this.metroGrid1.Columns["PatientState"].Visible = false;

These lines have a lot of repetition. It would be better to create an array of the column names, and then loop over the array and set each one.

    var patients = db.Patients;

What's the purpose of having a temporary variable here? You're not even declaring its type.

\$\endgroup\$
  • \$\begingroup\$ I will add the array loop now and change the return code to call "return db.Patients.toList(); Any feedback on how i am structuring my Controller/Views? \$\endgroup\$ – tornup Apr 21 '15 at 12:36
1
\$\begingroup\$

You have done a good job for being relatively new. However, I have a few remarks/questions:

  • Is there a special reason for inheriting? If not, try to keep the amount of levels low. This will make your code less complex and more maintanable.
  • If it isn't required, don't use static properties/methods
  • If you are binding to a grid, try to do this declaratively in the UI - not in the codebehind
  • Create an extra layer of separation by introducing a repository pattern to interact with entity framework and the database. There are many articles which describe this pattern and its implementation.
  • Controllers should not contain business logic, they are meant to navigate the user interface to the service layer of the application. Data retrieval actions belong at this level. In .NET, you would want to create a new class library for that.
  • Disposing DbContext: an instance of this class consumes a lot of memory so dispose it when you no longer need it.
\$\endgroup\$
  • \$\begingroup\$ WRT the repository pattern: isn't that overkill considering EF already implements such a pattern? \$\endgroup\$ – BCdotWEB Apr 30 '15 at 13:14
  • \$\begingroup\$ the answer you reference suggests adding a 'service layer' which personally i would call a respository. The case for, is you may well find you need to call a sproc or custom sql for performance on one or more of your queries rather than using the EF generated sql \$\endgroup\$ – Ewan Apr 30 '15 at 14:47
0
\$\begingroup\$

I think Dependency Injection deserves a mention here.

I suggest you inject your repository or db context into your controller as well at the logging component

This will require you create interfaces for them.

Also, is there a reason to have that .ToList() rather than just passing back the IEnumerable or IQueryable?

I second the comment about removing the inheritance

\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview, Ewan. Would you elaborate more on why you suggest your method? \$\endgroup\$ – Legato Apr 30 '15 at 15:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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