21
This part of the review is focused on the Javascript aspect of the answer
I will be really light on this review.
I will try to make it short and quick.
You have some mixed quotes.
Consider the following piece of code:
$("#dialog-confirm").dialog({
autoOpen: false,
resizable: false,
height: 'auto',
width: '400px',
modal: true,
[......
15
Use UriBuilder and set Port to -1
Uri oldUri = new Uri("http://myhost:443/Home/Index");
UriBuilder builder = new UriBuilder(oldUri);
builder.Port = -1;
Uri newUri = builder.Uri;
In case you want to remove the port part only when it is default, e.g. 80 for http and 443 for https, use snippet below (credit goes to Chris for the idea)
static Uri ...
15
As you have already mentioned, DbContext is a UoW, and DbSet is a repository -- there is no need to reimplement those patterns, unless you're into ridiculously useless complexity.
Entity Framework wraps all pending changes in a transaction for you already, so each DbContext in an application contains DbSets that are somehow related. Blogs and Posts are ...
13
Here are some observations:
you first iterate all rows in Credentials and then select the Password yet again - why not just select the pasword?
seems like your password is plain-text - BAD idea - hash your password together with a salt please
don't concat strings to get SELECT queries - use parameters instead
you are using C#/ASP.NET so why don't you use ...
13
I suppose the simplest thing is to go through the principles:
SRP - Cannot see any 'too big' things in your example
OCP - The example is small enough and you are not using class
inheritance that I cannot think of how this would apply
LSP - You only have one implementation of each interface so hard to
say
ISP - Your interfaces seem to serve a single purpose ...
12
/// <summary>
/// Insert value into the cache using
/// appropriate name/value pairs
/// </summary>
/// <typeparam name="T">Type of cached item</typeparam>
/// <param name="o">Item to be cached</param>
/// <param name="key">Name of item</param>
public static void Add<T>(T o, string key)
I think this ...
11
This sounds exactly like a desire for Dependency Injection (using an IoC container). ASP.NET MVC is perfectly setup for this already with plenty of containers out there for you to use!
You can still have a base constructor take arguments if you wish but I would try and limit how many.
So your controllers would look like:
public StudentController(...
11
I don't do much asp.net, but if this StackOverflow answer applies then you could just do this:
using Settings = MyProject.Namespace.Properties.Settings;
var loadedCode = Settings.Default.LoadedCode;
Like this other StackOverflow answer is saying:
We prefer to use Properties.Settings (aka. settings.settings) because it's strongly typed.
Now I know for ...
10
Don't use underscores in identifiers. If that's how the database has them, use a ColumnAttribute to specify the column name scratch that, you're going code-first. This:
public int Tournament_Id { get; set; }
public int Match_Id { get; set; }
public int Team_Id { get; set; }
public int PlayerAccount_Id { get; set; }
Is much more seesharpesque like this:
...
10
The recommended way to avoid SQL injection attacks is to use parameters.
Also I can recommend that you create a stored procedure instead of using dynamic SQL.
You can pass your dropdown indexes as parameters to your SP and use the old trick of using it as conditional on the where clause.
Your select can end up like this:
select CT.columnA, CT.columnB
from ...
10
private readonly string _ReCaptchaSecret;
private readonly string _ReCaptchaSiteKey;
private fields are either lowerCamelCase or _lowerCamelCase, both conventions are common.
public ReCaptchaValidator(string reCaptchaSecret)
{
_ReCaptchaSecret = reCaptchaSecret;
this.ErrorCodes = new List<string>();
}
public ReCaptchaValidator(string ...
9
I made a small example to show you that all your worries are in the past. Before you implement this yourself though, keep Mat's remarks in mind: I made this as a quick sketch and separating the mappings from the context, naming conventions, etc are important for your code's clarity.
That being said, this small setup shows you how you can change it:
public ...
9
try
{
if (!Exists(key))
{
value = default(T);
return false;
}
value = (T) HttpContext.Current.Cache[key];
} catch {
value = default(T);
return false;
}
return true;
This contains some duplicate logic. We can rewrite it like this:
if (Exists(key))
{
value = (T) HttpContext.Current.Cache[key];
return true;
}
...
9
if (dr.GetValue(0).ToString() == _str)
{
dr.Close();
cmdOne = new SqlCommand("select customername from customer where userid='" + objLogOnUserInformation.UserId + "'", con);
dr = cmdOne.ExecuteReader();
dr.Read();
objLogOnUserInformation.SessionUserName = dr.GetValue(0).ToString();
dr.Close();
//if usertype is ...
8
I have witnessed the UriBuilder .port = -1 fix elsewhere. Frankly guys, it is a hack that really isn't a good production ready solution.
The right solution here is to use UriBuilder as follows.
var uri = new UriBuilder(baseUri);
string newUrl = uri.Uri.ToString();
8
As @dreza mentioned, you might want to move the code around your LINQ-Statements to a method.
public void changeCategoryTo(String newCategory){
var record = (from n in db.Navigation where n.Category == newCategory
&& n.SectionId == SectionID select n).First();
record.Category = newCategory;
}
this greatly reduces ...
8
I'd suggest something like the following which is simple enough for even a novice like me to understand. Is there a real need to simulate button click? You're not testing buttons, are you?
// OnClick="ThisButton_OnClick" for ThisButton
protected void ThisButton_OnClick(object sender, EventArgs e)
{
// do stuff
if(condition)
...
8
Your questions imply that you might not be quite aware of how streams or references work in C#.
You pass in a reference to a Stream
You create a StreamWriter which writes to that Stream
This will automatically make changes visible to anyone holding a reference to the same Stream.
Therefor there is no need to try and return the Stream in any way from the ...
8
Accessors in C#
You should read on accessors in C# - your setter does not do what it should - it should set the value if Cname. If this has no meaning (you can't change the value of Cname - you should drop the set altogether.
Correct use of accessors:
public string Name
{
get
{
return name;
}
set
{
name = value;
}
}
For ...
8
I would suggest one more solution how to make usage a bit easier. You can add this method into CacheHelper
public static T GetOrAdd<T>(string key, Func<T> getter)
{
T value;
if (!TryGet<T>(key, out value))
{
value = getter();
Add(value, key);
}
return value;
}
In that case usage can be a bit simplier, ...
8
First off your methods are not extension methods as the name RequestsExtensions would indicate. Not sure if this intentional or. If you want to make them true extension methods then I'd consider passing the Url parameter as Uri rather than as string.
Other things I noticed:
The use of a magic number here: req.Timeout = 100000; - This should be a parameter (...
8
There are five lines that are repeated three times:
bll.LocationID = ddlLocations.SelectedValue;
bll.IsApproved = "y";
bll.ServiceProviderLocationAdd();
gvServiceProviders.DataSource = bll.GetServiceProviderLocations();
gvServiceProviders.DataBind();
Those should be moved to a separate method to start with. I also don't like that it does two things: update ...
7
This switch should be an if statement instead.
if (input == null) // we didn't get an input from GetCustomEditorInput
{
switch (property.PropertyType.Name)
{
case "Boolean":
input = new ExpressFormsCheckBox()
{
FormName = FormName,
...
7
I would write it like this
Dim m1 As TolerenceHelper
Dim flag As Boolean = True
m1 = New TolerenceHelper
flag = m1.IsInTolerence(Integer.Parse(txtT1.Text), Integer.Parse(txtA1.Text), Integer.Parse(txtTolerance.Text))
If flag = False Then
lblTest.Text = "Out of tolerance"
Else
lblTest.Text = "In tolerance"
End If
that function returns a boolean ...
7
I was going to switch it to a case statement but wanted to check if there were even better ways to reduce the clutter.
You could load the mappings from job titles to emails into a (static) dictionary, and then do a dictionary look-up (using the default value if it's not in the dictionary).
The Dictionary<string,string> can be initialized with a ...
7
Per the comment, what you probably want is an abstract class MyAppPage:
abstract class MyAppPage : Page {
protected Dictionary<NpInfoHelper, object> Informer;
protected override void OnInit(EventArgs e)
{
if (Session.Keys.Count == 1)
{
Session.Abandon();
Response.Redirect("~/Pages/Login?e=true", true)...
7
I would basically write a method wherever you've written a comment on what a block of code does. This allows you to hide the specifics of a particular operation and makes it clearer which code paths are taken and what they do at a high level:
private Service1 GetService()
{
return new TestProjectService.Service1();
}
private CommandMessages GetCommand(...
7
The major bottleneck is most likely in the ReadPdfFile method as we are dealing with a PDF file.
In your ReadPdfFilemethod, a PdfReader is created to read through every page of the document to find the searchText and the page numbers on which the searchText is found is stored inside a List<int> named pages.
Once the reader ran through every page, the ...
7
This can be shortened up quite a bit. First, you have presentation logic intertwined in your ASP code, which is the main cause for the code repetition. Shortening this code and DRY-ing it up involves two main pieces:
create a user control to encompass this menu, and only this menu
Move all presentation logic into the C# code-behind file
Creating the User ...
Only top voted, non community-wiki answers of a minimum length are eligible
