The Stack Overflow podcast is back! Listen to an interview with our new CEO.
10

I can't make a thorough review right now, but here's a quick fix that should help you structure things up. What you have is probably working, but you're mixing-up presentation with data concerns. I don't think these methods should be returning void. Picture this: protected IEnumerable<SalesInfo> LoadSales() { using (var db = new ...


7

First of all, your code doesn't compile, you have a poor lonely extra closing bracket in your Update method. Next, your Update method shouldn't Insert if Update doesn't work. If the entity doesn't exist, you should return false. Otherwise rename your method UpdateOrInsert so that it is clear what you are doing! Also, you shouldn't blindly catch every ...


6

Another way to do this is to remove NULL values from the comparison to begin with, like so: return Context.TblNumbers .Where(m => m.Number != null) .Where(m => convertedNumbers.Contains(m.Number.Value)) .Select(m => m.Number.Value) .ToList(); I think this also reads more naturally since you ...


5

Besides the two answers already given, I'd also suggest the following. Rename projLead to projectLeader. Same for projSet. But most importantly, I'd rethink your entire if. I suspect there are several errors -- department.Equals("Status")? department.Equals("Project Leader")? -- but more importantly I cannot figure out the logic and I suspect you're ...


4

You have two different Bracing styles in this code, which can be very confusing, else if (projLead.Equals("Project Leader")){ if (status.Equals("Status")) { I would not do that and many would agree that you should at the least pick one bracing style and stick with it. The most popular bracing style is to have braces on their own line ...


4

What you call business logic methods is actually your data access logic. And you've put that in a bunch of static methods on what I'm assuming is a static class, which means the GUI code / "populate fields/accounts" is tightly coupled with that specific data access logic implementation (AccountLogic); I'd make them instance methods and inject an instance of ...


4

I've also spotted a few things in your code... This if (id != null && id.HasValue) is exactly the same as if (id.HasValue) A nullable value has the HasValue so that you don't have to check against null, it cannot be null, it either has a value or not Here using (DataClassesDataContext db = new DataClassesDataContext()) { // ... } you ...


4

In addition to Heslacher's points: Careful with keys like "SuccessMessage" and "FailureMessage": a typo easily happens, so instead make these const string in a dedicated class. I find K&R style braces an odd sight in C# code; but you do seem to be consistent, so that's good. Properties should be PascalCase: fileUploadNews, message, title I wouldn't name ...


4

Just some quick shoots. if (id != null && id.HasValue) this two checks are doing the same where the first is just shortcut of the second. Classes should be named using PascalCase casing. See the NET naming guidelines. the comment here // If we're here - either a fake id was given or none - return an empty form ViewBag.ButtonText = ...


4

First, the fact that your methods are static, and accessing what appears to be a static, disposable field, scares me: your application quite possibly has structural problems much more important than a slow query - but you haven't included nearly enough code for us to review that aspect. All I can say, is that db instances should be as short-lived as ...


4

You have: if (db.BadgeAssignments.Any(bba => bba.UserIdReceiver == newbadgeassignment.UserIdReceiver && bba.BadgeAssociated.CourseId == courseId)) And later: .Where(bba => bba.UserIdReceiver == newbadgeassignment.UserIdReceiver && bba.BadgeAssociated.CourseId == courseId) The two lambdas: bba => bba.UserIdReceiver == ...


4

//countQuery mirrors query through the filter assignments I don't think that's necessary. You could have only one query for the first part of the method, then do recordCount = query.Count(); and then keep using query as you do now. This way, you have less code and less chance of the count getting out of sync. query.Where("IsActive = true") I realize ...


3

You have three calls to repository.GetAllEntities(). If those execute a db call each time, that's a bad thing. But it actually points to a deeper issue. May I introduce you to Enumerable.GroupBy: foreach(var regionStringGroup in repository.GetAllEntities().GroupBy(x => x.RegionString)) { var regionObject = new NameChildObject() { name = region }; ...


3

The code looks OK. Linq can be somewhat tricky to debug, as there is some magic happening behind the scenes. In my world that still doesn't warrant to rebuild it into somehting else just for debugging for a random case which you don't experience just now. Don't worry too much for problems which hasn't occured yet. Some say that worrying beforehand is double ...


3

Not so a direct answer to your question but this may help you. Most of methods you are using are really extension methods, If your only worry is debugging or logging, you can get code for these methods and create your own extension methods with same names but Namespace must be different. As these are extension methods, you will not be able to override them....


3

Your use of linq is fine. You may consider using a more descriptive variable than a, such as patient You should pass any query parameters likely to change as variables. Consider: var patientID = 1198; var detail = from patient in da.tblPatients where patient.ID == patientID select patient; I find the form population logic and ...


3

I would not write this extension method for int at all since it assumes everything of type int in your project is referring to the ID of a table. Also, as a rule of thumb, if you're using strings that contain structural metadata about your objects, it's generally a sign you might want to rethink your approach. Instead, I would find a way to apply an ...


3

Naming Based on the naming guidlines variables/fields should be named "using a noun, noun phrase, or adjective". So getByQuerable should be renamed to something following the guidlines and also meaningful. Optimization you should extract the searching for Source and Target to separate methods. This helps for readability and makes it easier for Mr....


3

So, let us see what you are doing. You query your database for IEnumerable<RezeptBilder> and iterate over the enumeration where you are converting the items to Image and add it to a List<Image>. Then you are converting the Image back to a Byte[] to write it into the response stream. Byte[] -> Image -> Byte[] isn't really necessary. ...


3

Flow You should for sure change the flow of this method. The first query is called each time, wether it is needed or not. You should as the simpliest change add a pure else to your big if statement where you execute the query. For each query you are calling .OrderBy(x => x.ProjectID).Skip(projSet * 75).Take(75).ToList(). which you can do at ...


3

With ToArray() you create an in-memory copy of all queried data. If you have 10m records this is incredibly expensive (both for CPU and memory). Avoid that and fetch data from DB each time you use it without needless copies. See also this question post for further details. First change it to: var query = from p in behzad....


3

It seems like you could do this: var results = (from d in ctx.Dramas where !d.Resolved group d by new { d.Student, DateHappened = d.DateHappened.ToString("ddMMyy") } into g select new { Student = g.Key.Student, DateHappened = g.Key.DateHappened, EquipmentDrama = g.Any(d => d.DramaType == DramaType....


3

So it looks like what you want is some sort of outer join. I think this should work for you: var categoryFilter = "Category1"; var query = from addon in dc.Addons select new { Addon = addon.Name, Amounts = from product in dc.Products where product.Category.Name == categoryFilter join ...


3

The principal performance issues are on the database side (or maybe you could do some kind of caching or pre-loading), but your code could be made much clearer and shorter (there should be also some minor performance improvements): string[] columnNames = new[] { "sub_id", "sibheadername" }; var categories = db.allcategories().ToList(); if (categories....


3

Look at the PredicateBuilder on LINQKit. These code snippets are from the linked page: public interface IValidFromTo { DateTime? ValidFrom { get; } DateTime? ValidTo { get; } } Now we can define a single generic IsCurrent method using that interface as a >constraint: public static Expression<Func<TEntity, bool>> IsCurrent<TEntity&...


3

String requires string.Compare Not true. There's a more generic option: if (typeof(IComparable).IsAssignableFrom(newSelector.Type)) { switch (condition.Item2) { case CompareTypes.Equals: expression = (ex, value) => value.CompareTo(cond.Item3) == 0; break; case CompareTypes....


3

Is this what you are looking for? skew.CalibrationDate.Date == db.Skew .Where(s => s.CalibrationDate.Date <= date.Date) .Max(s => s.CalibrationDate.Date) Max will return the maximum value itself, not the object with the maximum date.


3

You are rather removing those parts instead of replacing them, so more appropriate name would be remove all. You can make your own extension method like this : public static class Extensions { public static string RemoveAll(this string source, string[] charsToRemove) { return charsToRemove.Aggregate(source, (current, t) => current....


3

lstdepartment We don't use Hungarian notation with prefixes like lst in C#. It's just departments This is notepad coding but you can use a join to get the matching deparment var results = from e in _context.Employee join d in _context.Department on e.DepartmentId equals d.DepartmentId where e.EmployeeId = EmployeeId select new ...


3

Some quick remarks: pid, dateofvisit, stamp, type are all properties and thus should follow Microsoft's naming guidelines. Moreover "pid" is fairly meaningless, why not call this "PetId" as well? testTypes does not convey its actual contents. Using an int to indicate a type is meaningless. I don't know what a test of type "1" is, or a "2". You should ...


Only top voted, non community-wiki answers of a minimum length are eligible