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

Since I don't have the SharePoint libraries I wasn't able to test my solution, but it should work. Use proper indentation and formatting. == true is redundant and can be removed. Instead of the following: bool.Parse(properties.ListItem.Properties["Regular Customer"].ToString()) just convert to bool (casting won't work because of SharePoint): Convert....


8

I am making some assumptions about your code, specifically that listItem is of type SPListItem and that listItem.Properties is a HashTable. This is mainly to provide context for my explanation, but even if I am mistaken, most of my comments will still hold. For the sake of context, let's assume your actual method looks like: void ...


7

Personally, I think you would be better off moving this code into a separate class as it's probably logic you are going to want in multiple places in your application and is most likely violating SRP (Single Responsibility Principle) wherever it is at the moment. If we create a username class, we can encapsulate the logic in there and then re-use it from ...


6

Naming I won't list every bad name but you need to think more carefully about the names you choose. ID should be Id Microsoft have very specific guidelines for this. The two abbreviations that can be used in identifiers are ID and OK. In Pascal-cased identifiers they should appear as Id, and Ok. If used as the first word in a camel-cased identifier, ...


5

No, you can't use a factory to dispose your objects, your code is creating and immediately disposing the SPSite as soon as it steps out of your method. public override SPSite GetSite(string url = null) { return new SPSite(string.IsNullOrEmpty(url) ? Url : url)) } Have the disposing be handled by the calling code. You can't handle the disposing from ...


5

As well as some standard indenting, a quick thought could be reversing the item being compared and using some temporary variables for readability. EDIT: Ok, so svick pointed out that the original code was fundamentally flawed. So how about creating an extension method on string to do the Contains for us. Something like: // Method would do well with ...


4

"Fast" is not too relevant here, since it's such a simple method. And the canned response also applies: if you want to improve the speed, first do some benchmarking to know where the bottlenecks really are. But here my proposal of a better way: private const string DefaultEmployeeName = "JDoe"; /// <param name="userName">Format expected: "Domain\Name"...


4

The .Parse methods will throw exceptions if the value is null, in a bad format, etc. I suggest always using the .TryParse methods instead: bool val; if(!bool.TryParse(listItem.Properties["Boolean Property"].ToString(), out foo)) { val = false; } Or perhaps one of the Convert.ToBoolean() methods.


4

Give your code some room to breathe. Empty space costs nothing in C#, so add some gaps to group together similar concepts. public void createTask1_MethodInvoking(object sender, EventArgs e) { createTask1_TaskId1 = Guid.NewGuid(); createTask1_TaskProperties1.Title = "Test Title"; createTask1_TaskProperties1.AssignedTo = "i:0#.w|...


4

Eliminate potential logic error in return value I'm assuming the purpose of this code is to let you provide the name you want for your subsite, and the code will give you an available name based on whether the provided name already exists. It does this by appending/incrementing a suffix to the provided name whenever there is a naming collision. If this is ...


4

Naming conventions: As per the Microsoft guidelines, use PascalCase for method names. Read more here: Capitalization Conventions. Your method names will become GetValueFromList and CleanValue. Object Initialization and Format String: Use the String.Format method to build your string, don't use concatenation. The building of your CAML query will be: var ...


3

Have you tested this? You've used the words 'quick' and 'SharePoint' in the same question which, in my experience, is almost always a lie :) You have a problem here: private void RefreshOrders(string loadStatus) { Orders.LoadStatus = loadStatus; var collListItem = Orders.LatestOrders; if (!collListItem.Any()) { // omitted. } ...


3

Method names in .net should be PascalCase, so that would be GetValueFromList (and CleanValue), consistent with the rest of the framework and with what .net devs expect to see; Hungarian-style prefixes like the o of oWeb and oList are annoying too - nobody cares that an object is an Object. Name things after what they're used for, not for their type. It's ...


3

You asked about the loop. while (SubwebExists(temp)) { i++; temp = mainSite + subsiteName + i; } Is this loop guaranteed to execute at least once? I suspect so, because if it doesn't, you return subsiteName + 0. I don't think that's what you want. So, the loop should be a do {} while loop, like this: temp = mainSite + subsiteName; do { i++; } ...


3

public enum ExceptionLevel { Debug, Info, Error } And private enum LogType { Debug, Info, Error }; Both are enums, and both have the same values. This tells me you should probably merge these into one enum. while (true) { switch (type) { case LogType.Debug: Console.WriteLine("[{0}] {1}", DateTime.Now....


3

I think it's okay, but we can do better. Let's see what Get-Date returns. Run this: > get-date | get-member It shows that Get-Date returns a DateTime object, and it also lists all the properties that has. I notice that DateTime has a Date property. Looking on the Web, it says that this is the date part of the DateTime but with a zero time component. So ...


2

From a once over: Read this about the arrow pattern code, more specifically, your check on input should read like this: if( !input ) { return; //Or, you could even throw an error / exception ? } This can be shortened with the OR operator: var splitby = ';' if (typeof splitter != 'undefined') splitby = splitter to var splitby = splitter || ';' ...


2

It's quite late here, so just a quick answer but I can come back if this is still relevant to you. public static string FormatMyHomieURL(string subsiteName, string mainSite) { if (subsiteName == null) throw new ArgumentNullException(nameof(subsiteName)); if (mainSite == null) throw new ArgumentNullException(nameof(mainSite)); var url = ...


2

Good job on the formatting and using using. There's a few things I can see to clean this up. Is there a reason you are not using var to declare obvious variables? I would suggest to use it whenever you can. I would move the Console.WriteLine logging into a separate class, probably that had an interface, that is injected or set in your class. This will ...


2

As written, your code will process each item in mySourceItemColl for every user in the site. I would prefer to only go through the collection one time. As such, I would write it more like this: using (SPSite site = new SPSite("www.local.com")) using (SPWeb web = site.OpenWeb()) { SPList list = web.Lists["ListName"]; SPQuery query = ...


2

The my prefixes are unnecessary, I'd call the variables sourceList, sourceListQuery, sourceItems etc. I'd create a method for this: name = name.Substring(name.IndexOf("#") + 1); I guess removeId could be a good name. The method name would help readers to understand the intent of the writer.


2

You can use linq GroupBy and Select to reduce your list to distinct RoleTitle's in your foreach loop: foreach(Role role in emp.Roles.GroupBy(r=> r.RoleTitle).Select(g => g.First())) { //... }


2

Well, I cannot tell why it's so slow, but this should solve your problem: $sites = Get-SPOSite -Limit all -Detailed -Detailed switch will cause Get-SPOSite to load all properties. It makes iterating all sites to get title unnecessary.


2

orderedList: you should not have public fields in your class. Is it OK if someone else assigns an entirely new value to that field? If not then you should make a property with private setter. Also name isn't so helpful: orderer list of what? Even better: remove it. Static fields, properties and methods are terrible to test and in this case you don't even ...


2

Considering there are calls to MessageBox I suspect you're not using the MVVM pattern. I would really advise you to do this. That way you can also avoid things like dataGridOutstandingOrders.ItemsSource and instead bind via XAML. collListItem is an incomprehensible variable name, especially considering that it isn't a single item but apparently a list of ...


2

Overall, your code's fine. The only thing that really jumps out to me is that it clutters the global namespace with some variables; I'm not sure if that was intentional (such as if other code is then referencing those variables), but I'd try to avoid that if you can. I understand your unease with the way the code is written, so I'm going to use the rest of ...


2

There are numerous ways in which this code could be improved: You have a class that stores a bunch of information (id, dates etc). You have a class that performs actions upon that data. At the moment they are one class, when they should be two (Single responsibility principle). The naming of just about every variable and parameter is incomprehensible. What ...


2

Elvis is in the building! public string SiteUrl { get { return _spSiteUrl; } set { _spSiteUrl = value; _spSiteUrl = _spSiteUrl?.Replace(@"\", @"/"); } } I like that you're using the null conditional operator (Elvis), but I'm wondering if it's really necessary here. Neither your property or your backing field are nullable ...


2

Regarding performance, here's a major design flaw: string query = "<Query></Query>"; DataTable spDataTable = ExecuteCamlQuery("Ideas", query); string whereClause = "ID = '" + ideaID + "'"; DataRow[] results = spDataTable.Select(whereClause); This code is telling SharePoint to retrieve every item in the list, forcing all of the returned items ...


2

First of all "F:\test\Test.csv" should be escaped or a verbatim string: new StreamWriter("F:\\test\\Test.csv") Or: new StreamWriter(@"F:\test\Test.csv") Also you should not hardcode paths in your code but I suppose this is only because of posting here (but you'd better show your actual code cleaned from sensitive information). StreamWriter defaults to ...


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