Outlook VSTO: Fastening email loop on a large pst file In a Outlook VSTO Addin I needed to loop through a large pst file (e.g 30GB containing 50k+ mails). Following code block loop the pst file folder by folder recursively. It takes huge time (1hour+) to complete the loop especially when looping such large pst file for the first time.

The purpose of this code is to get the PR_SEARCH_KEY and the FolderPath of each email (folder wise) and store them in text file which will be used in future, such as in a periodic full loop we can compare the old and new PR_SEARCH_KEY to find out the new emails etc

How can I get a faster performance in this.

Const PropName As String = "http://schemas.microsoft.com/mapi/proptag/0x300B0102"
Dim SourceFolder = Outlook.NameSpace.Folders("Outlook_Backup")
Dim PstName = "Outlook"
Dim MailCount = FunctionToGetTotalMailCount(SourceFolder)
Dim PrSearchKeyDictionary As New List(Of String)

ScanPST(SourceFolder,PstName,MailCount)

Public Sub ScanPST(f As Outlook.Folder, PstName As String, MailCount As Long)
'-========================================================================
'- f = RootFolder/PST with Tag (e.g Outlook_Backup), PstName (e.g Outlook)
'-========================================================================
If f.Items.Count > 0 Then
    For i = f.Items.Count To 1 Step -1
        LoopCount += 1
        Try
            Dim Mail As Object = f.Items(i)
            Dim PropertyAccessor As Outlook.PropertyAccessor = Mail.PropertyAccessor
            Dim PrSearchKey As String = PropertyAccessor.BinaryToString(PropertyAccessor.GetProperty(PropName))

            '/Full folder path splited into array and removed the rootfolder name with tag (e.g Outlook_Backup -> _Backup is the tag)
            Dim FolderPath() As String = f.FolderPath.TrimStart("\"c).Split("\").Skip(1).ToArray

            '/Array joined back to get MailFolderPath without tag
            Dim MailFolderPath As String = String.Join("\", FolderPath)

            PrSearchKeyDictionary.Add(PrSearchKey & "," & PstName & "\" & MailFolderPath)
            Marshal.ReleaseComObject(PropertyAccessor)
        Catch ex As Exception
            LogInput("[Error: " & ex.ToString & "]"
            Continue For
        End Try
        BGWorkerStatus = "[" & PstName & "] " & "Scanning... " & Math.Round((LoopCount / MailCount) * 100, 2, MidpointRounding.AwayFromZero) & "%"
        BackGroundWorker.ReportProgress(0, BGWorkerStatus)
    Next
End If
If f.Folders.Count > 0 Then
    For c = 1 To f.Folders.Count
        OL.Folder = f.Folders.Item(c)
        ScanPST(OL.Folder, PstName, MailCount)
    Next
End If
End Sub

After looking at your code I see some stuff which could speed this up if coded different.
'/Full folder path splited into array and removed the rootfolder name with tag (e.g Outlook_Backup -> _Backup is the tag)
Dim FolderPath() As String = f.FolderPath.TrimStart("\"c).Split("\").Skip(1).ToArray

'/Array joined back to get MailFolderPath without tag
Dim MailFolderPath As String = String.Join("\", FolderPath)

For each Item in f.Items you are trimming the FolderPath and Split it by \ to Skip the first arrayitem before you call ToArray(). The ToArray() isn't needed because this overload of String.Join would take an IEnumerable(Of String).
But the main point here is that f.FolderPath won't change for each Item. It would be much faster to pull this outside of the For loop.
Removing the BackGroundWorker.ReportProgress() for each and every Item would speed this up as well. How about reporting progress only for each folder?

Although VB.NET isn't case sensitive it would be good to follow the .Net Naming Guidelines for naming things.


Making a list human-readable How is this function?

/// Extension methods used by RedViewerLibrary.
internal static class Extensions
{
    /// A string array extension method that gets the human readable enumerated version of the array.
    /// <param name="items">The strings to act on.</param>
    /// <returns>A string containing the enumerated results.</returns>
    /// <example>null => ""</example>
    /// <example>{} => ""</example>
    /// <example>{"foo"} => "foo"</example>
    /// <example>{"foo", "bar"} => "foo, and bar"</example>
    /// <example>{"foo", "bar", "baz"} => "foo, bar, and baz"</example>
    internal static string GetEnumerated(this string[] items)
    {
        if (items == null || items.Length == 0)
        {
            return string.Empty;
        }
        else if (items.Length == 1)
        {
            return items[0];
        }

        const string COMMA_SPACE = ", ";
        const string AND_STRING = "and ";

        var totalNamesLength = items.Aggregate(0, (oldValue, name) => name.Length + oldValue);
        var numberOfCommaSpaces = items.Length - 1;
        var commaSpacesLength = numberOfCommaSpaces * COMMA_SPACE.Length;
        var predictedLength = totalNamesLength + commaSpacesLength + AND_STRING.Length;
        var sb = new StringBuilder(predictedLength);

        int loopCount = items.Length - 1;
        sb.Append(items[0]);
        for (int idx = 1; idx != loopCount; ++idx)
        {
            sb.Append(COMMA_SPACE);
            sb.Append(items[idx]);
        }

        sb.Append(COMMA_SPACE);
        sb.Append(AND_STRING);
        sb.Append(items[items.Length - 1]);
        return sb.ToString();
    }
}

It's ok.  A bit hard to follow however since you're taking steps that really aren't necessary.

It is not necessary to give the StringBuilder a starting capacity.  Just let it do its business and you do yours, it will just work.

However if you really wanted to do this, then that's not a problem.  You can clean it up a bit.  Rather than aggregating over the items when adding up all the lengths, just use Sum() instead.

var totalNamesLength = items.Sum(s => s.Length);

Don't really have much of a comment on the rest of your code.  I'd be careful of using ==/!= comparisons in a loop that goes over consecutive values, if the loop variable were to ever change in the body of the loop, you'll have a hell of a time trying to debug any problems you have with that.  I'd stick to using < (or appropriate operator) there exclusively.

for (int idx = 1; idx < loopCount; ++idx)
    // ...

I'd write this differently however as this is much more readable to me and compact.

internal static string GetEnumerated(this string[] items)
{
    if (items == null)
        return "";

    if (items.Length <= 1)
        // concatenates all the (0 or 1) items into a single string
        return String.Concat(items);

    // place all but the last in a comma-separated string
    var commaSeparated = String.Join(", ", items.Take(items.Length - 1));
    // include the last item
    return commaSeparated + ", and " + items.Last();
}

Since we know we are dealing with arrays, you won't be paying for much in terms of performance in the LINQ calls.  If that bothers you, it is simple to write the equivalent without using LINQ.

internal static string GetEnumerated(this string[] items)
{
    // I'd leave these in separate conditions
    // to make it clear they are separate cases
    if (items == null)
        return "";
    if (items.Length <= 1)
        return String.Concat(items);

    var commaSeparated = String.Join(", ", items, 0, items.Length - 1);
    return commaSeparated + ", and " + items[items.Length - 1];
}


Apply stock quote changes using observer pattern I am implementing observer pattern(refering codeproject) for stock quote update. I have created an interface for Subject and Observer. I have created instances for Subject and Observer and also the functionality to register, notify and update. Kindly let me know the approach is correct

public  interface IObserver
    {
        void update(double value);
    }
}

//subject

public interface ISubject
    {
         void Register(IObserver o);
         void UnRegister(IObserver o);
         void notify();

    }

//Observer instance

public class InfosysStock : IObserver
    {
        private ISubject _subject;
        private double _latestvalue = 0;
        public InfosysStock(ISubject subject)
        {
            _subject = subject;
            _subject.Register(this);
        }

        public void update(double value)
        {
            _latestvalue = value;
            display();
        }

        private void display()
        {
            Console.WriteLine("The latest value is : " + _latestvalue);
        }

        public void unsubscribe()
        {
            _subject.UnRegister(this);

        }
    }

//Subject Instance

public class StockMarket : ISubject
    {

        List<IObserver> observers = new List<IObserver>();
        public int _stockvalue = 0;

        public void setValue(int v)
        {
            _stockvalue = v;
            notify();
        }

        public void notify()
        {
            foreach (var observer in observers)
            {
                observer.update(_stockvalue);
            }
        }

        public void Register(IObserver o)
        {
            observers.Add(o);
        }

        public void UnRegister(IObserver o)
        {
            int idx = observers.IndexOf(o);

            observers.RemoveAt(idx);

        }

    }

Finally the main class

StockMarket stockmarket = new StockMarket();
InfosysStock infy = new InfosysStock(stockmarket);
stockmarket.setValue(15);
stockmarket.setValue(21);

            infy.unsubscribe();
            Console.ReadLine();

1. If you are implementing this pattern for learning, then it is OK.

Otherwise, you can use built-in Observer Pattern of C#.

2. Think about readable:

    public void UnRegister(IObserver o)
    {
         int idx = observers.IndexOf(o);

         observers.RemoveAt(idx);

     }

 }

There are some redundant empty lines in your code. Inaddition, "o" is not a good variable name. I think you can change it to "observer".

3.You don't need to retrieve index of an object and then you remove it via the index. It will reduce the performance:

      public void UnRegister(IObserver o)
      {
          int idx = observers.IndexOf(o);

          observers.RemoveAt(idx);

       }

You can use Remove() method to remove an object from the list directly:

    public void UnRegister(IObserver o)
    {
        observers.Remove(o);
    }


Setting a default value if a variable is empty This is the pattern I've been using for "if the variable is empty, set a default value", and for variables like $muffin, it has all seemed well and good.  But in the following real example, this pattern gives me a super long line which smells a bit to me.  Is there a cleaner way to do this?

$newsItems[0]['image_url']= $newsItems[0]['image_url']=='' ? '/img/cat_placeholder.jpg' : $newsItems[0]['image_url'];

Maybe:

$newsItems[0]['image_url']= $newsItems[0]['image_url']=='' ?
    '/img/cat_placeholder.jpg' :
    $newsItems[0]['image_url'];   //this doesn't look much better to me?

Before dealing with your question, I'd like to point out that you probably want to use empty() instead of comparing with an empty string and that if you really want to compare to an empty string, use the === operator.

Anyway, default initialization is a real problem in many languages. C# even has a ?? operator to solve this kind of issue! So, let's try to make it better to make your code better step by step.

Ternary operator no more

The one-liner is too long, and using the ternary operator on multiple lines is a bad idea: that's what if-statement are for:

if (empty($newsItems[0]['image_url'])) {
    $newsItems[0]['image_url'] = '/img/cat_placeholder.jpg';
}

This is more readable for two reasons : it's easier to understand the syntax, but more importantly, it's easier to see that you simply want a default value when there's nothing. We don't need the 'else' part which is confusing with the ternary operator.

Note: as mentioned by Simon Scarfe and corrected by mseancole, PHP also has some special ternary syntax to do this:

$newsItems[0]['image_url'] = $newsItems[0]['image_url'] ?: 'img/cat_placeholder.jpg';

Factorize it!

If you're doing this only once or twice, then all is good, but otherwise you'd want to factorize it into a function, since you don't repeat yourself, right? The simple way is:

function default_value($var, $default) {
    return empty($var) ? $default : $var;
}

$newsItems[0]['image_url'] = default_value($newsItems[0]['image_url'], '/img/cat_placeholder.jpg');

(The ternary operator does make sense here since variable names are short and both branches of the condition are useful.)

However, we're looking up $newsItems[0]['image_url'] when calling default_value, and this is possibly not defined, and will raise an error/warning. If that's a concern (it should), stick to the first version, or look at this other answer that gives a more robust solution at the expense of storing PHP code as a a string and thus cannot be checked syntactically.

Still too long

If we don't care about the warning/error, can we do better? Yes we can! We're writing the variable name twice, but passing by reference can help us here:

function default_value(&$var, $default) {
    if (empty($var)) {
        $var = $default;
    }
}

default_value($newsItems[0]['image_url'], '/img/cat_placeholder.jpg');

It's much shorter, and feels more declarative: you can look at a bunch of default_value calls and see what the default values are instantly. But we still have the same warning/error issue.


Suggestions for better oop designs with real example (file services) I'm watching Uncle Bob to improve code maintainability and readability.

I've built a system in which can upload various file formats. Bellow I've posted the pdf one.

The below code works perfect but I need help if there is a way to improve code design and quality.

Any suggestion is welcome.

I use an abstract class FileServices:

abstract class FileServices
{
    protected $file;
    protected $fileName;

    public function __construct($file)
    {
        $this->file = $file;
        $this->fileName = pathinfo($this->file->getClientOriginalName(), PATHINFO_FILENAME);
        $this->setNewFileNameIfExist();
        $this->saveToDB();
        $this->saveToServer();
    }

    public function saveToServer()
    {
        $path = $this->path;
        $realNameWithExtension = $this->fileName  . $this->fileExtension;
        Storage::put($path . $realNameWithExtension, File::get($this->file));
    }

    abstract function saveToDB();
    abstract function setNewFileNameIfExist();
}

And PDF child class:

class PDF extends FileServices
{
    protected $fileExtension = '.pdf';
    protected $path = 'public/pdf/';

    public function saveToDB()
    {
        $newFile = new \App\PDF();
        $newFile->title = $this->fileName . $this->fileExtension;
        $newFile->save();
    }

    public function setNewFileNameIfExist()
    {
        $counter = 0;
        $realName = $this->fileName;
        $allPDF = \App\PDF::all();
        foreach ($allPDF as $pdf) {
            if($pdf->title === $this->fileName . $this->fileExtension) {
                while($pdf->title === $this->fileName . $this->fileExtension) {
                    $this->fileName = $realName . '(' . $counter . ')';
                    $counter++;
                }
            }
        }
    }

}

In the client:

$file = $request->file('pdf');
new PDF($file);

Many thanks to @tim for his answer.

Protected Field vs Getter and setter

I prefer to not access fields directly, because it's difficult to use and not very flexible.

When I need to implement a new fileservice, I just need to know that I need to define a field called fileExtension. If you do not thoroughly document this, implementing new services in the future will be a pain (and even with documentation it's not ideal, as nobody really reads it anyways).

The second problem is that I can only define a string as file extension. What if I want to create an image file service, that allows the uploading of jpg and png files? I would need to dynamically set fileExtension, and the only place I could do that is in saveToDB or setNewFileNameIfExist, neither of which should really be responsible (and even though it may work right now, it may easily break once you change your base FileServices class).

It would be a lot better to just add an abstract getter for the file extension.

The same problem exists with path as well, and also works the other way around: in PDF you work directly on fileName, instead of using some setter method.

Field location

It is odd that you store the filename in FileServices, but the fileExtension and path in PDF. Just put them all in FileServices to simplify things.

File class

You may also consider creating a File class, which could hold file, fileName and fileExtension, as they all belong together. It may also contain common functions such as the one to get a filename (without extension).

Various Store Methods

I think that it is not ideal to have saveToDB and saveToServer in your base service. What if I only want to save the file in the database, but not the server?

Instead, I would have a generic, abstract storeFile function in FileServices.

Need for abstract FileServices?

Of course, if you follow the advise above, it's not clear that FileServices is actually needed as an abstract class. The only thing it does is provide a function to get the name of a file (which may as well be a utility function in some file utility class, as it is likely used elsewhere as well), and a function to save a file to the server (which is really just a call to a different function.)

If you expect more common functionality in the future, this isn't really a problem, otherwise you may consider changing the abstract class to an interface.

Misc

the PDF class isn't really a PDF, but a specific kind of file service, so PDFFileService would be a better name.
I prefer constructors to not do the actual work. It seems unclean, and the resulting calling code looks wrong. Reading new PDF($file), I would assume that a new object was created, but not that anything else was actually done. Something like new PDF($file)->store() would read more logical to me.
Your variable names are not consistent with what you consider real names. In saveToServer it's actually the new file name, in setNewFileNameIfExist, it's the original file name.


Bash command helper in C Previously, I had written this program in C++ and asked about it here on Code Review. After spending some time with C++, I desired to become more familiar with its namesake. This is my first C program.

The purpose of the program is to help with remembering complex bash commands. It's invoked with arguments describing the desired operation and displays the commands matching the input ordered by closest match.

I'm trying to stick as close as possible to ANSI C, and I'm still not clear on when to use char [] or char *.

#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define MAX_TEXT_LENGTH 100
#define MAX_DESC_LENGTH 100
#define MAX_KEYWORDS_LENGTH 100
#define NUM_COMMANDS 8
#define NUM_RESULTS 3

struct command {
    char text[MAX_TEXT_LENGTH];
    char description[MAX_DESC_LENGTH];
    char keywords[MAX_KEYWORDS_LENGTH];
};
struct command commands[NUM_COMMANDS] = {
    { "amixer -Mq set Master 1%-", "decrease volume", "lower" },
    { "amixer -Mq set Master 1%+", "increase volume", "raise" },
    { "makepkg -sri", "build and install a package using a PKGBUILD" },
    { "makepkg -efi", "rebuild and reinstall a package using a PKGBUILD" },
    { "pacman -Qdt", "list orphaned packages", "find" },
    { "pacman -Qe", "list explicitly installed packages", "find" },
    { "pacman -Ql [package]", "list files owned by package", "find" },
    { "pacman -Qo [file]", "list packages that own file", "find owned" }
};
struct result {
    int found;
    char text[MAX_TEXT_LENGTH];
    char description[MAX_DESC_LENGTH];
};
struct result results[NUM_COMMANDS];

int compare_results(const void *, const void *);
void convert_lower(char []);

int main(int argc, char *argv[])
{
    char current[MAX_TEXT_LENGTH + MAX_DESC_LENGTH + MAX_KEYWORDS_LENGTH];
    int a, c, found, r;

    /* convert argv to lowercase */
    for (a = 1; a < argc; ++a)
        convert_lower(argv[a]);
    for (c = 0; c < NUM_COMMANDS; ++c) {
        strcpy(current, commands[c].text);
        strcat(current, commands[c].description);
        strcat(current, commands[c].keywords);
        /* convert current to lowercase */
        convert_lower(current);
        found = 0;
        /* count words in both argv and current */
        for (a = 1; a < argc; ++a)
            if (strstr(current, argv[a]) != NULL)
                ++found;
        if (found == 0)
            continue;
        results[c].found = found;
        strcpy(results[c].text, commands[c].text);
        strcpy(results[c].description, commands[c].description);
    }
    /* sort commands with most words in common with argv at top */
    qsort(results, NUM_COMMANDS, sizeof(*results), compare_results);
    /* only print NUM_RESULTS results */
    for (r = 0; r < NUM_RESULTS && results[r].found > 0; ++r) {
        printf("%s\n", results[r].text);
        printf("    %s\n", results[r].description);
    }
    return 0;
}

int compare_results(const void *a, const void *b)
{
    struct result *r1 = (struct result *)a;
    struct result *r2 = (struct result *)b;

    return r2->found - r1->found;
}

void convert_lower(char string[])
{
    int c;

    for (c = 0; string[c] != '\0'; ++c)
        string[c] = tolower(string[c]);
}

* or []
There isn't really a definitive time when you should and shouldn't use pointers or arrays  (at least not that I know of). However, I generally only use arrays for:

Large buffers. For example, if you were reading a file.
A definitive size array of a simple type, such as an int or a char.
A group of constants all sharing the same type (your commands array, for example).

The two things are very similar. However, pointers are more idiomatic and C and I find are actually a lot easier to use.

Here is a great example of using pointers or arrays:

for (c = 0; string[c] != '\0'; ++c)
    string[c] = tolower(string[c]);

If instead of receiving an array through an argument you received a char *, you could do this:
for(; *string != '\0'; string++) {
    *string = tolower(*string);
}

This completely eliminates the need for c, and is a lot cleaner. Here, in the above snippet, we are taking the pointer string and increment through it each iteration.
Note that you still might be able to use an array in the arguments, it would just look a lot cleaner if you wrote it like this:
char *string

instead. I think it would still work either way (I did not test it), which is one of those gray areas between arrays and pointers.

const vs #define
Your #defines as the top should not be #defines:
const int MAX_TEXT_LENGTH = 100
const int MAX_DESC_LENGTH = 100
const int MAX_KEYWORDS_LENGTH = 100
const int NUM_COMMANDS = 8
const int NUM_RESULTS = 3

This is better practice than using #defines as those do not specify a direct type for the value; the pre-processor simply substitutes that number as a string into your code and it is up to the compiler to determine a type.

sprintf

    strcpy(current, commands[c].text);
    strcat(current, commands[c].description);
    strcat(current, commands[c].keywords);

Three separate function calls to concatenate three strings? That's not very efficient; and it's quite ugly. It would be better to use sprintf:
sprintf(current, "%s%s%s", commands[c].text, commands[c].description, commands[c].keywords);

Actually, this looks like a toString method for a command struct. Time for a refactor!
void buffer_tostring(char *buf, struct command);

Simply move that code (with slight adjustments) into a method like the one above; it is good to split your code into as many different (and logical) methods as you can so that everything follows the single responsibility principle, overall making your code more maintainable.
You could also do the same thing for your results struct regarding here:

    strcpy(results[c].text, commands[c].text);
    strcpy(results[c].description, commands[c].description);


Detecting arithmetic overflow in C with NASM This snippet is about detecting whether the carry flag is set or cleared. I work on Mac OSX, so my snippet supports Mac only.

First, we need a routine that does the job:

func.s:

global _read_carry_flag

_read_carry_flag:
    mov al, 0
    jnc end
    mov al, 1
end:
    ret

(Try nasm2 -f macho64 func.s for compiling into an object file.)

main.c:

#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>

#define A (2 * 1000 * 1000 * 1000)
#define B (1 * 1000 * 1000 * 1000)

extern bool read_carry_flag();

int main(int argc, char* argv[])
{
    int32_t a = A;
    int32_t b = B;
    int32_t ret = a + a + b;

    printf("%d\n", read_carry_flag());

    a = A;
    b = 1;
    ret = a + b;

    printf("%d\n", read_carry_flag());
    return 0;
}

(Try gcc -o prog main.c func.o for obtaining a process image.)

I would like to hear about possible improvements/extensions to the idea.

Wrong flag
I believe you should be looking at the overflow flag instead of the carry flag, since all of your operands are signed values.  On x86, the overflow flag is set if signed addition overflows.  The carry flag is set if unsigned addition overflows.
Not reliable
As @Edward pointed out, it doesn't seem reliable to use this kind of function because you never know how the compiler is going to rearrange your code.  Even without the compiler rearranging your code, the results could be confusing.  From your own example:
ret = a + a + b;
overflow = read_overflow_flag();

Here, you are only detecting overflow over the second of the two additions.  If the first addition overflowed but the second didn't, you wouldn't catch it.  In other words, the assembly might look like this:
add %ecx, %edx, %edx  // ret = a + a  <- overflow not detected here
add %ecx, %ecx, %ebx  // ret = ret + b
seto %al              // overflow = overflow flag

To do it correctly, I would suggest you use a function that adds two numbers together and updates a cumulative overflow:
// If the add overflows, 1 will be added to *pOverflow.
int32_t add32_with_overflow(int32_t x, int32_t y, int *pOverflow);

int32_t ret      = 0;
int     overflow = 0;

ret = add32_with_overflow(a,   a, &overflow);
ret = add32_with_overflow(ret, b, &overflow);
printf("Overflow = %d\n", overflow);


Making multiple copies of a pattern Here's a function for making multiple copies of a block of memory in another block of memory. It handles the case when the size of the target is smaller than or not a perfect multiple of the source pattern.

It proceeds by copying the pattern once, twice, four times, eight times, etc. until it can make a last 'massive' copy and fill the target buffer.

The protocol on this site is (apparently) to answer your own question with the improved code rather than edit original question. That makes sense otherwise other answers and feedback may be difficult to understand!

#include <string.h> //This is where memcpy() hangs out. I know! I know!

///Fills a block of memory with a specified pattern.
///The target block starts at pToFill and extends for pToFillSize bytes.
///The source pattern starts at pFillWith and extends for pFillWithSize bytes.
///Will fill the whole buffer including a trailing partial pattern at the end (if necesssary).
///pFillWithSize must not be 0. pFillWith must not be NULL.
///If Either pToFill is NULL or pToFillSize is 0, does nothing and returns without error.
///The area to fill and pattern must not overlap or behaviour is undefined.
void memcopyfill(void*const pToFill,const size_t pToFillSize,void*const  pFillWith,const size_t pFillWithSize){
    if(pToFill==NULL||pToFillSize==0){
        return;//Nothing to do.
    }
    if(pFillWith==NULL||pFillWithSize==0){
        return;//Can do nothing...
    }
    if(pToFillSize<pFillWithSize){
        memcpy(pToFill,pFillWith,pToFillSize);
        return;//Short buffer.
    }
    //The to buffer is bigger so we start with a full copy of pattern.
    //Now we keep doubling the copies by copying and copying from the target onto itself.
    char*lFillFrom=((char*)pToFill)+pFillWithSize;
    size_t lFilledSoFar=pFillWithSize;
    char*lFillEnd=((char*)pToFill)+pToFillSize;
    while(lFilledSoFar<(lFillEnd-lFillFrom)){//Overflow safe.
        memcpy(lFillFrom,pToFill,lFilledSoFar);
        lFillFrom+=lFilledSoFar;
        lFilledSoFar=lFilledSoFar<<1;//Doubling....
    }
    //No we can fill the end of the buffer in one final step. Could be half the job.
    memcpy(lFillFrom,pToFill,pToFillSize-lFilledSoFar);
}

Can it be bettered?

One use is to fill an array with a default structure.
If that is the purpose you don't need the initial fannying about with the short buffer special case.

There is no guarantee that NULL has a bit-pattern of all zeros so if you wanted to be ultra-ultra portable you ought to be doing something like this to get an array full of NULL.

I think most people use calloc(.) and don't even think about it.

If you use it for strings don't forget your null terminator!

Bonus question

Does anyone know of an extant platform where NULL doesn't have a zero filled bit-pattern?

I half remember one where it was 0xFFF...FF and that was clever because the hardware could detect overflow easily and handle all sorts of wrap-around errors gracefully. I'm damned if I can remember what it was. It may have been a hypothetical example to show why standards should give implementers freedom.

Here are some things I see that might help you improve your code:

Use more whitespace

Lines like this one:

char*lFillEnd=((char*)pToFill)+pToFillSize;

make my eyes bleed.

char *lFillEnd = ( (char*)pToFill ) + pToFillSize;

That version is much easier for humans to parse because of the judicious use of whitespace.

Use better names

The code has an odd characteristic in that every variable names begins with either p or l rendering the names unprounceable and impeding readability.  I can only guess that these might have been intended to signify parameter and local but those distinctions are quite obvious to anyone who knows C and not helpful.  Also memcopyfill isn't a terrible name, but perhaps patternfill would be more descriptive?  I'd suggest using memcpy inspired names that are terse but easier to read for the parameters:  dst, dstsize, src and srcsize.

Combine conditions for early bailout

It's good to have the early bailout in the case of NULL pointers or zero-length but it makes less sense for those to be two separate if statements.  Because of short-circuit evaluations, the resulting code will be just as efficient if you write just a single if statement and it is, I think, a little easier to see what is going on in the code.

Think carefully about size comparisons

The code currently has a line:

if(pToFillSize<pFillWithSize){

But shouldn't that comparison be <=?

Consider returning an error code

In the case of an early bailout, it might be handy to return an error code indicating that something was wrong.  This might also be used in the case that the buffers overlap, violating your stated calling conditions.

Don't use const inappropriately

Normally I find myself advocating for more use of const in code I review, but in this case, I see two instances where it might be better omitted.  Specifically, having the size parameters declared as merely size_t rather than const size_t makes sense and mirrors the declaration of the library routine memcpy.  Also, we are clearly going to alter the memory pointed to by pToFill so it's inappropriate to declare it const.

Remove the local variables

Most of the local variables are actually not needed in this code.  Don't forget that your parameters are actually local copies that can be used instead:

Put it all together

Here's one alternative that incorporates all of these changes:

bool patternfill(char *dst, size_t dstsize, const char *src, size_t srcsize)
{
    if (dst == NULL || dstsize == 0 || src == NULL || srcsize == 0) {
        return false;  //Can do nothing...
    }
    size_t remaining = dstsize;
    if (remaining <= srcsize) {
        memcpy(dst, src, remaining);
        return true;
    }
    // make one full copy
    memcpy(dst, src, srcsize);
    dst += srcsize;
    remaining -= srcsize;
    // now copy from destination buffer, doubling size each iteration
    for (src = dst; remaining > srcsize; srcsize *= 2) {
        memcpy(dst, src, srcsize);
        dst += srcsize;
        remaining -= srcsize;
    }
    // copy any remainder
    memcpy(dst, src, remaining);
    return true;
}

Perform tests

I wrote some simple test code to exercise the two versions of this function (with some minor changes to the original to have the same signature).  Here is the code.  First, a simple verification function to check that the pattern was successfully copied:

bool verifyfill(const char *buff, size_t buffsize,
        const char *pattern, size_t patternsize)
{
    if (buff == NULL || pattern == NULL || buffsize == 0 || patternsize == 0)
        return true;
    const char *pattend = (char *)pattern+patternsize;
    for (const char *p = pattern; buffsize && *buff == *p; buff++, --buffsize)
        if (++p == pattend)
            p = pattern;
    return buffsize == 0;
}

Then a bit more test harness to collect and compare the two functions:

typedef bool (*TestFunc)(char *, size_t, const char *, size_t);

bool checkpatternfill(TestFunc test, char *buffer, size_t buffsize,
        const char *pattern, size_t patternsize)
{
    memset(buffer, 'x', buffsize);
    test(buffer, buffsize, pattern, patternsize);
    return (verifyfill(buffer, buffsize, pattern, patternsize));

}

struct {
    TestFunc fn;
    const char *name;
} tests[] = {
    { memcopyfill, "original" },
    { patternfill, "improved" }
};

Finally, a main routine to check both accuracy and performance:

int main()
{
    const size_t buffsize = 100;
    const char pattern[]="Pattern";
    const size_t patternsize = strlen(pattern);
    const int testCount = sizeof(tests)/sizeof(tests[0]);
    char *buffer = malloc(buffsize);
    if (buffer == NULL) {
        perror("Can' allocate buffer");
        return 1;
    }

    // basic functionality tests
    for (int i=0; i < testCount; ++i) {
        if (checkpatternfill(tests[i].fn, buffer, buffsize, pattern, patternsize))
        {
            printf("%s Test passed\n", tests[i].name);
        } else {
            printf("%s Test failed!\n", tests[i].name);
            char *buffend = buffer+buffsize;
            for (char *p = buffer; p < buffend; ++p)
                putc(*p, stderr);
        }
    }

    // speed tests
    for (int i=0; i < testCount; ++i) {
        clock_t start = clock(), diff;
        for (int j=10000000; j; --j) {
            tests[i].fn(buffer, buffsize, pattern, patternsize);
        }
        diff = clock() - start;
        printf("%s took %f seconds\n", tests[i].name, 1.0 * diff / CLOCKS_PER_SEC);
    }
    free(buffer);
}

Test results:

On my 64-bit Linux box using gcc with -O2 optimization, I get this result:

 original Test passed
 improved Test passed
 original took 0.338862 seconds
 improved took 0.270134 seconds


Computing average without overflows and with half-decent precision At some point I needed to compute the average of a big collection of integers. I knew the size of the collection prior computation, but a naive average computation was prone to integer overflow while repeatedly adding every elements divided by the size was prone to loss of precision. To mitigate overflow and loss of precision (speed wasn't an issue), I came up with the following algorithm:

#include <cstdint>
#include <iterator>
#include <type_traits>

template<typename RandomAccessIterator>
auto average(RandomAccessIterator first, RandomAccessIterator last)
    -> long double
{
    static_assert(std::is_integral<
        typename std::iterator_traits<RandomAccessIterator>::value_type
    >::value, "average only available for built-in integer types");

    auto size = std::distance(first, last);
    if (size == 0) return 0.0L;

    std::intmax_t accumulator = 0;
    long double res = 0.0L;

    while (first != last) {
        std::intmax_t tmp;
        if (__builtin_add_overflow(*first, accumulator, &tmp)) {
            res += static_cast<long double>(accumulator) / static_cast<long double>(size);
            accumulator = *first;
        } else {
            accumulator = tmp;
        }
        ++first;
    }

    res += static_cast<long double>(accumulator) / static_cast<long double>(size);
    return res;
}

Basically, here is what the program does:

It initializes the result and an accumulator with 0.
It accumulates the values from the collection in an std::intmax_t accumulator as long as said accumulator does not overflow.
When the accumulator is on the verge of overflowing, the accumulator is divided by the size of the collection and added to the result. It then takes the value that would have made it overflow. The loop continues.
When then is nothing more to treat in the collection, the current value of the accumulator is divided by the size of the collection and added to the result.

I think that the goal of avoiding overflows is reached: the accumulation is guarded against overflow, and the division of every element to add by the size ensures that the result itself can't overflow either.

Regarding the precision, adding integers in an std::intmax_t as long as possible ensures that no precision is lost before the division. In the best case, a single division will occur. Moreover, the algorithm should divide bit integers most of the time, which may help the precision too. I picked long double to have the best precision guaranteed by the standard too.

Now, there are a few known shortcomings, to name a few:

Returning 0.0L is probably not the best solution when the collection is empty, but I'm not sure that returning NaN would be better either. One way to circumvent that would be not to handle the case, and document it as a precondition.
__builtin_add_overflow is a GCC builtin, not a standard C++ function. Unfortunately, I don't know any standard equivalent to it, and manually checking for overflows is hard to do correctly.

The algorithm obviously uses O(1) extra memory. Now, did I properly manage to correctly avoid overflow while mitigating precision loss? Is there anything else to be said about this code?

First, having a forward-iterator is good enough, don't ask for more. Yes, it might mean you have to go over the range twice (once to measure its size), but that's a minor irritant.
Just please, verify it is a forward-iterator like this:

std::forward_iterator_tag = std::iterator_traits<ForwardIt>::iterator_category{};

Use the right loop for the job. It's more readable, and more concise.
Mark any variable you can const to help comprehension and error-checking.
Next, you really have to improve your overflow-handling, as res += static_cast<long double>(accumulator) / static_cast<long double>(size); might loose a small amount of precision.
Stay in the integer-realm by adding / subtracting enough multiples of size to change the sign of accumulator.
Don't cast to long double unless you can guarantee perfect accuracy.
You can easily supplant the builtin with a bit of logic:
Test the sign of accumulate and depending on the result check whether MAXVALUE - accumulate > *first or MINVALUE - accumulate < *first.
Not sure whether the compiler can optimize that as well as the builtin though...
Consider whether making it SFINAE-friendly by avoiding hard errors might be worth the more verbose error-messages.

Modified code:

#include <cstdint>
#include <iterator>
#include <type_traits>
#include <limits>

template<typename ForwardIt>
long double average(ForwardIt first, ForwardIt last)
{
    static_assert(std::is_integral<typename std::iterator_traits<ForwardIt>::value_type>(),
        "average only available for built-in integer types");
    std::forward_iterator_tag = std::iterator_traits<ForwardIt>::iterator_category();

    const auto size = std::distance(first, last);
    if (!size) return 0;

    std::intmax_t accumulator = 0;
    long double res = 0;
    do {
        using snl = std::numeric_limits<decltype(accumulator)>;
        if (accumulator < 0 && snl::min() - accumulator > *first) {
            res += accumulator / size - 1;
            accumulator = accumulator % size + size;
        } else if (accumulator > 0 && snl::max() - accumulator < *first) {
            res += accumulator / size + 1;
            accumulator = accumulator % size - size;
        }
        accumulator += *first;
    } while (++first != last);
    res += accumulator / size;
    res += static_cast<decltype(res)>(accumulator % size) / size;
    return res;
}


Wiring classes together in PHP I've created a base structure for my project and found a way to wire classes together, I create an array of previously loaded classes and call them from the cache. I then have a static class that holds the cache and I call the static class when I need a class.

I feel that there is a much better way to do this, and maybe I'm doing something extremely wrong? How do other PHP applications do this? I'm pretty new to it, so I was wondering if anyone could advise me towards a better way to do this? It feels like it could be hugely improved.

Current directory structure:

The static class, I use this to get my classes (providers, I call them):

<?php declare(strict_types = 1);

namespace App;

class App {
    private static $providers = array();

    public static function getProvider($provider) {
        if (!isset(self::$providers[$provider])) {
            $providerName = 'App\Providers\\' . $provider;
            self::$providers[$provider] = new $providerName();
        }

        return self::$providers[$provider];
    }
}

Example of how I use it, I do this in all my controllers to show a view:

<?php declare(strict_types = 1);

use App\App;

namespace App\Controllers\Frontend\Guest;

class LoginController
{
    public function getView()
    {
        \App\App::getProvider('Template')->renderTemplate('index.html');
    }
}

Here is one of my few providers:

<?php declare(strict_types = 1);

namespace App\Providers;

class Template {
    private $twig;

    public function __construct() {
        $this->twig = new \Twig_Environment(new \Twig_Loader_Filesystem(ROOT . '/resources/templates'), array(
            'cache' => ROOT . '/storage/cache/templates',
        ));
    }

    public function renderTemplate($template) {
        echo $this->twig->render($template, array('the' => 'variables', 'go' => 'here'));
    }
}

Looks sort of like you're trying to accomplish a singleton pattern, using the App class to control them all. Try this approach instead:

<?php
// Base / abstract class that defines a basic singleton pattern
abstract class SingletonProvider
{
  // Singleton pattern
  protected static $_instance;
  public static function getInstance()
  {
    if(static::$_instance == null)
    {
      static::$_instance = new static();
    }
    return static::$_instance;
  }

  // Private constructor
  private function __construct() { }
}

// A specific provider class definition that inherits the singleton pattern
class MyProvider extends SingletonProvider
{
  // Class-specific instance reference
  protected static $_instance;

  // --- CLASS INSTANCE-SPECIFIC PROPERTIES AND METHODS BELOW HERE ---

  // Class properties
  public $FieldX;

  protected function __construct()
  {
    $this->FieldX = "Hello World!";
  }
}

// Another specific provider class definition that inherits the singleton pattern
class MyOtherProvider extends SingletonProvider
{
  // Class-specific instance reference
  protected static $_instance;

  // --- CLASS INSTANCE-SPECIFIC PROPERTIES AND METHODS BELOW HERE ---

  // Class properties
  public $FieldY;

  protected function __construct()
  {
    $this->FieldY = "Foo";
  }

  public function MakeMoreExciting($sentence)
  {
    return $sentence . "!!!!!!!!";
  }
}

// Usage examples
echo MyProvider::getInstance()->FieldX; // Hello World!
echo MyOtherProvider::getInstance()->FieldY; // Foo
echo MyOtherProvider::getInstance()->MakeMoreExciting("Hi there"); // Hi there!!!!!!!!

The "static" keyword is used for late static binding, which basically means that when you run MyOtherProvider::getInstance(), all the "static" references like "static::$_instance" resolve to the calling class, so it's like automatically determining either MyOtherProvider::$_instance or MyProvider::$_instance, depending on which class is being called.

The protected static $_instance is there to ensure that each class has its own property to hold the class instance.

In theory, you could even shorten all of this up into nothing but static methods and properties, too:

abstract class MyOtherProvider
{
  // Class properties
  public static $FieldY;

  public static function MakeMoreExciting($sentence)
  {
    return $sentence . "!!!!!!";
  }
}

echo MyOtherProvider::MakeMoreExciting("Hi there"); // Hi there!!!!!!

Although sometimes you might want an actual instance rather than just being completely static.


Splitting a string basing myself on categories I was thinking about splitting a string based on certain categories, for example alphabetic and numerical, lower and upper case, and even the 3 lines of the keyboard. I wrote a generic function for this but it seems unreadable, so I ask you for better modularization and improvements to readability.

import doctest

def split_on_changes(string, cats):
    """
    Given an input list and some categories,
    splits the list whenever the category changes.
    Also splits if the next char is in no categories.

    >>> split_on_changes("010010110", ["0", "1"])
    ['0', '1', '00', '1', '0', '11', '0']
    >>> split_on_changes("this is an alaskian example", ["qwertyuiop", "asdfghjkl", "zxcvbnm "])
    ['t', 'h', 'i', 's', ' ', 'i', 's', ' ', 'a', 'n ', 'alask', 'i', 'a', 'n ', 'e', 'x', 'a', 'm', 'p', 'l', 'e']
    >>> split_on_changes("MiXeD CASing ahahah", ["qwertyuiopasdfghjklzxcvbnm", "qwertyuiopasdfghjklzxcvbnm".upper()])
    ['M', 'i', 'X', 'e', 'D', ' ', 'CAS', 'ing', ' ', 'ahahah']
    """
    def same_cat(a,b):
        for cat in cats:
            if a in cat and b in cat: return True
    return ''.join([x + '·' if not same_cat(x,string[i+1]) else x
                            for i,x in enumerate(string[:-1])] + list(string[-1])).split('·')

if __name__ == "__main__":
    doctest.testmod()

Relying on special values (namely '·') not to be present in the input is a bad idea.  Your problem is basically a variant of itertools.groupby(), so you should use that.

from itertools import groupby

def split_on_changes(string, cats):
    """
    docstring and doctest here

    """
    def category(char):
        """
        Return the index of the category to which char belongs
        (or None if it does not belong to any category).
        """
        return next((i for (i, cat) in enumerate(cats) if char in cat), None)
    return list(''.join(group) for _, group in groupby(string, category))

The category() helper function is based on this technique to find the first element of a list that satisfies a predicate.  It could also be written as

def category(char):
    for i, cat in enumerate(cats):
        if char in cat:
            return i

Depending on what you want to do with the results, you might want to consider not calling list() and string.join().


Message Queue for inter-thread communication I wrote this little piece of code, a while back. The intention behind it was to create a system to send messages between consumer and producer threads. I have no idea for what i wanted to use it. I just found it in an old snippet folder of mine. The type of the queue is FIFO.

The usage
Create a queue with new MessageQueue<MessageType>().

let's assume we will store an instance of the message queue in an private field inside the classes that will be using it.
This field is called messageQueue

Use messageQueue.push(new MessageType([...])) in a producer thread to add a
Message.
Take it out of the queue from a consumer thread with messageQueue.pull().
Check if there is a message in the queue with messageQueue.hasMessage().

I know that I thought about implementing a messageQueue.peek() but I never implementet it.

The code

/**
 * Holds a queue of messages for inter-thread communication
 * Producer and Consumer should not be the same thread!
 *
 * @param <T> type of the messages
 */
public class MessageQueue<T> {

private static final int MAX_SIZE = 255;

    private T[] messages;
    private int lastReadIndex;
    private int lastInsertIndex;

    private int adjustIndex(int index) {
        if (index >= MAX_SIZE) {
            return 0;
        }
        return index;
    }

    /**
     * Constructor of the {@link MessageQueue}
     */
    @SuppressWarnings("unchecked") // Java can't create an Array of a generic type...
    public MessageQueue() {
        messages = (T[]) new Object[MAX_SIZE];
    }

    /**
     * Pushes a new entry into the queue.
     * This function will block when the queue is full.
     *
     * @param msg The entry to push on the stack
     * @throws InterruptedException
     */
    public synchronized void push(T msg) throws InterruptedException {
        int newIndex = adjustIndex(lastInsertIndex + 1);

        // Check if we can push the message
        while (messages[newIndex] != null) {
            wait();
        }

        // Push the message
        messages[newIndex] = msg;
        lastInsertIndex = newIndex;
        notifyAll();
    }

    /**
     * Pulls an entry from the queue.
     * The entry will be returned and removed from the queue.
     *
     * @return The entry from the queue
     */
    public synchronized T pull() {
        int newIndex = adjustIndex(lastReadIndex + 1);

        T shouldReturn = messages[newIndex];
        messages[newIndex] = null;
        lastReadIndex = newIndex;
        notifyAll();
        return shouldReturn;
    }

    /**
     * Checks if there is an entry in the queue.
     *
     * @return true, when there is an entry in the queue
     */
    public synchronized Boolean hasMessage() {
        if (lastInsertIndex > -1) {
            int newIndex = adjustIndex(lastReadIndex + 1);

            if (messages[newIndex] != null) {
                return true;
            }
        }

        return false;
    }
}

Nothing in-depth, just a few things that threw me off as a developer that would be using your class:

If you design a queue, use proper interfaces.
There are a few interfaces in Java, designed to offer standardized access to a Queue

public class MessageQueue<T> { // A queue should implement the Queue<E> interface, preferably BlockingQueue<E>

Mention constants in your documentation, together with their reason.
I have a hard time understanding your limit of 255 messages. As a developer that cannot look at private variables, I would be thrown off with errors once I go over that limit. Document it. Tell others about this limitation!

private static final int MAX_SIZE = 255; // Mention this constant in your documentation

Is there a reason to use the Boolean object type in favor of the boolean primitive? Using Boolean involves unnecessary boxing and might, if used excessively and in time-sensitive applications, introduce lower performance.

public synchronized boolean hasMessage() { // Use primitives when objects are not necessary

This isn't meant as an exhaustive list, but only my subjective opinion.


First Chess Project (ver 1.1) I'd already asked for my code to be reviewed earlier here

First Chess project)

Full code

Considering the suggestions put forth for that question, I spent a day and refactored my whole code. For those who have seen my previous question/code, could you you point out anything which I have as of yet not improved upon?

For everyone else, all I want to know is whether I am on the right track while programming in python. Could you please tell me if there is something unpythonic in my code? What parts are not written in the way "good Python code" is meant to be written? Here is a snippet I feel is not really good:

1. Taking input from user

This is in the main loop:

input_read=False

if len(m)==4:
  X1,Y1,X2,Y2 = m                           # assigning final co-ordinates
  m=[]                                  # empty the buffer input list
  input_read=True

if not input_read:
    events=pygame.event.get()
      for event in events:

          if event.type == pygame.QUIT:         # Quitting
              exit(0)

          if event.type == pygame.MOUSEBUTTONUP: pressed= False     # get ready for next click
          if event.type ==  pygame.MOUSEBUTTONDOWN and not pressed:
              pressed=True
              if event.button==1:
                  if len(m)==2:                                     # add second click co-ordinates
                      pos = pygame.mouse.get_pos()
                      x = pos[0]/80+1
                      y = pos[1]/80+1
                      m.extend([x,y])

              else:
                    pos = pygame.mouse.get_pos()                    # add first click co-ordinates
                    x = pos[0]/80+1
                    y = pos[1]/80+1
                    j=checkPositionToPieceDict((x,y))               # Checks that the first click has a piece
                    if j:                                           # of your colour and selects it.
                        if j.colour==colour:
                            selectedPiece=j
                            m.extend([x,y])

Could you please just go through the code and tell me if this is how Python is written in the industry?

This code seems much better than before, so congrats. Here are a few more things to adjust:

Move your main game loop into a function and call it in an if block like this:

if __name__ = "__main__":
    #call your main function

This is so that people (including yourself) importing your module don't immediately get a chess window popping up.
You factored out the Piece baseclass from all of the pieces, simplifying that code quite a bit. This is good! But now you should finish the job of merging the different coloured pieces into the same class. They are extremely close to identical. You can pass in all of the values that distinguish them (colour, position, etc.) in the __init__ method when you initialize them.
At this point, with the number of global variables and global functions operating on those variables, you should group them into a GameState object to encapsulate them.
I will second the recommendations to replace numbers that are clearly constants with variables to improve readability, as well as follow the style guidelines in PEP 8.

Beyond that, I think the changes we're looking at are either implementation specific (you would do it differently depending on what this is supposed to accomplish, and what your goals are), are too minor to bring up one by one, or down to personal coding style.


Join() equivalent function for F# sequences I am porting C# code to F# that makes use of LINQ's Join() extension method. Just as I use that in method call chains, I would of course like to have an F# function to pipe into. However, there is no equivalent in the Seq module, and while the same result could also be achieved with a nested lambda using Seq.where, I'd also want something that tells me by name that it is in fact a join.

I came up with this function using a simple query expression:

let seqJoin innerKeySelector outerKeySelector resultSelector (innerSequence : 'b seq) (outerSequence : 'a seq) =
    query {
            for outer in outerSequence do
            join inner in innerSequence on
                (outerKeySelector outer = innerKeySelector inner)
            select (resultSelector outer inner)
    }

While this works, I'm not sure about the idiomaticity of the API.

LINQ's Join() method has this signature:

public static IEnumerable<TResult> Join<TOuter, TInner, TKey, TResult>(
    this IEnumerable<TOuter> outer,
    IEnumerable<TInner> inner,
    Func<TOuter, TKey> outerKeySelector,
    Func<TInner, TKey> innerKeySelector,
    Func<TOuter, TInner, TResult> resultSelector)

Parameters are 'logically' ordered by importance, the first being the sequence on which the method is (syntactically) called. In F#, in order to be able to pipe the 'original' sequence into the function, that argument needs to be last instead of first, which requires shuffling the whole signature around a bit.

In order to keep the two sequences next to each other, I moved innerSequence to the second to last position, and to stay in line with their order, I also switched outerKeySelector and innerKeySelector. I wonder, though, whether once I've changed it that far, it might be more idiomatic to go all the way and make it the full reverse of the "C# version", in part because now the resultSelector parameter separates the sequences from their key selector functions and looks a bit out of place in that position. What would be the most 'natural'/idiomatic way to arrange those parameters?

As for the query itself, is that the proper way to do it with regards to laziness and expression trees that will be used under the cover? Will the seq type annotations break anything in that respect? Those were necessary to actually allow for using seq and list values as inputs.

(This is of course supposed to become an extension for the Seq module; I just haven't bothered to do that yet.)

I am porting C# code to F# that makes use of LINQ's Join() extension method. Just as I use that in method call chains, I would of course like to have an F# function to pipe into.

I think that using Join() directly in C# is actually not that common, since query syntax is much nicer for that. And I think you should consider the same thing in F#: you already know about query expressions and join, so why not use that?

Though I understand that function call chains are much more flexible, so this might not be a good option for you.

Have you considered removing the resultSelector altogether? Tuples are not very idiomatic in C#, but they are in F#, so you might want to use them.

Since you want to copy the behavior of Enumerable.Join(), I would use that to implement your method.

When considering the order of parameters, I think you should think about what is the most natural usage. I think it's this:

outer
|> Seq.join inner (fun o -> o.OKey) (fun i -> i.IKey)

This will make the order of parameters of the function quite weird, but I think it makes the most sense this way.

Putting this all together:

let join (innerSequence : 'b seq) outerKeySelector innerKeySelector (outerSequence : 'a seq) =
    outerSequence.Join(innerSequence,
        Func<_, _>(outerKeySelector), Func<_, _>(innerKeySelector),
        fun outer inner -> (outer, inner))

(Those Func casts are necessary, I think it's because F#'s weird rules for implicit casting from lambdas to delegates.)


Standardizing Controller + Service method signatures In my controllers, I have the following:

    [System.Web.Http.HttpPost]
    [Route("api/report")]
    public async Task<HttpResponseMessage> Report(ReportDto report)
    {
        await _profileService.SubmitReportAsync(User.Identity.Name, report);
        return Request.CreateResponse(HttpStatusCode.OK, true);
    }

Then in my service method (and in all service methods), I have the following:

public async Task<ApiResponseDto> SubmitReportAsync(string username, string ReportDto report)
{
    try // repeated
    {
       using (var db = new DbContext()) // repeated
       {
         // this is where the unique stuff is
       }
    }
    catch(Exception e){ // repeated
       HandleServiceLayerException();
    }
 }

My issue is that I'm repeating so much "signature" code in my service methods.  How can I simplify, keeping in mind that service methods have varying parameter signatures?

Just create a helper method for executing a query where you wrap the entire try/catch error handling and pass the query via a lambda expression:

private async Task<T> ExecuteAsyc<T>(Func<DbContext, T> query)
{
    try
    {
        using (var db = new DbContext())
        {
            return query(db);
        }
    }
    catch (Exception e)
    {
        HandleServiceLayerException();
        return default(T);
    }
}

Then you call it like this with a lambda:

public async Task<ApiResponseDto> SubmitReportAsync(string username, string ReportDto report)
{
    return ExecuteAsyc(context =>
    {
        // this is where the unique stuff is
    });
}


Format an Unsigned Byte to a Three ASCII Digits I'm working on a game for the Atari VCS and have been dusting off my 30+ year old skills in writing MOS 6502 assembly language.

The VCS runs at 1.19 MHz and is based on a MOS 6507 chip with some custom logic and 128 bytes only of RAM.

Outline of Problem

The game needs to display a value as 3 ASCII digits, but I'm working in full range signed and unsigned bytes, rather than BCD, so I now have the problem of converting from a full range byte to ASCII digits.

What I have so Far

I have been able to put the following together, and it appears to work. I'd welcome any tips for making it smaller, faster etc.

Currently the program is estimated to run in around 114+ cycles, just counting the instruction cycle counts with no branches taken.

;
; FormatUnsigned.asm
;
; 6502 routine to format an unsigned byte into a space
; followed by 2 decimal digits if < 100, or 3 decimal
; digits if >= 100.
;

        seg.u Variables
        org $80

char1   .byte                   ; output chars
char2   .byte
char3   .byte
Temp    .byte                   ; temp

        seg Code
        org $f000

TestFormat
        lda #211                ; sample value
        jsr FormatUnsigned      ; format
        brk                     ; halt

; A = value to format unsigned
FormatUnsigned:
        ldy #$20                ; default hundreds digit
        sty char1               ; is a space
; calculate char1
        ldy #$30                ; '0'
        sec                     ; set carry for sub loop
Sub100
        sbc #100
        iny                     ; y = '1', '2'
        bcs Sub100              ; loop whilst carry is not borrowed
        adc #100                ; add 100 back
        dey                     ; and take back the inc to y
        cpy #$30                ; if y is '0', just leave the space in there
        beq SkipHundreds
        sty char1               ; save '1' or '2' into char1
SkipHundreds
; format value < 100 into BCD
        tay                     ; save a in y
        lsr
        lsr
        lsr
        lsr
        and #$F                 ; get high nybble
        tax                     ; into x for indexing
        lda highNybble,x
        sta Temp                ; save in temp

        tya                     ; get value - 100s back
        and #$0f                ; low nybble
        tay                     ; low nybble in y
        cmp #$0a                ; <10?
        bcc NoSub10

        sbc #$0a                ; subtract 10
        tay                     ; save in y
        clc
        lda #$10                ; Add '10' to bcd saved value
        adc Temp
        sta Temp
        tya
NoSub10
        sed                     ; decimal mode
        adc Temp                ; add bcd value to 0-9 in a
        sta Temp                ; save bcd value
        cld                     ; leave decimal mode

Write2ndChar
        lsr                     ; get high nybble
        lsr
        lsr
        lsr
        and #$0f
        adc #$30
        sta char2               ; save 2nd character

Write3rdChar
        lda Temp
        and #$0f                ; get low nybble
        adc #$30
        sta char3               ; save 3rd character
        rts

highNybble
        .byte $00
        .byte $16
        .byte $32
        .byte $48
        .byte $64
        .byte $80
        .byte $96

The program works correctly for tested inputs, producing an output of $32 $31 $31 for the decimal value 211.

First observations.

Memory access is costly in terms of cycles. Your solution uses 10 of these. The program I present later on manages to just require 3 accesses to memory.
By placing the most used variables in the zero-page, you obtained the best possible memory access performance, but avoiding variables is preferable.
The FormatUnsigned subroutine uses some 76 bytes and the byte-variables add another 11 bytes. This is a lot for such an humble task. The routine I present later on will be written in 46 43 bytes and only add 3 byte-variables.

An opportunity for optimizing.

lsr
lsr
lsr
lsr
and #$F                 ; get high nybble

You don't need the and #$0F instruction because the 4 lsr instructions in a row already leave the high nibble empty. This shaves off 2 bytes from the program and reduces the number of cycles by 2. These benefits double because you used this construct twice throughout the program.

A unclear comment.

; 6502 routine to format an unsigned byte into a space
; followed by 2 decimal digits if < 100, or 3 decimal
; digits if >= 100.

From this comment I understand that numbers in the range [0,9] will be shown with 1 leading space and 1 leading zero. Wouldn't you agree this to be ugly?
The version I propose next will show 2 leading spaces on such numbers.

My version.

The worst case runs in 108 cycles. This happens with values from 190 to 199.
The best case for this code runs in just 44 cycles. Not surprisingly this is for inputs from 0 to 9.
I personally don't like the decimal mode of the 6502 and so this solution doesn't use it.

    ldx #$20    ;" "
    sec
    sbc #100
    bcc OK1
    ldx #$31    ;"1"
    sbc #100    ;           (1)
    bcc OK1
    inx         ;"2"
    sbc #100    ;           (2)
OK1
    adc #100
    stx char1

    ldy #$2F    ;"0"-1      (4)
    sec
Sub10
    sbc #10     ;           (3)
    iny         ;["0","9"]  (4)
    bcs Sub10
    adc #10
    cpy #$30    ;"0"
    bne OK2
    cpx #$20    ;" "        (5)
    bne OK2     ;           (5)
    ldy #$20    ;" "
OK2
    sty char2

    ora #$30    ;["0","9"]  (6)
    sta char3

(1)(2)(3) The carry flag is already set. No need to use sec before sbc.
(2) This instruction is meant to be a jump over the following adc #100. This would have required 3 bytes using jmp STXCHAR1 or 2 bytes using bcs STXCHAR1. In both cases it would also have taken 3 cycles to run. By writing a complementary subtraction I undo the effect of the following addition, and realise the shortest code and the least cycles!
(4) The peculiar value $2F is correct because the loop at Sub10 always runs at least once the iny instruction. Therefore 1 to 10 iterations of this loop will yield Y-register values from $30 to $39.
(5) This extra code takes care of the 2 leading spaces on numbers from 0 to 9.
(6) Normally here we would like to add 48 to the value in the accumulator holding [0,9]. Using the adc #48 instruction would have required a clc to give correct result. By using the ora #48 the result is equally correct but the code is 1 byte smaller and 2 cycles faster.

Still room for improvements.

In an effort to get rid of the separate sec instruction I came up with a 1 byte smaller solution. This also no longer contains the complementary adc #100 that you commented about.

    ldx #$20    ;" "
    cmp #100
    bcc OK1
    sbc #100
    ldx #$31    ;"1"
    cmp #100
    bcc OK1
    sbc #100
    inx         ;"2"
OK1
    stx char1

Nice, but wouldn't it be possible to combine a few things (the first sbc #100 and the second cmp #100)? Yes. Using sbc #200 and a conditional complementary adc #100 the code has yet again shrunk, this time by another 2 bytes.

    ldx #$20    ;" "
    cmp #100
    bcc OK1
    ldx #$32    ;"2"
    sbc #200
    bcs OK1
    adc #100
    dex         ;"1"
OK1
    stx char1

Some speed measurements.

All of these numbers relate to the first part of the program where the hundreds digit gets calculated.

 Input    Question  Answer1  Answer2  Answer3
-------   --------  -------  -------  -------
  0- 99     24        14       10       10      cycles
100-199     33        20       18       19      cycles
200-255     40        23       21       16      cycles
-------   --------  -------  -------  -------
Average    31.02     18.31    15.53    14.83    cycles
           21        20       19       17       bytes

For the average it is understood that all numbers from 0 to 255 have the same probability.


Binary to decimal converter in JavaScript This exercise with a test suite is from here. I did not intend to use ES6 features, it's plain old JavaScript. That's why I seek advice more on the good practices, performance side, than modern syntax. Basically anything to make this code more elegant, without ES6.

// Constructor
var Binary = function(binary) {
  this.binary = binary;
  this.base = 2;
};

// Transform the string input in a reversed array
Binary.prototype.reverseInput = function() {
  return this.binary.split('').reverse();
};

// Handle the conversion to decimal
Binary.prototype.toDecimal = function() {
  var output = 0;
      binaryArray = this.reverseInput();

  if (this.invalidInput()) {
    return output;
  }

  for (var i = 0, j = binaryArray.length; i < j; i++) {
    output += binaryArray[i] * Math.pow(this.base, i);
  }

  return output;
}

// Check if any character is not a 1 or a 0
Binary.prototype.invalidInput = function() {
  for (var i = 0, j = this.binary.length; i < j; i++) {
    if (this.binary[i] != '0' && this.binary[i] != '1') {
      return true;
    }
  }
}

module.exports = Binary;

Flawed exercise

Before I begin, I would like to point out that the exercise is flawed:

The task is not really converting the input from a binary to a decimal representation.  Rather, it is to convert it to a number, whose conventional string representation is rendered in base 10.  The test suite included this test:

xit('11010 is decimal 26', function() {
  expect(new Binary('11010').toDecimal()).toEqual(26);
});

… which could also have been written as:

xit('11010 is twenty-six', function() {
  expect(new Binary('11010').toDecimal()).toEqual(0x1a);
});

It is bad practice to return 0 for new Binary('junk').toDecimal().  Acceptable results would be to either throw an exception or return Number.NaN.

Review

Your invalidInput function either returns true or falls off the end.  Good practice would be to name it isInvalidInput, and have it explicitly return true or false.  Better yet, reverse the sense and call it isValidInput.  Even better, don't expose the validation function as a method at all — you don't really need one.

this.binary.split('').reverse() and Math.pow(…) seem extravagant.

Suggested solution

// Constructor
var Binary = function(binary) {
  this.binary = binary;
  this.base = 2;
};

Binary.prototype.toDecimal = function() {  // <-- Method name is a misnomer!
  var output = 0;
  for (var i = 0; i < this.binary.length; i++) {
    output *= 2;
    switch (this.binary.charAt(i)) {
      case '0': break;
      case '1': output += 1; break;
      default:  return 0;   // <-- This sucks, but the test suite requires it!
    }
  }
  return output;
}


Javascript condition optimization Can the following condition be optimized/simplified ?

if (!this.properties.width ||
    this.properties.width <= 0 ||
    !this.properties.height ||
    this.properties.height <= 0)
  return;

So basically if properties does not have a width or height property or the width or height property is less than or equal to zero then return

I'd simply flip it around:

if( !(this.properties.width > 0 && this.properties.height > 0) ) {
  return;
}

This will catch null, undefined, NaN, etc., etc.. It will allow numeric strings (provided they can be coerced to a number greater than zero), and of course it'll allow straight-up positive, non-zero numbers.

Update: There's one edge-case I've come across: Arrays. It's some unfortunate JavaScript weirdness to do with type coercion:

[] > 0;     // => false (so far so good)
[0] > 0;    // => false (as it should be)
[1] > 0;    // => true  (what?)
[1, 1] > 0; // => false (WHAT?)

So an array with just a single numeric element will be treated as a number when comparing.

Well, actually, as far as I can tell, the array is coerced to a string by coercing each element to a string, and joining them with a comma, and the joined string is then compared lexicographically coerced to a number in order to be compared. (Thanks to Robert in the comments for the correction)

[1] + 0;    // => "10"
[1, 1] + 0; // => "1,10"
"0" > 0;    // => false
"1" > 0;    // => true
"1,1" > 0;  // => false

I love JavaScript, but sometimes... jeez...


Determine all prime, palindrome string integers within a range I have written a program in Python 3x to determine all the prime, palindrome string integers in the range of two given numbers.

from math import sqrt

def check_prime(n):
    if n<2:
        return False
    for i in range(2,int(sqrt(n))+1):
        if n%i == 0:
            return False
    else:
        return True

for times in range(5):
    a,b = map(int,input().split())
    ct = 0
    for n in range(a,b+1):
        if str(n) == str(n)[::-1] and check_prime(n):
            ct += 1
    print(ct)

How can I make this program run faster, mainly optimizing my check_prime function so that it is faster than currently?

One suggestion: That sqrt is expensive, and you can eliminate it in a lot of common cases. For example: if any number except 2 is even, then it's not prime. Every second time you call check_prime, you're passing an even number.

So you can check n%2 before the loop, and eliminate that sqrt on half of your calls. You can extend this to check the first few primes explicitly, before starting your loop:

def check_prime(n):
    if n<2:
        return False
    if n%2==0:
        return n==2
    if n%3==0:
        return n==3
    if n%5==0:
        return n==5
    for i in range(7, int(sqrt(n))+1, 2):
        if n%i == 0:
            return False
    return True

You get diminishing returns by adding more checks. I benchmarked the above at about 2x as fast as your original code.


Groupby and moving average function in pandas works but is slow I have some time series data collected for a lot of people (over 50,000) over a two year period on 1 day intervals.  I want to applying a exponential weighted moving average function for each person and each metric in the dataset.  After calculating the moving average, I want to join the new values up with the existing values in the dataframe.  I have figured out how to do this on a small sample dataset, but I'm afraid that it's not optimized and therefore won't scale to my actual dataset. I have plenty of RAM available, so I'm not afraid of running out of space, but I am concerned with how long this could take to calculate for my entire dataset. Especially as I experiment with different spans and aggregation levels.

Thanks to issue #13966 for the starting place!

How can I optimize this code to better scale to a larger dataset?

import pandas as pd
import random
from datetime import datetime

# create some sample data
vals = ['A,' * 8, 'B,' * 8, 'C,' *8]
person = []
for x in vals:
    for y in x.split(','):
        if y != '':
            person.append(y)

metric1 = [random.random() for _ in range(0, 24)]
metric2 = [random.random() for _ in range(0, 24)]
dates = [datetime(2017,1,1), datetime(2017, 2, 1), datetime(2017, 3, 1), datetime(2017, 4, 1),
         datetime(2017,5,1), datetime(2017, 6, 1), datetime(2017, 7, 1), datetime(2017, 8, 1)] * 3

# load into a dataframe
df = pd.DataFrame({'person': person,
                   'metric1': metric1,
                   'metric2': metric2,
                   'timestamp': dates})

def run_ewm(df):

    # sort the dataframe by the timestamp
    df.sort_values('timestamp', inplace=True)

    # group the df by person
    grouped = df.groupby('person')

    # create a temporary list to hold frames
    frames = []

    # iterate over the groups and apply exp. weighted moving average
    for group in grouped.groups:
        frame = grouped.get_group(group)
        frame['metric1_emw'] = frame['metric1'].ewm(span=60).mean()
        frames.append(frame)

    # concat the frames for a new dataframe
    df_new = pd.concat(frames)

    return df_new

%timeit df_new = run_ewm(df)

/home/curtis/Program_Files/miniconda2/envs/py35/lib/python3.5/site-packages/ipykernel_launcher.py:15: SettingWithCopyWarning:
A value is trying to be set on a copy of a slice from a DataFrame.
Try using .loc[row_indexer,col_indexer] = value instead

See the caveats in the documentation: http://pandas.pydata.org/pandas-docs/stable/indexing.html#indexing-view-versus-copy
  from ipykernel import kernelapp as app

10 loops, best of 3: 101 ms per loop

Iterating in Python is slow, iterating in C is fast. So, it's best to keep as much as possible within Pandas to take advantage of its C implementation and avoid Python.

You can use apply on groupby objects to apply a function over every group in Pandas instead of iterating over them individually in Python.

df["metric1_ewm"] = df.groupby("person").apply(lambda x: x["metric1"].ewm(span=60).mean())

This gives me a run time of

  3.21 ms ± 181 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

You can then also apply this over multiple columns:

df[["metric1_ewm", "metric2_ewm"]] = df.groupby("person").apply(lambda x: x[["metric1", "metric2"]].ewm(span=60).mean())

With that being said, you could generalize and reduce your run_ewm function by passing it a dataframe and a list of columns to apply ewm to:

def set_ewm(df, cols):
   df.sort_values("timestamp",inplace=True)
   df[[c+"_ewm" for c in cols]] = df.groupby("person").apply(lambda x: x[cols].ewm(span=60).mean())

You could also just return the columns or copy the dataframe before setting ewm, then returning the new dataframe to avoid modifying the dataframe inplace.


CSS for a calculator I've a small assignment to do: style a given HTML file with CSS.

My result looks exactly as it should. But I'm not really satisfied with my style sheet. I'm sure there are better ways to achieve the same.

The different sizes are not specified. I had just an image as template.

How would a real web design solve this task?

jsFiddle

body > div {
    width: 404px;
    border: 1px solid black;
    padding: 8px;
}

.header {
    margin-bottom: 5px;
}

.header input {
    width: 400px;
    height: 40px;
    border: 1px solid black;
    text-align: right;
}

.numberFields {
    margin: 0;
    width: 360px;
    font-size:0;
    overflow: auto;
    float: left;
}

.numberFields input {
    background-color: green;
    border: 1px solid black;
    width: 120px;
    height: 40px;
}

.numberFields input[name="0"] {
    width: 360px;
}

.operations {
    margin-left: 4px;
    width: 40px;
    font-size: 0;
    display: inline-block;
}

.operations input {
    background-color: purple;
    border: 1px solid black;
    width: 40px;
    height: 40px;
}

.sum {
    margin-top: 5px;
}

.sum input {
    background-color: purple;
    border: 1px solid black;
    width: 404px;
    height: 40px;
}

            <input name="1" value="1" disabled />

            <input type="button" name="1" value="1" />
            <input type="button" name="2" value="2" />
            <input type="button" name="3" value="3" />
            <input type="button" name="4" value="4" />
            <input type="button" name="5" value="5" />
            <input type="button" name="6" value="6" />
            <input type="button" name="7" value="7" />
            <input type="button" name="8" value="8" />
            <input type="button" name="9" value="9" />
            <input type="button" name="0" value="0" />

            <input type="button" name="+" value="+" />
            <input type="button" name="-" value="-" />
            <input type="button" name="*" value="*" />
            <input type="button" name="/" value="/" />

            <input type="button" name="=" value="=" />

First, your CSS has no errors, which is very good.  You can check for errors at the W3C validator.

Looking through your CSS, it looks very neat, and I would only remove a little bit of unnecessary code.  Also, the display seems to be optimized for IE.  I set it for FireFox and Chrome, and it displays almost correctly in IE.  The trouble is, the box for displaying the output is visibly small in FireFox and Chrome.  With my changes, it is 1 pixel too large in IE, but it is not as noticeable.

First, I would just use the body element here, instead of body > div:

body > div {
    width: 404px;
    border: 1px solid black;
    padding: 8px;
}

Right here, I adding a small padding so the font would have a small amount of space between it and the border:

.header input {
    /* ... */
    padding-right: 2px;
}

I found I could remove the overflow: auto; lines without effecting the display.

In .operations, you don't need to set font-size: 0;.  I do not know why you need to do that to remove the spacing in the .numberFields class, but I was unable to get it working without it.  I do not like that as it is not clear why you need to set the font-size there, and there is probably a cleaner way to do it, but the usual trick of using a negative margin value isn't working, and neither did setting the padding to 0px.

I did not make any other changes; as I said above, your CSS is very clean and neat.


Django view function I have a view function that allows a user to Add / Edit / Delete an object. How does the following function look, and in what ways could I improve it? I was thinking about separating aspects of the function into different parts, but since no 'component' is more than four-five lines of code, I thought that would be a bit overkill.

@login_required
def edit_education(request, edit=0):
    profile = request.user.get_profile()
    education = profile.education_set.order_by('-class_year')
    form = EducationForm(data=request.POST or None, request=request)

    if request.method == 'POST':

        ##### to add a new school entry ######
        if 'add_school' in request.POST:
            if form.is_valid():
                new_education = form.save(commit=False)
                new_education.user = profile
                new_education.save()
                return redirect('edit_education')

        ##### to delete a school entry #####
        for education_id in [key[7:] for key, value in request.POST.iteritems() if key.startswith('delete')]:
            Education.objects.get(id=education_id).delete()
            return redirect('edit_education')

        ###### to edit a school entry -- essentially, re-renders the page passing an "edit" variable #####
        for education_id in [key[5:] for key, value in request.POST.iteritems() if key.startswith('edit')]:
            edit = 1
            school_object = Education.objects.get(id = education_id)
            form = EducationForm(instance = school_object, request=request)
            return render(request, 'userprofile/edit_education.html',
                        {'form': form,
                         'education':education,
                         'edit': 1,
                         'education_id': education_id}
                    )
        ##### to save changes after you edit a previously-added school entry ######
        if 'save_changes' in request.POST:
            instance = Education.objects.get(id=request.POST['education_id'])
            form = EducationForm(data = request.POST, instance=instance, request=request, edit=1)
            if form.is_valid():
                form.save()
                return redirect('edit_education')

return render(request, 'userprofile/edit_education.html', {'form': form, 'education': education})

And in my template, if this help to clarify anything:

{% for education in education %}

{{ education.school }} {% if education.class_year %}{{ education.class_year|shorten_year}}, {% endif %} {{ education.degree}}
        <input type="submit" name="edit_{{education.id}}" value='Edit' />
        <input type="submit" name="delete_{{education.id}}" value="Delete" />

{% endfor %}

The if conditions can be simplified.

fpv_list = [ item.partition("_") for item in request.POST.iteritems() ]
deletes = [ v for f,p,v in fpv_list if f == 'delete' ]
for education_id in deletes:
    whatever

edits = [ v for f,p,v in fpv_list if f == 'edit' ]
for education_id in edits:
    whatever

This removes the dependence on knowing the length of 'delete' and the length of 'edit' prefixes on the  buttons.


Heap Sort implementation time complexity I have implemented Heap sort, but i think the time complexity is more than (nlogn).
Could anyone explain me; what is wrong in my code which makes more time complexity.

I will be glad if the answer is in terms of algorithm time complexity based.

import java.util.Arrays;

public class HeapSort {
    public static void main(String[] args) {
        int[] array = { 5, 6, 10, 3, 9, 2, 12, 1, 8, 7 };
        maxHeapSort(array, array.length);
        System.out.println(Arrays.toString(array));
        System.out.println("\n\n");
    }

    private static void maxHeapSort(int[] array, int length) {
        for(int i = 1; i < length; i++ ) {
            maxHeapify(array, 1, length-i);
            swap(array, 0, length-i);
        }
        //Working on last three indexes [which is 0,1,2]
        correctNode(array, 1, 3);
        swap(array, 0, 2);
        correctNode(array, 1, 2);
        swap(array, 0, 1);
    }

    private static void maxHeapify(int[] array, int nodeIndex, int length) {
        if(nodeIndex < length) {
            correctNode(array, nodeIndex, length);
            maxHeapify(array, 2 * nodeIndex, length);
            maxHeapify(array, (2 * nodeIndex)+1, length);
            correctNode(array, nodeIndex, length);
        }
    }

    private static void correctNode(int[] array, int index, int length) {
        int rootIndex = index - 1;
        int leftIndex = (2 * index) - 1;
        int rightIndex = ((2 * index) + 1) - 1;
        if(leftIndex < length && array[rootIndex] < array[leftIndex]) {
            swap(array, rootIndex, leftIndex);
        }

        if(rightIndex < length && array[rootIndex] < array[rightIndex]) {
            swap(array, rootIndex, rightIndex);
        }
    }

    private static void swap(int[] array, int m, int n){
        int temp = array[m];
        array[m] = array[n];
        array[n] = temp;
    }
}

correctNode() looks like an (algorithmic) code-smell: its not needed. The only routines required to implement heap sort are:

swap
maxHeapify for maintaining the max-heap invariant; the topmost invocation must run in \$\mathcal{O}(\log n)\$.
buildMaxHeap, Introduction to Algorithms has a funky proof that this runs in linear time. Since, it is called only once, linear time is OK.
The actual heap sort routine: builds the max heap and pops in order until sorted.

The length argument of your heap sort is not needed. You can alwasy ask array.length.

I had this in mind:

public static void sort(final int[] array) {
    buildMaxHeap(array);
    int heapSize = array.length;

    for (int i = array.length - 1; i > 0; --i) {
        swap(array, 0, i);
        maxHeapify(array, 0, --heapSize);
    }
}

private static void buildMaxHeap(final int[] array) {
    for (int i = array.length / 2; i >= 0; --i) {
        maxHeapify(array, i, array.length);
    }
}

private static void maxHeapify(final int[] array,
                               final int index,
                               final int heapSize) {
    final int leftChildIndex = (index << 1) + 1;
    final int rightChildIndex = leftChildIndex + 1;

    int largestIndex;

    if (leftChildIndex < heapSize
            && array[leftChildIndex] > array[index]) {
        largestIndex = leftChildIndex;
    } else {
        largestIndex = index;
    }

    if (rightChildIndex < heapSize
            && array[rightChildIndex] > array[largestIndex]) {
        largestIndex = rightChildIndex;
    }

    if (largestIndex != index) {
        swap(array, largestIndex, index);
        maxHeapify(array, largestIndex, heapSize);
    }
}

private static void swap(final int[] array,
                         final int index1,
                         final int index2) {
    final int tmp = array[index1];
    array[index1] = array[index2];
    array[index2] = tmp;
}

Hope that helps.


Simplifying the if else statement I am trying to simplify the below if else statement, and I think it can be done.

Algorithm is like this -

  If user_ptr exists, then lock attrmap, find user_ptr. And if it
  is a batch, then don't ping. And else if user_ptr doesn't exists
  and if it is not a batch, then do other stuff.  But if it is a
  batch and user_ptr doesn't exists, then don't do anything.

With the above algorithm, this is what I've came up with, but I'm not sure whether it can be simplified or not.

if (user_ptr) {

    if(batchOrTimestamp != "batch") {
        user_ptr->ping(); // dont ping it if its a batch update
    }

    // lock attribute map from outside access and modification
    AttributeMap::GuardSPtr guard = user_ptr->m_attrMap.get_guard();
    user_ptr->m_attrMap.find(column_key.c_str(), actual_binary_value, attributeLength, schemaId, lastModifiedDate);
} else if(!user_ptr && batchOrTimestamp != "batch") {
    user_ptr = User::MakeUser(user_id);
    user_ptr->m_attrMap.find(column_key.c_str(), actual_binary_value, attributeLength, schemaId, lastModifiedDate);
    User::Find(user_ptr);
}

Any thoughts on this?

What you have is essentially:

if (a) {
   if (b) c();
   d();
}
else if (b) e();

Where:

a = user_ptr
b = batchOrTimestamp != "batch"
c = user_ptr->ping();
d = 2 lines, starting with `AttributeMap::GuardSPtr guard = user_ptr->m_attrMap.get_guard();`
e = 3 lines, starting with `user_ptr = User::MakeUser(user_id);`

One way to rewrite this is:

if (a && b) c();
if (a) d();
else if (b) e();

And another way is:

if (b) {
   if (a) c();
   else e();
}
if (a) d();

Which of these ways you prefer is up to you. The problem is that you have a total of three branches. One for a, one for b && !a and one for a && b.

I don't believe it's possible to simplify this in a better way (I would gladly be proven wrong).


Concatenating two IEnumerables with a limit I've been playing around with generators, generics and extension methods in C# (5.0) and wanted to create an extension method for IEnumerable, which would append another IEnumerable to it and allow a limit to be put on the whole thing.

The way this differs from just using Concat() and then Take() is that instead of passing an IEnumerable, you pass a function that takes an int (which represents the amount of elements that are needed at maximum from the second IEnumerable) and returns an IEnumerable.

Here's what I came up with:

using System;
using System.Collections.Generic;
using System.Linq;

public static class EnumerableExt
{
    public static IEnumerable<T> ConcatLimiting<T>(this IEnumerable<T> first, Func<int, IEnumerable<T>> getSecondWithLimit, int limit = int.MaxValue)
    {
        int elems = 0;
        foreach (T elem in first.Take(limit))
        {
            yield return elem;
            elems++;
        }

        foreach (T elem in getSecondWithLimit(limit - elems).Take(limit - elems))
        {
            yield return elem;
        }

        yield break;
    }
}

An example of a use case for this is when you need a limited amount of records from two different databases. You can write something like:

return GetEnumerableFromDatabase(someDatabase, 20)
    .ConcatLimiting(limit => GetEnumerableFromDatabase(someOtherDatabase, limit))
    .ToList();

My concerns:

Are the two IEnumerables going to be handled correctly, assuming that the resulting one is properly disposed of?

I have tried out quite a few possible scenarios and it seems to be handled correctly, but I'm not entirely confident.

Is the name ConcatLimiting() not very conform with what you usually see in C#?
Is this extension method completely useless?

Does an easy way to do this already exist?

Am I more or less following best practices?
Are there any other glaring issues?

Before answering your questions let me note that your example code is wrong, second parameter should be Func<int, IEnumerable<T>> directly IEnumerable<T>. If your concern is to do not retrieve unneeded elements from DB you probably need to let this task to Entity Framework (or your favorite ORM), it understands Take() and generate proper TOP clause.

Let me assume, in this example, that you're using EF (and Users and Employees table have same type...):

return dbContext.Users.Concat(dbContext.Employees).Take(20);

EF may generate something like this:

SELECT TOP 20 * FROM
(
    SELECT * FROM Users
    UNION ALL
    SELECT * FROM Employees
)

Now from QP you may verify what effectively will be generated. Almost-optimal expected result (what you're trying to do with your custom method) is something like this:

SELECT TOP 20 * FROM
(
    SELECT TOP 20 * FROM Users
    UNION ALL
    SELECT TOP 20 * FROM Employees
)

As you can imagine if (2) is ORM generated code (check with your) or ORM generates (1) but you verify QP and you see (2) then simplest proposed code is preferable. ORM will generate best SQL code (for its knowledge) and you will be able to defer query to the last moment (using expressions you can't call custom functions in this way) effectively losing some optimizations it may apply. Note that generated code for, let's say, SQL Server and MySQL may be pretty different (recall rules for nested LIMIT in MySQL.)

My favorite is still this code (unless you have a measured performance issue):

return dbContext.Users.Concat(dbContext.Employees).Take(20);

Let's say that you do need your extension method and you can't use proposed code. What should be changed?

First of all if called function respects its contract (to return at most limit items) then you do not need to reinforce this constraint.
You're not validating your parameters (and if you did you will have trouble because generated IEnumerable<T> does it when enumeration effectively happens, that's why so many *Impl() method proliferated.)

In code:

public static IEnumerable<T> ConcatLimiting<T>(this IEnumerable<T> first,
    Func<int, IEnumerable<T>> getSecondWithLimit,
    int limit = int.MaxValue)
{
    if (first == null)
        throw new ArgumentNullException(nameof(first));

    if (second == null)
        throw new ArgumentNullException(nameof(second));

    if (limit <= 0)
        throw new ArgumentOutOfRangeException(nameof(limit));

    var itemsFromFirst = first.Take(limit);
    return itemsFromFirst.Concat(getSecondWithLimit(itemsFromFirst.Count() - elems);
}

Third: I don't like guard values like Int32.MaxValue even when they're literally used. In this case you do not really want to get only 2,147,483,647 items but you're using a very big limit to...don't have limits.

Not to mention that useless TOP clause may slow down your query. See, for example but it's not the only one, Why SELECT TOP clause could lead to long time cost. What I'd do is then to replace int limit = Int32.MaxValue with a nullable argument and apply Take() only when required.

public static IEnumerable<T> ConcatLimiting<T>(this IEnumerable<T> first,
    Func<int, IEnumerable<T>> getSecondWithLimit,
    int? limit = null)
{
    // Arguments validation...

    if (limit == null)
        return first.Concat(getSecondWithLimit(null));

    var itemsFromFirst = first.Take((int)limit);
    return itemsFromFirst.Concat(getSecondWithLimit((int)limit - itemsFromFirst.Count());
}

For completeness some answers to your questions.

  Are the two IEnumerables going to be handled correctly...

Yes, compiler generated code is correct and it will dispose everything as expected. Note, however, that initialization code won't be executed until you effectively enumerate returned enumeration. This caused some funny bugs in code like File.ReadLines("path_to_file").Take(0).

  Is the name ConcatLimiting() not very conform with what you usually see in C#?

It has to be clear, waiting for elevator we may discuss if ConcatWithLimit() is better or not...

  Is this Extension Method completely useless?
  Am I more or less following best practices?
  Are there any other glaring issues?

Maybe yes, maybe not. See answer...


Trial Division Prime Generator import math

def generate_primes(limit):
    """Generates and prints a list of primes up to a given limit"""
    prime_list = [2, 3]
    num = 5
    increment_2 = True  # When True increment by 2. When False increment by 4

    while num <= limit:
        is_prime = True
        sqr_lim = math.sqrt(num)
        for x in prime_list:
            # Loop through list of prime numbers and check divisibility
            if x > sqr_lim:
                break
            if num % x == 0:
                is_prime = False
                break

        if is_prime:
            # Add primes to list to only check prime numbers
            prime_list.append(num)

        # Alternate step size between 2 and 4 to skip multiples of 2 and 3
        if increment_2:
            num += 2
            increment_2 = False
        else:
            num += 4
            increment_2 = True

    print(prime_list)
    return prime_list

def main():
    generate_primes(200)

if __name__ == "__main__":
    main()

I'm writing a program to generate a list of prime numbers to a given limit through the use of trial division. I was wondering if there is a way to further optimize this code. Currently it: skips multiples of 2 and 3; stores primes in a list and the list is used to check for primality; and checks divisibility up until the square root of the number it's analyzing.

I would also appreciate it if someone could critique me on my coding style such as comments, white space, and organization because I know its pretty rough around the edges. Thanks for the help.

The function generate_primes() shouldn’t print the list it generated.  The caller should print the returned value, if desired.

Calling generate_primes(2) will unexpectedly return [2, 3], as will generate_primes(1) or generate_primes(-500).

The increment_2 flag is awkward.  When True, increment by 2 seems ok, but when False the increment is ... what?  0?  1?

Instead of an increment_2 flag, you could use an increment amount, starting with 2 ...

increment = 2

... and then toggle that value between 2 & 4:

num += increment

# Alternate step size between 2 and 4
increment = 6 - increment

No more if ... else ..., reducing the complexity of the function slightly.

Stretch goal:

After generate_primes(200), if a call is made to generate_primes(500), you will recalculate all of the primes beneath 200.  Perhaps there is a way to cache the computed values for future generate calls?