Skip to main content
deleted 29 characters in body
Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

I was told that using a Model View Presenter pattern would help me out with that, but I don't see how I can decouple the control from the Module object that it parses and uses to jump to a specific line of code on double click.

So we are told always :-)

The view is already decoupled: it's in the todoItemsGridView and refreshButton fields that are just magically there in your class, ready to stick event listeners on.

To decouple the model, I think you'd have to extract RefreshTaskList (and consequently TryGetMarker) to a different class dedicated to manage the task list, including the logic of parsing lines for TODO markers.

I'm also not very happy with the private TryGetMarker method. I just can't shake the feeling that there's a better way to return a marker.

It seems quite fine to me, though maybe partly because I've seen you do this in other answers.

In this particular example, it might be better to change the method to GetMarker, make it return ToDoMarker or null if not found. Then the caller would change to a null check instead of a boolean check. The implementation will be shorter and just as good.


This is not great:

var priority = (TaskPriority)marker.priority;

Since the available markers don't change during runtime, it feels odd to cast again and again. More importantly, if the config file contains an invalid value, you will get an error when a marker is used, which is a bit late. I would prefer to get errors as soon as possible: if not at compile time, then during startup at runtime.

A solution to correct both of these is to wrap the parsing of the configuration in a cleaned object that performs type checking. This way if something's wrong you'll get an error during startup, and you can eliminate the ugly cast in the middle of your model's logic.


upCasedLine.Contains(marker.text)

This assumes that marker.text is always uppercase. But there's nothing in the code to guarantee that, and such, it will have to depend on good documentation that tells users to use uppercase names. It might be a good idea to uppercase marker.text. Again, this could be done in the config wrapper I mentioned above, to perform only once during startup.

I was told that using a Model View Presenter pattern would help me out with that, but I don't see how I can decouple the control from the Module object that it parses and uses to jump to a specific line of code on double click.

So we are told always :-)

The view is already decoupled: it's in the todoItemsGridView and refreshButton fields that are just magically there in your class, ready to stick event listeners on.

To decouple the model, I think you'd have to extract RefreshTaskList (and consequently TryGetMarker) to a different class dedicated to manage the task list, including the logic of parsing lines for TODO markers.

I'm also not very happy with the private TryGetMarker method. I just can't shake the feeling that there's a better way to return a marker.

It seems quite fine to me, though maybe partly because I've seen you do this in other answers.

In this particular example, it might be better to change the method to GetMarker, make it return ToDoMarker or null if not found. Then the caller would change to a null check instead of a boolean check. The implementation will be shorter and just as good.


This is not great:

var priority = (TaskPriority)marker.priority;

Since the available markers don't change during runtime, it feels odd to cast again and again. More importantly, if the config file contains an invalid value, you will get an error when a marker is used, which is a bit late. I would prefer to get errors as soon as possible: if not at compile time, then during startup at runtime.

A solution to correct both of these is to wrap the parsing of the configuration in a cleaned object that performs type checking. This way if something's wrong you'll get an error during startup, and you can eliminate the ugly cast in the middle of your model's logic.


upCasedLine.Contains(marker.text)

This assumes that marker.text is always uppercase. But there's nothing in the code to guarantee that, and such, it will have to depend on good documentation that tells users to use uppercase names. It might be a good idea to uppercase marker.text. Again, this could be done in the config wrapper I mentioned above, to perform only once during startup.

I was told that using a Model View Presenter pattern would help me out with that, but I don't see how I can decouple the control from the Module object that it parses and uses to jump to a specific line of code on double click.

The view is already decoupled: it's in the todoItemsGridView and refreshButton fields that are just magically there in your class, ready to stick event listeners on.

To decouple the model, I think you'd have to extract RefreshTaskList (and consequently TryGetMarker) to a different class dedicated to manage the task list, including the logic of parsing lines for TODO markers.

I'm also not very happy with the private TryGetMarker method. I just can't shake the feeling that there's a better way to return a marker.

It seems quite fine to me, though maybe partly because I've seen you do this in other answers.

In this particular example, it might be better to change the method to GetMarker, make it return ToDoMarker or null if not found. Then the caller would change to a null check instead of a boolean check. The implementation will be shorter and just as good.


This is not great:

var priority = (TaskPriority)marker.priority;

Since the available markers don't change during runtime, it feels odd to cast again and again. More importantly, if the config file contains an invalid value, you will get an error when a marker is used, which is a bit late. I would prefer to get errors as soon as possible: if not at compile time, then during startup at runtime.

A solution to correct both of these is to wrap the parsing of the configuration in a cleaned object that performs type checking. This way if something's wrong you'll get an error during startup, and you can eliminate the ugly cast in the middle of your model's logic.


upCasedLine.Contains(marker.text)

This assumes that marker.text is always uppercase. But there's nothing in the code to guarantee that, and such, it will have to depend on good documentation that tells users to use uppercase names. It might be a good idea to uppercase marker.text. Again, this could be done in the config wrapper I mentioned above, to perform only once during startup.

Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

I was told that using a Model View Presenter pattern would help me out with that, but I don't see how I can decouple the control from the Module object that it parses and uses to jump to a specific line of code on double click.

So we are told always :-)

The view is already decoupled: it's in the todoItemsGridView and refreshButton fields that are just magically there in your class, ready to stick event listeners on.

To decouple the model, I think you'd have to extract RefreshTaskList (and consequently TryGetMarker) to a different class dedicated to manage the task list, including the logic of parsing lines for TODO markers.

I'm also not very happy with the private TryGetMarker method. I just can't shake the feeling that there's a better way to return a marker.

It seems quite fine to me, though maybe partly because I've seen you do this in other answers.

In this particular example, it might be better to change the method to GetMarker, make it return ToDoMarker or null if not found. Then the caller would change to a null check instead of a boolean check. The implementation will be shorter and just as good.


This is not great:

var priority = (TaskPriority)marker.priority;

Since the available markers don't change during runtime, it feels odd to cast again and again. More importantly, if the config file contains an invalid value, you will get an error when a marker is used, which is a bit late. I would prefer to get errors as soon as possible: if not at compile time, then during startup at runtime.

A solution to correct both of these is to wrap the parsing of the configuration in a cleaned object that performs type checking. This way if something's wrong you'll get an error during startup, and you can eliminate the ugly cast in the middle of your model's logic.


upCasedLine.Contains(marker.text)

This assumes that marker.text is always uppercase. But there's nothing in the code to guarantee that, and such, it will have to depend on good documentation that tells users to use uppercase names. It might be a good idea to uppercase marker.text. Again, this could be done in the config wrapper I mentioned above, to perform only once during startup.