Hey guys. This is my second code review posting here. This time it’s for my Weather Alert Database.
The applications Graph View section still needs to be completed and I’m looking for advice on how to proceed.
I want an excuse to practice javascript/jquery (or similar) and insert that in the Graph View section. Basically I’m assuming the Graph View tab content will have to be handled by another project (Like ASP.net maybe?) and then displayed to the user. I guess think of it like a “front end” section if you will. Do you guys have any advice? Thanks.
It looks like you’re using WPF, so why not create the graph using whatever tools you have available, or draw them? If it’s going to be javascript driven, then under the rules of this forum we can’t help you in that aspect, just note that you’ll somehow have to host an ASP.NET site somewhere either as a separate application or hosted on your desktop application, just to serve up an HTML page to show you the graphs.
UpdateUIElements.PopulateAllEventViewControls is called every time a user interaction is performed. Even when typing an event ID.
That means you refresh all the UI elements. For user inputs, you don’t expect these to change constantly. Since the database is local, it isn’t that obvious, but I do still see slight delays when interacting with the controls.
Separate out populating the controls. A refresh button for a total refresh should be considered, when you perform filtering, you should just be updating your results. This goes with the Single Responsibility Principle (SRP) of SOLID.
UpdateUIEventType populates itself from the listview. I’m not sure what you want to do here, but this ties up with the previous suggestion. Populate your controls from the database, not the current listview. If you have hundreds of thousands of data points and you are forced to only show the first thousand for performance reasons, and somehow those thousand don’t have some event you want to filter on, then you will never get that item in your dropdown.
EV_EventID_TextBox_TextChanged again calls UpdateEventViewUI() unnecessarily, where you have placed your UI valdiation. This violates SRP. Separate validation from data retrieval.
Consider using some sort of way to accept only numeric characters, and ignore others, instead of showing a dialog box.
If you must perform a search without pressing enter, look for a way to delay the database request by, say 500 milliseconds up to a second so that typing a longer ID doesn’t trigger a request every keypress. This is often called “debounce”, in reference to how physical contact switches in digital situations actually “bounce” between on and off before making contact, and so require the software to wait for the keypress state to settle before processing the input.
You have a database, use it to it’s fullest. Don’t fetch all alerts and filter them in the application. Build a query from your inputs and return only the data you need.
InsertIn_DB does not need DoesObjectAlreadyExist. Since you are writing your SQL anyway (not using something like EntityFramework) you can do an INSERT WHERE NOT EXISTS to let the database handle the check, versus reading from the database then writing to the database (a round-trip operation). You can get the result to check how many records were affected, if you need to find if the insert was successful. And don’t use LIKE to check for exiting Ids. They should be unique and so comparions should be exact.
Create a class that holds all the values you want filter on, and populate that from the text boxes prior to each search. Then pass that to your data layer.
Consider moving all the static stuff out of Alert class and into another class AlertHelper, or separate classes depending on what data they handle.
Go for classes and interfaces over static methods.
Utilize encapsulation to define behavior. For example, you could abstract the database methods into an interface. Then have an abstract base class that has a connection string constructor, and some predefined virtual methods for pulling data. Since you have a “debug” database and a “live” database, that perform basically the same thing, you create two classes that inherit from the base data class, differing by the connection string you pass them upon instantiation. So the classes will be empty.
Why two classes, where one can suffice? I saw some comment about preventing invalid edits to the dummy DB, (which never acutally happens in a Select, but anyway). If you want to prevent updates to your dummy DB, just override the methods in DebugSqlLiteDataAccess that perform edits to do nothing, and your dummy db will be safe.
so why not create the graph using whatever tools you have available, or draw them?
I’m sure I could find libraries that would handle this stuff easily within the wpf environment. Specifically I’m just looking to quickly integrate some JS into what I have already developed. The reason is that employers typically require some JS knowledge and this would serve as an “all in one” project I could show off.
If it’s going to be javascript driven, then under the rules of this forum we can’t help you in that aspect,
I’m not looking for JS specific advice. Rather I’m looking at integrating another project into my solution which may or may not be JS driven.
just note that you’ll somehow have to host an ASP.NET site somewhere
Thanks. I’ll keep this in mind, I assume I could just generate local one as needed and display it, hopefully.
I seen that you updated your reply with a thorough review. Thanks for taking your time to review the application.
look for a way to delay the database request by, say 500ms
I really like this suggestion as I liked the fact the Listview get updated semi real time and this seems like a good middle ground. Possibly I could use a timer that resets the interval if more text is detected within a period.
you can do an INSERT WHERE NOT EXISTS
I had no idea about this, thanks for the idea. I’ll have to see about retrieving the number of alerts that didnt get inserted as that number is output to the log.
You have a database, use it to it’s fullest.
I’m 100% guilty of this. This was actually intentional, I wanted a reason to use LINQ so I decided early on that I would filter the objects using LINQ rather than requesting the DB dynamically (like I should).
Create a class that holds all the values you want filter
This is a perfect idea, thanks. I use static methods too much because I’m not exactly sure how conceptually to use constucted objects apart from storing basic information. This gives me a great idea to work with though.
preventing invalid edits to the dummy DB
This comment should be on the methods that alter the DB not the Select method. The Insert/Update method specifically is forced to use the MainDB connection string.
Again, thanks for the code review. You’ve given me quite a few things to refactor.
C# devs
null reference exceptions