SpellParser for a text-based RPG Fellow students are supposed to review and tell what my code is doing for a grade, this is why I'm asking on here first. I want to see if its clear enough.

public class SpellParser {

private static final String SPELL_PACKAGE_LOCATION = "edu.swosu.wordcraft.spells.";

         private Document dom;

         public SpellParser() {
                 setUpDom();
         }

            public void setUpDom(){
    DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
    try {
        DocumentBuilder docBuilder = dbf.newDocumentBuilder();
        dom = docBuilder.parse("resources/Spells.xml");
    } catch(Exception e) {
        e.printStackTrace();
    }
}

public void parse(){
    //get the root element
    Element docEle = dom.getDocumentElement();

    //get a nodelist of elements
    NodeList nl = docEle.getElementsByTagName("spell");
    if(nl != null && nl.getLength() > 0) {
        for(int i = 0 ; i < nl.getLength();i++) {

            //get the spell element
            Element el = (Element)nl.item(i);

            //get the Employee object
            Spell s = getSpell(el);

            //add it to list
            Spell.addSpell(s);
        }
    }
}

Your comments don't help much.  They should explain why you're doing what you're doing, not just paraphrase the code.  If you can't tell the reader anything more than the code already does, then a comment is pretty much just noise.
setUpDom() should probably be private.  Or possibly, done away with altogether, and its contents moved into one of the other methods.  It's doing stuff that looks like it should only be done once per XML document, and thus either once over the whole lifetime of the object or once each time you parse.  But it shouldn't be done by anything but your code.
If you can't handle an exception, you shouldn't be catching it.  And "printing a stack trace" is not handling.  :P

Once setUpDom() fails, the object is broken, so it doesn't make much sense to continue.  Let the exceptions propagate (declaring them in a throws clause instead), or throw your own exception type that describes what went wrong, including e as the inner exception.
You don't need to check that nl is an object, or worry about the length.  The Document class should always hand you back a NodeList, and your for loop will already loop 0 times if the list is empty, as the condition i < nl.getLength() will be false before the very first iteration.
The getSpell function and Spell class don't seem to be here.  So there's not a whole lot more to say, other than "fix your indents".  (I half-expect that that is a copy/paste issue, but it should still be mentioned.)

It does appear that Spell has some static variables, though, which is usually not a great solution.  And you're hard-coding where the data comes from and where it goes.

I'd personally suggest a SpellBook class or the like, that holds spells.  You could pass an instance of such a type (and the name of the XML file) either to your constructor or to parse.  Among other things, that leaves open the possibility for multiple sets of spells.

If that's one of your classmates' part, though, and you can't change that...eh.  Carry on.


Running parallel tasks until they're done The General Idea

I wish to simulate something which works like Java's ThreadExecutor, but as a first step, I would like to run a fixed number of identical tasks in parallel.
The next step should be queuing different tasks.

The method in question

public static async Task RunTasks(Func<Task<bool>> action, int tasks, int retries, string method)
{
    // Its just a source in order to use the library.
    byte[] source = new byte[tasks];

    // Wrapping the method with a retry mechanism
    async Task NewAction(byte _)
    {
        await DoWithRetries(async () => await action(), retries, method);
    }

    // Running everything
    await source.ParallelForEachAsync(NewAction, tasks);
}

private static async Task<bool> DoWithRetries(Func<Task<bool>> action, int retryCount, string method)
{
    while (true)
    {
        if (retryCount <= 0)
            return false;

        try
        {
            bool res = await action();
            if (res)
                return true;
        }
        catch (Exception e)
        {
            LogFactory.General.Fatal(e, method);
        }

        retryCount--;
        await Task.Delay(200);
    }
}

External Libraries

I used the AsyncEnumerator Nuget (which allows the ParallelForEachAsync call)

Code Execution

int page = -1;
int tasks = 10;
long count = 0;

await RetryComponent.RunTasks(async () =>
{
    int localPage = Interlocked.Increment(ref page);
    int results = -1;

    while (results != 0)
    {
        Console.WriteLine($"Page: {localPage}");

        // req.IsSuccess holds if the request processed well
        // mData contains the data returned from db (List)
        var req = await FetchSomeListFromDb();
        if (!req.IsSuccess)
            continue;

        foreach (var item in req.mData)
        {
            // simluate some IO..
            Interlocked.Increment(ref count);
        }

        // shuts down if zero
        results = req.mData.Count;

        // next page
        localPage = Interlocked.Increment(ref page);
    }

    return true;

}, tasks, 1, MethodBase.GetCurrentMethod().Name);

The issues I have with this code:

It seems like someone 100% did it already in the past, but I couldn't find anything like that or like the Java's ThreadsExecutor I mentioned
It seems like the execution code (The code I used to run everything) is large and seems like it will repeat itself..especially the page increment and the while handling (the last 2 statements at the end of the while block)

I will be glad to get a feedback on it or suggestions if the code exists somewhere (Either my implemetation or something similar to Java's ThreadsExecutor)

1) The library you are trying to use clearly does not fit your requirements. So why force it? Aren't you essentially just calling:

var tasks = Enumerable.Range(0, tasks).Select(i => DoWithRetries(action, retries, method));
await Task.WhenAll(tasks);

2) Fatal error is an error that your software cannot recover from. If you just swallow it in retry loop and keep going, then it is not fatal.

3) Delay duration should probably be a parameter and not a hardcoded value.

4) You can use nameof operator instead of MethodBase.GetCurrentMethod().Name.

P.S. I don't know what ThreadExecutor is and I didn't understand your use-case example, so I won't comment on those.


Employing 3DES algorithm in Java I am making a project just for fun that employs the 3DES algorithm in Java. I was wondering if my algorithm looks secure. Is there any advice or feedback you could give me? Do I need to include an IV? Am I transferring the salt successfully?

My code can encrypt a file using the Triple DES algorithm.

// author @Alex Matheakis
public static void main(String[] args) throws Exception {

    // file to be encrypted
    Scanner inputScanner = new Scanner(System.in);
    System.out.println("Enter filename");
    String filename = inputScanner.nextLine();

    FileInputStream inputFile = new FileInputStream("C:\\Documents\\Encryptor\\" + filename + ".txt");

    // encrypted file
    FileOutputStream outputFile = new FileOutputStream("C:\\Documents\\Encryptor\\encryptedfile.des");

    // password to encrypt the file
    String passKey = "password";
    byte[] salt = new byte[8];
    Random r = new Random();
    r.nextBytes(salt);
    PBEKeySpec pbeKeySpec = new PBEKeySpec(passKey.toCharArray());
    SecretKeyFactory secretKeyFactory = SecretKeyFactory
            .getInstance("PBEWithSHA1AndDESede");
    SecretKey secretKey = secretKeyFactory.generateSecret(pbeKeySpec);

    PBEParameterSpec pbeParameterSpec = new PBEParameterSpec(salt, 99999);
    Cipher cipher = Cipher.getInstance("PBEWithSHA1AndDESede");
    cipher.init(Cipher.ENCRYPT_MODE, secretKey, pbeParameterSpec);
    outputFile.write(salt);

    byte[] input = new byte[64];
    int bytesRead;
    while ((bytesRead = inputFile.read(input)) != -1) {
        byte[] output = cipher.update(input, 0, bytesRead);
        if (output != null)
            outputFile.write(output);
    }

    byte[] output = cipher.doFinal();
    if (output != null)
        outputFile.write(output);

    inputFile.close();
    outputFile.flush();
    outputFile.close();
    inputScanner.close();
    System.out.println("File has been Encrypted.");
}

It's kind of secure, but it uses older algorithms.

Although Benjamin correctly identifies 3DES, I would not call 3 key triple DES "broken". It still delivers a security of about 112 bits which nobody sane will try and break.

There is a chance that somebody would try and break your password though, and the shown password is clearly not random enough as it only contains 12 lowercase characters from a 26 character alphabet, which translates in 4.7 * 12 = 56 bits of entropy (each fully random letter delivers about 4.7 bits of entropy, 5.7 if upper and lowercase are randomly mixed). It may be that the relatively high number of iterations (99,999 iterations) will save you, but you're only supplying the 3DES key with half the entropy it requires to obtain the 112 bit security, so that's not enough.

The derivation method is probably secure, but it likely also performs too many operations which may just benefit an adversary. You are much better off with a more modern key derivation method such as Argon2. Likewise, we generally try and use authenticated encryption nowadays instead of the underlying CBC mode encryption. Problem is that there is no such prebuild solution directly available from the Java API, so you'd have to implement a copy of a protocol yourself or use a good library. Fernet would e.g. give you a more modern format.

You may want to include a version number to your encrypted messages so you can upgrade your algorithms or iteration count / salt size (etc.) at a later date. That way you can recognize older ciphertext, decrypt it, re-encrypt it with the newer protocol or keys and finally securely erase the old ciphertext. Or you could add additional encryption by encrypting it again using a different key derived from the password (a bit harder to do and to decrypt, but certainly possible).

SHA-1 has been broken, but not enough for it to become a problem for PBE. Of course you should still try and avoid old algorithms such as 3DES and SHA-1 and replace them with new ones such as AES and SHA-256.

Oh, almost forgot. Triple DES has a block size of 64 bits / 8 bytes, which is deemed pretty small nowadays. You could lose some confidentiality when encrypting large files with it in CBC mode because blocks may start to repeat (you might lose much more confidentiality with other modes of operation).

The idea of the password consisting of characters is that you can clear a char array, while you cannot do the same thing for a String. If you supply the password as a string then you lose this ability.

Do you know that there is a CipherInputStream and CipherOutputStream that can be put in front of a FileInputStream or FileOutputStream?


Coin Flip Streaks script I am attempting to complete the coin flip streaks problem from automate the boring stuff with python.
My code works fine but my only concern is the phrasing of the task.
Does the question want us to find the number of samples (100 flips) that contain at least one streak and divide that by the total number of samples (10,000)
Or are we supposed to find the total number of streaks (6H or 6T) in all samples and divide that by 10,000?
My solution was for the first option but I make no distinction between one streak and several.
My code and the question are listed below.
Coin Flip Streaks

For this exercise, we’ll try doing an experiment. If you flip a coin 100 times and write down an “H” for each heads and “T” for each tails, you’ll create a list that looks like “T T T T H H H H T T.” If you ask a human to make up 100 random coin flips, you’ll probably end up with alternating head-tail results like “H T H T H H T H T T,” which looks random (to humans), but isn’t mathematically random. A human will almost never write down a streak of six heads or six tails in a row, even though it is highly likely to happen in truly random coin flips. Humans are predictably bad at being random.
Write a program to find out how often a streak of six heads or a streak of six tails comes up in a randomly generated list of heads and tails. Your program breaks up the experiment into two parts: the first part generates a list of randomly selected 'heads' and 'tails' values, and the second part checks if there is a streak in it. Put all of this code in a loop that repeats the experiment 10,000 times so we can find out what percentage of the coin flips contains a streak of six heads or tails in a row. As a hint, the function call random.randint(0, 1) will return a 0 value 50% of the time and a 1 value the other 50% of the time.

import random
import re

totalRuns = 0
streakScore = 0

def coinFips():
    global streakScore,totalRuns
    # Return results into a single string for regular expressions
    results = ''
    for i in range(100):
      value = random.randint(0, 1)
      if value == 0:
        results += "H"
      elif value == 1:
        results += "T"
    # Use regular expressions to check results sting
    match = re.findall(r'(HHHHHH|TTTTTT)', results)
    if len(match) > 0:
      streakScore = streakScore +  1
    totalRuns = totalRuns + 1

for i in range(10000):
  coinFips()

print("The Chance of a streak is: {}%".format(round(streakScore/totalRuns * 100)))

The assignment

Write a program to find out how often a streak of six heads or a streak of six tails comes up in a randomly generated list of heads and tails.

Now this requirement you didn't match, so your program doesn't contain the required functionality, even if it does match the required result.
Note that there is a bit of a problem with the assignment, as you might see a streak of 7 as two streaks of 6, or a streak of 12 as two streaks of 6. I presume, given the assignment, that you may use any streak of 6 or more. This is not what your program currently does, by the way; I presume from the code that a streak of 12 will be seen as two streaks of 6.
[EDIT] Rereading the question, it seems that you do match the assignment, if you assume that the "often" part depends on how often the streak of 6 comes up in the randomly generated distributions, instead of in one distribution. That seems to be the case if I take the rest of the context in, but the assignment isn't stated very clearly.

our program breaks up the experiment into two parts: the first part generates a list of randomly selected 'heads' and 'tails' values, and the second part checks if there is a streak in it.

You fail to meet this requirement: you haven't specified two methods, which is generally the way you would split things up in programming.

Put all of this code in a loop that repeats the experiment 10,000 times so we can find out what percentage of the coin flips contains a streak of six heads or tails in a row.

So if you'd create the method that counts the streaks, and the streak count is higher than 0 then you would increase the counter. You do that perfectly :)
As indicated, you haven't separated it from the main functionality. Worse, you update global variables from a function.
If this course is about structured programming then you might fail even though your application is likely to generate the right answer.
Code review
totalRuns = 0
streakScore = 0

This is fine, but in that case you need to update them from global code. If not the variables should be local to a function.
def coinFips():

This method name is misspelled. Does the method really only perform coin flips? If so, shouldn't there be a parameter that indicates how many? And return an array of results?
global streakScore,totalRuns

Every time you type global, you should wonder if it really helps design / readability, and refrain from using globals if it does not. I would add a space before totalRuns, but that's kind of personal.
results = ''

Why not use a (character) array instead? That way you can assign it the right size from the start and then fill it by indexing the coin flip...
for i in range(100):

100 is a literal, a so called "magic value". Here you should use a parameter (or, in other cases, a constant).
# Use regular expressions to check results sting

Another spelling mistake. Spelling mistakes are not terrible, but if you make too many they act as red flags to a reviewer. They are more likely to take a deeper look.
match = re.findall(r'(HHHHHH|TTTTTT)', results)

It will probably not be terribly performant, but it is a nice way to test and easy to program. But note the problem indicated in the first section of the review.
Instead you probably want to use something like ([H]{6,}|[T]{6,}). Also, you'll find that regular expressions are hard to parameterize (what if you want to use 7 or any other number instead of 6?). Some kind of state machine would impress me (even) more.
print("The Chance of a streak is: {}%".format(round(streakScore/totalRuns * 100)))

Here too much is happening for me. The calculation should be on it's own line. If your function ends with ))) then your application is getting too much LISP like if you ask me. You can also see that it is getting too cramped because you suddenly don't use spacing around the operator / anymore.

The other lines seem fine to me. There isn't too much happening on them except for the last line - which is good. And the variable naming seems fine too. Spacing and readability are fine. The application code is concise too.
Conclusion: you are well on your way, but the part where you "divide-and-conquer" is lacking. This is not much of a problem in the above code of course, but it may become a problem for larger, more complex systems.


Count wins and losses for each team in a league Here is the problem.
Repeatedly ask the user to enter game scores in a format like team1 score1 - team2 score2. Store this information in a dictionary where the keys are the team names and the values are tuples of the form (wins, losses).
Here is my Solution, what can I change for the better in it?
number_of_games = int(input("Enter the total number of Games: "))
team_wins_and_loses = {}
for _ in range(number_of_games):
  team1, score1, __, team2, score2 = input("Enter the game results(Note that the name of the team must be a single word, without spaces):").split()
  team1_result, team2_result = ((1, 0), (0, 1)) if int(score1) > int(score2) else ((0, 1), (1, 0))
  if team1 not in team_wins_and_loses:
    team_wins_and_loses[team1] = team1_result
  else:
    updated_result = (team_wins_and_loses[team1][0] + team1_result[0], team_wins_and_loses[team1][1] + team1_result[1])
    team_wins_and_loses[team1] = updated_result

  if team2 not in team_wins_and_loses:
    team_wins_and_loses[team2] = team2_result
  else:
    updated_result = (team_wins_and_loses[team2][0] + team2_result[0], team_wins_and_loses[team2][1] + team2_result[1])
    team_wins_and_loses[team2] = updated_result
  print(team_wins_and_loses)

Please always use 4 spaces to indent when using Python.

Rather than the verbose name team_wins_and_loses we could use team_results.

Rather than adding both values of the tuple we should focus on adding 1 win to the winning team, and 1 lose to the losing team.
winner[0] += 1
loser[1] += 1

We can change your turnery to pick the wining or losing team, and then extract the value with the correct default.
winning_team, losing_team = (
    (team1, team2)
    if int(score1) > int(score2) else
    (team2, team1)
)
if winning_team not in team_results:
    winner = team_results[winning_team] = [0, 0]
else:
    winner = team_results[winning_team]
# same for losing team

We can use dict.setdefault to remove the need for the if.
winner = team_results.setdefault(winning_team, [0, 0])

We can use collections.defaultdict to make interacting with team_results much cleaner, rather than using dict.setdefault.
def default_scores():
    return [0, 0]

team_results = collections.defaultdict(default_scores)
# ...
team_results[winning_team][0] += 1

import collections

def default_scores():
    return [0, 0]

team_results = collections.defaultdict(default_scores)
number_of_games = int(input("Enter the total number of Games: "))
for _ in range(number_of_games):
    team1, score1, __, team2, score2 = input("Enter the game results(Note that the name of the team must be a single word, without spaces):").split()
    winner, loser = (
        (team1, team2)
        if int(score1) > int(score2) else
        (team2, team1)
    )
    team_results[winner][0] += 1
    team_results[loser][1] += 1
    print(team_results)


Converting XML to DOT I would like to show an XML tree as a tree using GraphViz Dot tool. Then I must convert it to a DOT file.

It is what I have tried

    string dot = "digraph G {" + Environment.NewLine;
    XML2DOT(XmlRoot, "r");
    dot += Environment.NewLine + "}";
    ....
    private void XML2DOT(XmlNode n, string id)
    {
        if (n == null)
            return;
        string NodeName = n.Name.Replace("-", "_");
        if (n.HasChildNodes)
        {
            dot += Environment.NewLine + NodeName
                + id + "[label=\"" + NodeName + "\"];";
        }
        else
        {
            dot += Environment.NewLine
                + "leaf" + id + "[label=\"" + n.InnerText + "\"];";
        }
        int i = 0;
        foreach (XmlNode item in n.ChildNodes)
        {
            string cid = id + i;
            dot += Environment.NewLine + NodeName + id + " -> "
            + (item.HasChildNodes ? item.Name.Replace("-", "_") : "leaf")
            + cid + ";";
            XML2DOT(item, cid);
            i++;
        }
   }

The input xml is:

<?xml version="1.0" encoding="UTF-8"?>
<S>
<MN clitic="empty" ne_sort="pers">
   <PUNC clitic="empty">
      <w clitic="empty" gc="Ox" lc="Ox" lemma="#">#</w>
   </PUNC>
   <MN clitic="empty" ne_sort="pers">
      <N clitic="ezafe">
         <w clitic="ezafe" gc="Apsz ; Nasp--- ; Nasp--z ; Ncsp--z" lc="Nasp--z" lemma="مسعود" n_type="prop" ne_sort="pers">مسعود</w>
      </N>
      <N clitic="ezafe">
         <w clitic="ezafe" gc="Apsy ; Nasp--- ; Nasp--z ; Ncsp--y" lc="Nasp--z" lemma="شجاعی" n_type="prop" ne_sort="pers">شجاعی</w>
      </N>
      <N clitic="empty">
         <w clitic="empty" gc="Nasp--- ; Nasp--z" lc="Nasp---" lemma="طباطبایی" n_type="prop" ne_sort="pers">طباطبایی</w>
      </N>
   </MN>
   <PUNC clitic="empty">
      <w clitic="empty" gc="Ox" lc="Ox" lemma="#">#</w>
   </PUNC>
</MN>
</S>

And it is what I get:

It works as expected, just would like to know a more general and efficient way to do that.

There's an XML syntax for DOT called DotML at http://martin-loetzsch.de/DOTML/. Generating a generic DotML tree from XML is very straightforward using XSLT. Most of it is just:

<xsl:template match="*">
  <node id="{generate-id()}" label="{name()}"/>
  <edge from="{generate-id(..)}" to="{generate-id()}"/>
  <xsl:apply-templates/>
</xsl:template>

<xsl:template match="text()">
  <node id="{generate-id()}" label="{.}"/>
  <edge from="{generate-id(..)}" to="{generate-id()}"/>
</xsl:template>


Search string for a substring - indexOf I am trying to write my own implementation of indexOf for learning purposes. I have an implementation of as follows. I'm sure a more efficient and shorter implementation exists.

function _indexOf(needle, haystack) {

    var n = needle.split("");
    var h = haystack.split("");

    var n_len = n.length;
    var h_len = h.length;

    for (var i = 0; i < h_len; i++) {

        var j = 0;
        var flag = false;

        while (h[i] == n[j] || flag) {

            if (flag)
                j = 0;

            flag = false;

            i++;
            j++;

            if (h[i] == n[0])
                flag = true;

            if (j >= n_len)
                return i - n_len;

        }

    }

    return -1;

}

Here's a test: assert(_indexOf("ced","ccceccecedeeced")==7) // true

Your code is pretty neat, and well structured. I like the variable names. There are a number of problems to point out, though...

splitting the string is a huge "cheat".... if you're going to use regex, you may as well just match for the substring.
splitting the strings is also slow. Just use an index in to the string and the string[indx] operator. You can remove the h and n and h_len and n_len variables as well then.
your outer loop can terminate at h_len - n_len - there's no need to loop to the end when a match would be impossible

You have some bugs.... like:

your needle cannot contain the same letter as the start letter in a another place... For example, your code will fail to find cec. This is because you do weird logic here: if (h[i] == n[0]) ... I am not sure why that's needed.... other than to reset the search?
related to the above, you cannot backtrack and reset a search in the event that the search pattern starts part way through a partial match....

Even when you resolve the above issues, your algorithm is considered 'naive'. There are two commonly used algorithms that are considered better. The Rabin–Karp algorithm is simpler to implement because you just create a hash of the needle, and then a matching rolling-hash in the haystack. If you find the hash in the haystack you can confirm a match, and return, in what is effectively \$O(n+m )\$. A more complicated, but also more efficient algorithm is the Boyer-Moore algorithm which creates a "jump table" from the needle, which allows you to 'skip' values in the haystack based on the details in the needle. It is efficient, and, the longer the needle, the more you can skip.


Getting country based on IP I'm using Struts2 and Google App Engine. Below is my program for getting the country based on the IP of the visitor of the website.

In my Struts2 action:

HttpServletRequest request = ServletActionContext.getRequest();
String IP = request.getRemoteAddr();
String countryCode = Utilities.fetchUrl( "http://api.hostip.info/country.php?ip=" + IP );
if ( countryCode != null){
    String country = CountryUtil.COUNTRY_MAP.get( countryCode );
    System.out.println("COUNTRY: " + country);
}

My fetchUrl static function:

public static String fetchUrl(String strUrl){
    String output = "";
    String line = null;
    try {

        URL url = new URL( strUrl );
        BufferedReader reader = new BufferedReader(new InputStreamReader(url.openStream()));
        while ((line = reader.readLine()) != null) {
            output += line;
        }
        reader.close();

    } catch (MalformedURLException e) {
        System.out.println("ERROR CATCHED: " + e.getMessage());
        return null;
    } catch (IOException e) {
        System.out.println("ERROR CATCHED: " + e.getMessage());
        return null;
    }

    return output;
}

My COUNTRY_MAP:

public static final Map<String, String> COUNTRY_MAP;
static {
    Map<String, String> map = new HashMap<String, String>();
    map.put("AF", "afghanistan");
    map.put("AL", "albania");
    ...
    map.put("ZW", "zimbabwe");
    COUNTRY_MAP = Collections.unmodifiableMap( map );
}

Any suggestion on improving its speed?  I also need suggestions for the coding.

The slowest part will be the request to the remote server. There is not much you can do to make this faster. You may want to cache the results you get back from hostip, or you may want to have a local database.

Furthermore, the API seems to return XX if it does not know the country, you may want to check for that.

Also, you do not close the InputStream or the InputStreamReader, and it is possible that you do not close the BufferedReader when an exception occurs. I don't know exactly whether this will cause any problems.


Getting the size on disc I wrote a C# tool that make a recursive search. I need the file size and the size on disc. I am using a function that give me the size on disc but it is very slow.

Is there any other way to do it except the following code?

public void RecursiveSearch(string path)
{
    try
    {
        foreach (string dir in Directory.GetDirectories(path))
        {
            foreach (string file in Directory.GetFiles(dir))
            {
                FileInfo fi = new FileInfo(file);

                Application.DoEvents();
                lblPfad.Text = fi.FullName;

                long FileLength = fi.Length;
                long FileLengthOnHarddisc = GetFileSizeOnDisk(file,fi);

                if ((FileLength == FileLengthOnHarddisc && FileLength == 4096) || FileLength >= FileLengthOnHarddisc)
                {
                    myStreamWriter.WriteLine(file + " - Größe - " + FileLength.ToString() + " - Größe auf Datenträger - " + FileLengthOnHarddisc.ToString());
                }

            }

            RecursiveSearch(dir);

        }
    }
    catch (Exception ex)
    {
        MessageBox.Show(ex.Message);
    }
}

Here is the function for getting the size on disc:

  public static long GetFileSizeOnDisk(string file,FileInfo info)
        {
            uint dummy, sectorsPerCluster, bytesPerSector;

            int result = GetDiskFreeSpaceW(info.Directory.Root.FullName, out sectorsPerCluster, out bytesPerSector, out dummy, out dummy);
            if (result == 0) throw new Win32Exception();
            uint clusterSize = sectorsPerCluster * bytesPerSector;

            uint hosize;
            uint losize = GetCompressedFileSizeW(file, out hosize);
            long size;
            size = (long)hosize << 32 | losize;
            return ((size + clusterSize - 1) / clusterSize) * clusterSize;
        }

        [DllImport("kernel32.dll")]
        static extern uint GetCompressedFileSizeW([In, MarshalAs(UnmanagedType.LPWStr)] string lpFileName,
           [Out, MarshalAs(UnmanagedType.U4)] out uint lpFileSizeHigh);

        [DllImport("kernel32.dll", SetLastError = true, PreserveSig = true)]
        static extern int GetDiskFreeSpaceW([In, MarshalAs(UnmanagedType.LPWStr)] string lpRootPathName,
           out uint lpSectorsPerCluster, out uint lpBytesPerSector, out uint lpNumberOfFreeClusters,
           out uint lpTotalNumberOfClusters);

One aside to mention: you should split your UI and business logic code.  For the sake of tossing it into a working example quickly (I'm doing this over lunch), I removed all the UI code and replaced any print-outs with Console.WriteLine calls.

I also did a little refactoring to split method calls.

Additionally, I renamed RecursiveSearch to RecursiveSearch2 so I could run it side-by-side with the original.

Finally, I made some of the non-static method static, but only to speed up running the project.  Pay no mind to these changes.

  public static void RecursiveSearch2 (string path)
  {
     try
     {
        RecursiveSearch2 (new DirectoryInfo (path));
     }
     catch (Exception ex)
     {
        Console.WriteLine (ex.Message);
     }
  }

Since I switched to use DirectoryInfo, I went ahead and made two overloads of the RecursiveSearch method.  One takes in the string for the initial search, but sub-directories can call the overload which takes in DirectoryInfo directly.  The first allows you to make the change without breaking your interface, while the second is more useful during processing work.

For speed, I pushed the try/catch up here and left out exception handling elsewhere, though you may want to add some at the lower level methods, if only to provide more descriptive logging/exception messages.

  public static void RecursiveSearch2 (DirectoryInfo dir)
  {
     foreach (var item in dir.GetFileSystemInfos ())
     {
        if (item is FileInfo)
           PrintFileInfo (item as FileInfo);
        else if (item is DirectoryInfo)
           RecursiveSearch2 (item as DirectoryInfo);
     }
  }

If you call the DirectoryInfo.GetFileSystemInfos method, it returns all directories and files in one call.  The upside is that it reduces the calls to the filesystem, which are generally going to be your bottleneck.  The downside is that you get a base type back, so some type handling may be necessary.

  private static void PrintFileInfo (FileInfo fi)
  {
     long FileLength = fi.Length;
     long FileLengthOnHarddisc = GetFileSizeOnDisk (fi.FullName, fi);

     if ((FileLength == FileLengthOnHarddisc && FileLength == 4096) || FileLength >= FileLengthOnHarddisc)
     {
        Console.WriteLine (fi.FullName + " - Größe - " + FileLength.ToString () + " - Größe auf Datenträger - " + FileLengthOnHarddisc.ToString ());
     }
  }

We are then left with handling the actual file, which I split off into a different method.  The only notable differences is that I changed the input to be a FileInfo type rather than string, since I already have the FileInfo object from the calling method's loop.

Running your method versus the modified version on my c:\downloads folder cut the time consistently between 1/4 to 1/2 the time of the original, though my sample size is far too small to be conclusive.  I would advise benchmarking it yourself, though.  It is possible that the performance characteristics change wildly based upon the folder structure, depth, and ratio of directories to files.


Python class w/ Telnet interface to memcached I haven't had anyone help me out with code review, etc, so I thought I'd post a Python class I put together for interfacing with Telnet to get information from a memcached server.

import re, telnetlib

class MemcachedStats:

    _client = None

    def __init__(self, host='localhost', port='11211'):
        self._host = host
        self._port = port

    @property
    def client(self):
        if self._client is None:
            self._client = telnetlib.Telnet(self._host, self._port)
        return self._client

    def key_details(self, sort=True):
        ' Return a list of tuples containing keys and details '
        keys = []
        slab_ids = self.slab_ids()
        for id in slab_ids:
            self.client.write("stats cachedump %s 100\n" % id)
            response = self.client.read_until('END')
            keys.extend(re.findall('ITEM (.*) \[(.*); (.*)\]', response))
        if sort:
            return sorted(keys)
        return keys

    def keys(self, sort=True):
        ' Return a list of keys in use '
        return [key[0] for key in self.key_details(sort=sort)]

    def slab_ids(self):
        ' Return a list of slab ids in use '
        self.client.write("stats items\n")
        response = self.client.read_until('END')
        return re.findall('STAT items:(.*):number', response)

    def stats(self):
        ' Return a dict containing memcached stats '
        self.client.write("stats\n")
        response = self.client.read_until('END')
        return dict(re.findall("STAT (.*) (.*)\r", response))

This is also up on GitHub.

I would love some feedback on:

Organization
Better ways of accomplishing the same result

The pattern

self.client.write("some command\n")
response = self.client.read_until('END')

appears three times in your code. I think this is often enough to warrant refactoring it into its own method like this:

def command(self, cmd):
    self.client.write("%s\n" % cmd)
    return self.client.read_until('END')

In key_details you're using extend to build up a list. However it's more pythonic to use list comprehensions than building up a list imperatively. Thus I'd recommend using the following list comprehension:

regex = 'ITEM (.*) \[(.*); (.*)\]'
cmd = "stats cachedump %s 100"
keys = [key for id in slab_ids for key in re.findall(regex, command(cmd % id))]

Afterwards you do this:

if sort:
    return sorted(keys)
return keys

Now this might be a matter of opinion, but I'd rather write this using an else:

if sort:
    return sorted(keys)
else:
    return keys

I think this is optically more pleasing as both returns are indented at the same level. It also makes it immediately obviously that the second return is what happens if the if condition is false.


Recursive reverse function Write a recursive version of the function reverse(s), which reverses the string s in place.

Here is my solution:

void reverse(char a[], int i, int j) {
    char tmp;

    if(i >= j) {
        return;
    }

    reverse(a, i + 1, j - 1);
    tmp = a[i];
    a[i] = a[j];
    a[j] = tmp;
}

This exercise is right after the presentation of the quicksort algorithm.

This is the signature of the quicksort function presented in the book:

void qsort(int v[], int left, int right)

This is why I added 2 new parameters to the functions - to narrow the size of the array.

There are 2 base cases:

When i == j, so n is even
When i > j, so n is odd

n represents the number of characters in string s.

It looks ok to me.

As a few comments in a random order:

I guess I don't have to tell you that writing a non-recursive function for this is quite straightforward.
You could give tmp its final value as you define it : char tmp = a[i];.
You could make it a tail call recursion by doing reverse(a, i+1, j-1) at the end.
It might be a good option to define a function reverse taking only the array and the length as an argument and calling the one you've just posted here. If you were to use this function in a C++ program, you could do this with a single function definition if you use default parameter (i is 0 by default). The main drawback is that you have to give the parameter a somewhat awkward order.


Jest unit test that handles a wrapped Promise I've wrapped an imported function (a promise) and attempted to write unit tests that bring my codebase to 100% coverage.

import { Auth } from 'aws-amplify';

import signIn from './shared';

describe('signIn', () => {
  afterEach(() => {
    jest.restoreAllMocks();
  });

  it('invokes the success callback on success', (done) => {
    const mockUser = { username: 'jon', password: 'snow' };

    jest
      .spyOn(Auth, 'signIn')
      .mockImplementation(() => Promise.resolve(mockUser));

    const mockSuccessCb = (user) => {
      expect(user).toBe(mockUser);
      done();
    };

    const mockErrorCb = (err) => {
      done(err);
    };

    signIn('jon', 'snow', mockSuccessCb, mockErrorCb);
  });

  it('invokes the error callback on err', (done) => {
    const mockUser = { username: 'jon', password: 'snow' };

    jest
      .spyOn(Auth, 'signIn')
      .mockImplementation(() => Promise.reject(mockUser));

    const mockSuccessCb = (user) => {
      expect(Auth.signIn).toHaveBeenCalledWith('jon', 'snow');
      expect(user).toBe(mockUser);
      done();
    };

    const mockErrorCb = () => {
      expect(Auth.signIn).toHaveBeenCalledWith('bob', 'tom');
      done();
    };

    signIn('bob', 'tom', mockSuccessCb, mockErrorCb);
  });
});

Auth is the imported object that is getting wrapped. In my first test, I attempt verify a successful login by returning a resolved promise. In the second test, I reject the promise. I got my 100% coverage, but since I tell my promise how to resolve itself, I can't help but feel like I'm setting myself up for false positives. Is this a good test? How can it be improved?

Edit: Added the original wrapper function for detail:

import { Auth } from 'aws-amplify';

const signIn = (
  username,
  password,
  successCallback,
  errorCallback,
) => {
  Auth.signIn(username, password)
    .then(user => successCallback(user))
    .catch(err => errorCallback(err));
};

export default signIn;

I think the test is ok. Remember what you're testing is this

const signIn = (
  username,
  password,
  successCallback,
  errorCallback,
) => {
  Auth.signIn(username, password)
    .then(user => successCallback(user))
    .catch(err => errorCallback(err));
};

and NOT Auth. Here Auth is just a dependency so you will stub its functionality. Considering that its interface is Promise-based, you need to test both cases - therefore having a test that verifies your expected (promise solved) and the one with the error is correct.

There are some changes I would add, though. For instance, rather than creating my own successCallback and errorCallback I would use mock functions, that would allow me to have further information

const successCallback = jest.fn();
const rejectCallback = jest.fn();

and I can check there if they were called or not and verify their arguments.

expect(successCallback).toHaveBeenCalledWith(mockUser);
expect(rejectCallback).not.toHaveBeenCalled();

and the other way around for the other test.

Then, I would take advantage of async/await for the tests, so we don't need to ensure that ugly done function is called. Basically, we declare the arrow function of the test as async, we await the promise calls and just by having the arrow function to be finished ensures our tests have passed.

An example of these changes would be something like this

it('invokes the success callback on success', async () => {
    jest
      .spyOn(Auth, 'signIn')
      .mockImplementation(() => Promise.resolve(mockUser));
    const successCallback = jest.fn();
    const rejectCallback = jest.fn();
    await signIn('jon', 'snow', successCallback, rejectCallback)
    expect(Auth.signIn).toHaveBeenCalledWith('jon', 'snow');
    expect(successCallback).toHaveBeenCalledWith(mockUser);
    expect(rejectCallback).not.toHaveBeenCalled();
  });

Another step would be that we have strings all over our tests, such as "jon" and "snow". If we already have a mocked user we could reference them from there by using mockUser.username, mockUser.password

That leaves us with the tests looking like this

it('invokes the success callback on success', async () => {
    jest
      .spyOn(Auth, 'signIn')
      .mockImplementation(() => Promise.resolve(mockUser));
    const successCallback = jest.fn();
    const rejectCallback = jest.fn();
    await signIn(mockUser.username, mockUser.password, successCallback, rejectCallback)
    expect(Auth.signIn).toHaveBeenCalledWith(mockUser.username, mockUser.password);
    expect(successCallback).toHaveBeenCalledWith(mockUser);
    expect(rejectCallback).not.toHaveBeenCalled();
  });

  it('invokes the error callback on err', async () => {
    jest
      .spyOn(Auth, 'signIn')
      .mockImplementation(() => Promise.reject(mockUser));

    const successCallback = jest.fn();
    const rejectCallback = jest.fn();
    await signIn(mockUser.username, mockUser.password, successCallback, rejectCallback);
    expect(Auth.signIn).toHaveBeenCalledWith(mockUser.username, mockUser.password);
    expect(rejectCallback).toHaveBeenCalledWith(mockUser);
    expect(successCallback).not.toHaveBeenCalled();
  });

As you can see they are pretty similar, so we could somewhat extract that functionality to a method with the common assertions.

We could create a function like this

const assertSignIn = async (mockedResponse, shouldBeCalledCallback, shouldNotBeCalledCallback) => {
        jest
            .spyOn(Auth, 'signIn')
            .mockImplementation(() => mockedResponse);
        const successCallback = jest.fn();
        const rejectCallback = jest.fn();
        await signIn(mockUser.username, mockUser.password, successCallback, rejectCallback)
        expect(Auth.signIn).toHaveBeenCalledWith(mockUser.username, mockUser.password);
        expect(shouldBeCalledCallback).toHaveBeenCalledWith(mockUser);
        expect(shouldNotBeCalledCallback).not.toHaveBeenCalled();
  };

and then call it properly from each test

it('invokes the success callback on success', async () => {
    const successCallback = jest.fn();
    const rejectCallback = jest.fn();
    await assertSignIn(Promise.resolve(mockUser), successCallback, rejectCallback);
  });

  it('invokes the error callback on err', async () => {
    const successCallback = jest.fn();
    const rejectCallback = jest.fn();
    await assertSignIn(Promise.reject(mockUser), rejectCallback, successCallback);
  });

Although both last parameters are mock function, the difference is conceptual - by declaring them outside you can ensure by reading the tests in which the successCallback should be executed and in which should not.


Store first value of http request with rxjs but keep doing httprequest With RXJS library :
I need to store values from the FIRST http request, but keep doing other http request on demand.

I have done this code so far, and that is working. But I think there is a more elegant way.

const Rx = require("rxjs");
let numHttp = 0;
const httpRequest = () => {
    let a = [];
    numHttp = numHttp + 1;
    for (let i = 0; i < 10; ++i) {
        a.push({ num: i, http: numHttp });
    };
    return new Rx.BehaviorSubject(a)
        .do(a => {
            console.log("antoher http request");
            if (getC.getValue() === undefined) {
                getC.next(a)
            }
        })
        // .do(n => { if (getB.) getB.next(n) })
}
const getB = () => {
    const a = new Rx.BehaviorSubject();
    return a
};
getC = getB();
httpRequest().subscribe(d => d);
httpRequest().subscribe(d => d);
getC.subscribe(data => console.log('cache', data))
httpRequest().subscribe(d => d);
httpRequest().subscribe(d => d);

It is really hard to solve the problem since the question is not providing all the necessary details.

Assuming you have a factory for your Http observable (call it createHttpObservable()) you may do something like this:

const observable = createHttpObservable()

  // Share the stream of events across all the subscribers.
  .publishReplay(1)
  .refCount()

  // Use `.scan()` against event stream (similarly to how we `.reduce()` against an array).
  .scan(
    (history, currentValue, valueIndex) => {
      history.push({ num: valueIndex, httpNum: currentValue });
      return history;
    },
    []
  )

  // Take the last event from the `history`
  .map(history => history[history.length - 1].httpNum)

  // Arbitrarily use `.do` for side effects.
  .do(console.log);

// Subscribe wherever you need.
observable.subscribe();
observable.subscribe();
observable.subscribe();

Here's a related, but not quite the same exact plunkr.

You can have more custom logic in the .scan() block, but frankly, I have no idea what are you trying to achieve and why is the first Http request's results are of any special interest.


Fast evaluation of Euclidean distance with sparse data I am working on a KNN implementation for sparse datasets (that I apply to text analysis). My points are represented by a Dictionary<string, double>, each key represents a word of the text and the value, its count or TFIDF.

I am using index inversion to reduce the number of distances to evaluate. However, evaluating the distance remains the bottleneck. It is represented in the following method:

    public static double Euclide(Dictionary<string, double> sp1, Dictionary<string, double> sp2)
    {
        double distance = 0;
        foreach (KeyValuePair<string, double> kvp1 in sp1)
        {
            if (sp2.ContainsKey(kvp1.Key))
                distance += Math.Pow((kvp1.Value - sp2[kvp1.Key]), 2);
            else
                distance += Math.Pow((kvp1.Value), 2);
        }

        foreach (KeyValuePair<string, double> kvp2 in sp2)
            if (!sp1.ContainsKey(kvp2.Key))
                distance += Math.Pow((kvp2.Value), 2);

        return distance;
    }

How can I make it faster? Any help would be greatly appreciated.

I think I could reduce the time of evaluation using a Dictionary<int, double>, but I prefer to stick to strings, as it allows me to see at a glance what is going on (hashing words would compromise that).

The following yieled a 1.3~2.1x improvement (depending on the length of the input, but it is still not enough) :

    public static double FastEuclide(Dictionary<string, double> sp1, Dictionary<string, double> sp2)
    {
        double distance = 0;
        foreach (KeyValuePair<string, double> kvp1 in sp1)
        {
            double sp1Value = kvp1.Value;

            if (sp2.ContainsKey(kvp1.Key))
            {
                double sp2Value = sp2[kvp1.Key],
                    diff = kvp1.Value - sp2[kvp1.Key];
                distance += diff * diff;
            }
            else
                distance += sp1Value * sp1Value;
        }

Calling ContainsKey() on a dictionary if you need to get the value later on should be replaced with TryGetValue() to reduce the processing time.
See: what-is-more-efficient-dictionary-trygetvalue-or-containskeyitem

TryGetValue will be faster.
ContainsKey uses the same check as TryGetValue, which internally refers to the actual entry location. The Item property actually has nearly identical code functionality as TryGetValue, except that it will throw an exception instead of returning false.
Using ContainsKey followed by the item basically duplicates the lookup functionality, which is the bulk of the computation in this case

The next part to increase performance is to replace the call to Math.Pow() with multiplying the value by itself.
So instead of Math.Pow((kvp2.Value), 2) you should use kvp2.Value * kvp2.Value.
You just edited your question to do this but I will keep it in my review.

Using the said above will lead to
public static double Euclide(Dictionary<string, double> sp1, Dictionary<string, double> sp2)
{
    double distance = 0;

    foreach (KeyValuePair<string, double> kvp1 in sp1)
    {
        double possibleValue = 0.0d;
        sp2.TryGetValue(kvp1.Key, out possibleValue);

        double currentValue = kvp1.Value - possibleValue;

        distance += currentValue * currentValue;
    }

    foreach (KeyValuePair<string, double> kvp2 in sp2)
    {
        if (!sp1.ContainsKey(kvp2.Key))
        {
            distance += kvp2.Value * kvp2.Value;
        }
    }
    return distance;
}

This

double possibleValue = 0.0d;
sp2.TryGetValue(kvp1.Key, out possibleValue);

double currentValue = kvp1.Value - possibleValue;

will store the value of the searched key inside possibleValue if the key is in the dictionary. If TryGetValue() does not succeed it will return default(T) which for a double is 0.0.

I have added braces {} for the for and the if which makes the code less error prone. I would like to encourage you to always use braces.


Convert Array CSV to Object using Header Takes the first array in the array and uses that to set the properties of an array-object.

var _ = require("underscore");
function csvToObject(data){
  var header = data.shift();
  return _.map(data, function(row){
    var temp = {};
    _.each(row, function(cell, key){
      temp[header[key]] = cell;
    });
    return temp;
  });
}

I created a non-underscore dep version

function csvToObject2(data) {
  var header = data.shift();
  var alpha = [];
  for (var i = 0; i < data.length; i++) {
    var beta = {};
    for (var j = 0; j < data[i].length; j++) {
      beta[header[j]] = data[i][j];
    }
    alpha.push(beta);
  }
  return alpha;
};

They both performed the same in jsPerf. Thoughts?

You can also use built-in methods such as map and reduce. According to JSPerf they are as fast and the code is shorter, so you could potentially get rid of the Underscore dependency:

function csvToObject2(data) {
  return data.slice(1).map(function(xs) {
    return xs.reduce(function(acc, x, i) {
      acc[data[0][i]] = x
      return acc
    },{})
  })
}


Parse sequence of characters detecting formatting categories I am a beginner coder and SonarLint tells me code is too complex. I tried recursion, iterators, slicing but had not luck making this code any simpler.
The code will parse the input string to detect the following categories:

bold
italic
normal

depending on whether the characters are enclosed in 0-2 asterisks.
Sorry for the short description luckily, Iterniam inferred correctly. And side note nested **Bold *and* italic** are not supported by the code. I will read more on Iterniam's answer thank you for that!
Maybe a philosophical question at last, even though cyclomatic complexity in my code is higher the regex answer, the proposed solution is still much harder to understand (if it were not for the comments), debug and has worse performance. So is it the better code or should I just ignore cyclomatic complexity hints in the future?
txt = "A **A** is *italic* **bold** but some are *a* *b* **B**"

def fun(text=None):
    if text:
        bold_splits = text.split('**')
        for i, s in enumerate(bold_splits):
            if i % 2:
                print('BOLD:', s )
            else:
               italic_splits = s.split('*')
               for m, n in enumerate(italic_splits):
                   if m % 2:
                       print('ITALIC:', n )
                   else:
                       if n != '':
                           print('NORMAL:', n )

It looks like you're trying to extract information from a string. This is most often done through regexes as they are often faster than your own code while being less complex.
Because as of writing, you have no clear description of your goal, I'm inferring these assumptions from your code:

You want to split every sequence of characters in your string in one of the categories bold, italic and normal, determined by the characters being enclosed in 2, 1 or 0 asterisks.
You want to preserve the order of the original string.

If we disallow asterisks in the normal text, a regex for this is rather simple. I recommend exploring what it does on regex101.
\*\*(?P<bold>.+?)\*\*|\*(?P<italic>.+?)\*|(?P<normal>[^*]+)

Notably, the | character matches on the first match it finds, from left to right, meaning that a match will always try to match bold, then italic, then normal.
In Python, we can iterate over all the matches using re.finditer. For every match, lastgroup will return the name of the captured group, so we can use that in our print statement and also use it to get the actual match that we're interested in.
def fun(text=None):
    if text:
        for match in re.finditer(r'\*\*(?P<bold>.+?)\*\*|'  # match bold lazily
                                 r'\*(?P<italic>.+?)\*|'  # match italics lazily
                                 r'(?P<normal>[^*]+)',  # match normal greedily
                                 text):
            print(f'{match.lastgroup.upper()}: {match.group(match.lastgroup)}')

It should be noted that if the function is called plenty of times and performance is of concern, you should compile the regex once before using it in fun.
The regex used in this example is very primitive because you haven't specified your constraints or even what the function is meant to do. By specifying your question better and explaining how edge cases like ** a ** *** and **ab*cd** should behave, a more complete answer can be given.
Everything below this is in response to your 'philosophical question':

The regex answer [...] has worse performance.

I did some testing using timeit on the original string (multiplied in size anywhere between 0-3000 times) on differing number of runs and found that the regex approach is consistently 1.5 times faster than the original solution as long as the print statements are included.
Because printing it is unlikely to be your actual use-case, I modified both functions to return a tuple of all bold, italic and normal matches. I was surprised to find that your code is generally twice as fast.

The proposed solution is still much harder to understand (if it were not for the comments).

The comments are exactly what make the regex a relatively readable solution compared to yours. By commenting what the individual parts do, someone that needs to use the code can quickly see what the code achieves. The cost is that it may require more time to figure out how the code works.
Note that this extra cost also diminishes as you get more experience. For some, figuring out how your code achieves its goal is more difficult than understanding the regex. Regexes are very powerful, quite popular, and are used in many languages. This means that when one encounters a regex construct they know what to look for and how to read it. Your solution, meanwhile, is based on a few implicit assumptions like "every other string from the split is the thing we want to match". Someone that reads your code has to discover these assumptions because they're not explicitly written down.

The proposed solution is still much harder to [...] debug.

If nothing else, it's easy to pinpoint where the issue if there is one: in the regex. Given the input, it shouldn't be hard to figure out why the regex is failing if you inspect it using a tool like regex101. It may be difficult to correct the regex, but at least you won't have a scavenger hunt for where your bug is.

Should I just ignore cyclomatic complexity hints in the future?

Avoiding a high cyclomatic complexity mostly comes down to avoiding nesting a little more. This ensures that you have a more linear (and therefore more comprehensible) flow to your code.
The algorithm you used requires quite a bit of nesting which is not readily avoidable. You can, however, make some improvements by using the if as a guard. Using list comprehension, you can also eliminate the final for-if-else loop.
def fun_unnested_order(text=None):
    if not text:
        return None

    res = []
    bold_splits = text.split('**')  # Odd entries are bold, even entries are italicized or normal.
    for i, s in enumerate(bold_splits):
        if i % 2 == 1:  # Store bold
            res.append((s, 'bold'))
        else:
            italic_splits = s.split('*')  # Odd entries are italicized, rest normal.
            # Replace the for-if-else loop with list comprehension
            res += [(e, 'italic') if i % 2 == 1 else (e, 'normal')
                    for i, e in enumerate(italic_splits)]
    return res

This example is 30% slower than your original code using the same timeit setup.

So is it the better code?

It depends on your viewpoint. If performance is of concern, you should go for greybeard's answer as it is around 30% faster than your original code.
If readability is of concern, I would take the regex version any day of the week. While many may consider regexes black magic, it is widely applicable to many problems in most if not all popular languages. Most crucially perhaps, the regex solution doesn't require a complex algorithm, which means it's easier to develop and less error-prone.
Again, in terms of readability, it's about the familiarity of the programmer. If you are unfamiliar with regexes, list comprehensions and/or list slicing, your original version is the option to go for.
In the end, there is no one implementation that wins across all categories. You should familiarize yourself with the different ways to approach the problem and then tackle it in a way you deem the best solution.


Optimize K-Mean for large number of clusters I am writing a Python code for KMeans clustering.
The aim of this post is to find out how I can make my below mentioned code optimal when the number of clusters is very large. I am dealing with data with tens of millions of samples and scores of features. The number of clusters that I am talking about is of the order of 500000. I have checked the correctness of the code by comparing that with results from scikit-learn's implementation of KMeans clustering.
I am aware that I can use sckit learn for KMeans clustering. But, I am writing my own custom Python code for a few purposes which include but are not limited to the below reasons.

To be able to change distance functions for my specific work assignments : from Euclidean distance to Huang distance.
To use this custom code for various other tasks like anomaly detection.

Below is my code:
class KMeans:
    def __init__(self, arr, k):
        self.arr = arr
        self.k = k

    # Main driver function
    def loop_over_centers(self, max_iter):

        # Choose random centroids.
        random_centers = np.random.choice(len(self.arr), self.k, replace=False)

        # Iterate till a certain max number of iterations.
        for iteration in range(0, max_iter, 1):
            cluster_dict = defaultdict(list)

            if iteration == 0:
                chosen_centers = random_centers

            # Assign samples to cluster centers based on minimum distance
            for i in range(len(self.arr)):
                distance_list = []
                for center in chosen_centers:
                    distance_list += [self.distance(self.arr[center], self.arr[i])]

                cluster_dict[chosen_centers[np.argmin(distance_list)]] += [i]

            chosen_centers = self.update_center(cluster_dict)

        return cluster_dict

    # Distance between two arrays
    def distance(self, arr1, arr2):
        return np.sqrt(np.sum(np.subtract(arr1, arr2) ** 2))

    # Gives new centroids with each new iteration
    def update_center(self, cluster_dict):
        cluster_center = []

        for cluster in list(cluster_dict.keys()):
            cluster_list = []

            # Find samples corresponding to each cluster.
            for sample in cluster_dict[cluster]:
                cluster_list += [self.arr[sample]]

            # Mean of samples in a given cluster.
            cluster_mean = np.mean(np.array(cluster_list), axis=0).tolist()

            distance_from_mean = []

            # Find which sample is closest to mean of cluster.
            for sample in cluster_dict[cluster]:
                distance_from_mean += [self.distance(cluster_mean, self.arr[sample])]

            # Build an updated list of cluster centers.
            cluster_center += [cluster_dict[cluster][np.argmin(distance_from_mean)]]

        return cluster_center

arr = [[12, 3, 5], [1, 6, 7], [8, 92, 1], [6, 98, 2], [5, 90, 3], [29, 5, 6], [11,4,4], [30, 6, 5],]

testKMeans = KMeans(arr, 3)
testKMeans.loop_over_centers(1000)

In a real world case, arr will have tens of millions of samples and I would like to have about 500000 cluster centroids. At present the performance of code is okay when arr has tens of millions of samples and number of cluster centroids is small (10-20). But is very slow when the number of clusters is very large (hundreds of thousands).
How can I improve the runtime performance of this code for large number of cluster centers?.

First, some Python improvements. You do repeated lookups of the kind cluster_dict[cluster]. If you were to directly iterate over cluster_dict.values() these would all be not needed. Also, list comprehensions are usually faster than building a simple list with a for loop. And finally, docstrings go inside the function:
def update_center(self, cluster_dict):
    """Gives new centroids with each new iteration."""
    cluster_center = []

    for cluster in cluster_dict.values():
        cluster_list = [self.arr[sample] for sample in cluster]

        # Mean of samples in a given cluster.
        cluster_mean = np.mean(np.array(cluster_list), axis=0).tolist()

        # Find which sample is closest to mean of cluster.
        smallest_distance_from_mean = np.argmin([self.distance(cluster_mean, sample)
                                                 for sample in cluster_list])

        # Build an updated list of cluster centers.
        cluster_center.append(cluster[smallest_distance_from_mean])

    return cluster_center

Another small improvement is not to calculate the sqrt in your distance function, since you only ever need the minimum value, so the squared values work just fine.
def distance(self, arr1, arr2):
    """Squared distance between two arrays"""
    return np.sum(np.subtract(arr1, arr2) ** 2)

However, the greatest improvements are probably to be gained by not using lists everywhere, but to use numpy wherever you can:
class KMeans:
    def __init__(self, arr, k):
        self.arr = np.array(arr)
        self.k = k

    def loop_over_centers(self, max_iter):

        # Choose random centroids.
        chosen_centers = np.random.choice(len(self.arr), self.k, replace=False)

        # Iterate till a certain max number of iterations.
        for _ in range(max_iter):

            cluster_dict = defaultdict(list)

            # Assign samples to cluster centers based on minimum distance
            centers = self.arr[chosen_centers]
            for i, sample in enumerate(self.arr):
                closest_center = np.argmin(np.sum((centers - sample)**2, axis=1))
                cluster_dict[chosen_centers[closest_center]].append(i)
            chosen_centers = self.update_center(cluster_dict)

        return cluster_dict

    def update_center(self, cluster_dict):
        """Gives new centroids with each new iteration."""
        cluster_center = []

        for cluster in cluster_dict.values():
            samples = self.arr[cluster]
            # Mean of samples in a given cluster.
            cluster_mean = np.mean(samples, axis=0)
            closest_sample = np.argmin(np.sum((samples - cluster_mean)**2, axis=1))
            cluster_center.append(cluster[closest_sample])

        return cluster_center

Your code takes 391 ms ± 21.7 ms on my machine, the first two changes drop this down to 340 ms ± 15.9 ms and the final change to 145 µs ± 588 ns, faster by a factor 2700!
You could go even further by doing this part without the for loop using only array operations as well:
        for i, sample in enumerate(self.arr):
            closest_center = np.argmin(np.sum((centers - sample)**2, axis=1))
            cluster_dict[chosen_centers[closest_center]].append(i)

However, this will then take k * len(arr) * dim(arr) memory, which may be a bit too much if both are very large.
Note: You can factor out the distance into a function again, I just don't want to rerun the timings...


Lattice lotka-volterra Can anyone comment on the efficiency of my code? I am new with Python using NumPy/SciPy and I am trying to program a lattice biological model where a certain (one and only one) species (represented by a nonzero element) occupies a lattice square (represented in the code by a 2D array). This species can reproduce to an adjacent and empty lattice square (represented by a zero element). (Quite similar to a lattice gas model.)

The part of the code that takes the longest time is the reproduction part (def repro) because it has to check whether a site is already chosen every time a random site is generated to avoid repetition (the first while loop in def repro). Can anyone suggest a faster way to do this?

I am thinking of creating a collection of indices (2D index) where I can choose a random element and delete it at the same time so that the while loop won't be necessary. How can I do this?

import numpy as np
import matplotlib.pyplot as plt
import random
import time
import csv

open('p14.4.csv','w').close()

t0 = time.time()

def dispose(matrix,dens,s,sp):
    limit = int(n*n*dens)
    for num in range(limit):
        i = np.random.randint(n)
        j = np.random.randint(n)
        while matrix[i,j] != 0:
            i = np.random.randint(n)
            j = np.random.randint(n)
        matrix[i,j] = sp
    return matrix

def mort(matrix,d):
    RN = np.random.random((n,n))
    casualty = (RN < d)
    matrix[...][casualty] = 0
    return matrix

def reprod(matrix,b):
    p = np.random.randint(-1,2)
    q = np.random.randint(-1,2)
    idx = np.random.randint(-1,0,(n,n))
    jdx = np.random.randint(-1,0,(n,n))
    for k in range(n*n):
        i = np.random.randint(n)
        j = np.random.randint(n)
        while (idx[i,j] == i and jdx[i,j] == j):
            i = np.random.randint(n)
            j = np.random.randint(n)
        idx[i,j], jdx[i,j] = i, j
        if matrix[i,j] != 0:
            spp = int(matrix[i,j])
            while ((i+p<0 or i+p>n-1) or (j+q<0 or j+q>n-1)):
                p = np.random.randint(-1,2)
                q = np.random.randint(-1,2)
            if matrix[i+p,j+q] == 0 and b[spp-1] > np.random.random():
                matrix[i+p,j+q] = matrix[i,j]
    return matrix

def csv_writer(data,path,line):
    with open(path, "a") as csv_file:
        writer = csv.writer(csv_file, delimiter=',')
        writer.writerow(data)
        csv_file.close()
    return path

#birth rate
spp = 10
B = [0.57710226,0.57722738,0.577233935,0.577120507,0.576885731,0.576528293,
     0.576046935,0.575440457,0.574707715,0.573847625]
dr = 0.3

#initial density
initD = 0.005

#mortality rate
m = 0.1

#matrix size
n = 1000

#lattice system is initially empty
latsys = np.zeros((n,n))

#randomly place the species in the matrix
for i in range(1,spp+1):
    dispose(latsys,initD,spp,i)

#reproduction and mortality
for t in range(1):
    print t

    x=latsys

    #reproduction
    reprod(latsys,B)

    t3 = time.time()
    total3 = t3-t0
    print total3

    #mortality
    mort(latsys,dr)

    S1 = float(np.count_nonzero(x==1))/(n*n)
    S2 = float(np.count_nonzero(x==2))/(n*n)
    S3 = float(np.count_nonzero(x==3))/(n*n)
    S4 = float(np.count_nonzero(x==4))/(n*n)
    S5 = float(np.count_nonzero(x==5))/(n*n)
    S6 = float(np.count_nonzero(x==6))/(n*n)
    S7 = float(np.count_nonzero(x==7))/(n*n)
    S8 = float(np.count_nonzero(x==8))/(n*n)
    S9 = float(np.count_nonzero(x==9))/(n*n)
    S10 = float(np.count_nonzero(x==10))/(n*n)

    print S1,S2,S3,S4,S5,S6,S7,S8,S9,S10

    R = [t,S1,S2,S3,S4,S5,S6,S7,S8,S9,S10]

    path = "p14.4.csv"
    csv_writer(R,path,t)

SPCS = [S1,S2,S3,S4,S5,S6,S7,S8,S9,S10]
SURVIVORS = np.count_nonzero(SPCS)

print "The number of surviving species is ", SURVIVORS

t4 = time.time()
total4 = t4-t0
print 'Total simulation run time =', total4, 'second(s).'

#for graphing snapshots
plt.imshow(latsys, cmap=plt.cm.spectral, interpolation='nearest')
plt.colorbar()
plt.show()

There's quite a lot of code here, so for now I'm just going to look at the dispose function. You'll see that there's more than enough to say about this function for one answer.

1. Review

There's no docstring. What does this function do? How do I call it? What values do I pass for parameters? What does it return? It's very hard to maintain code when you don't know what it is supposed to do. This doesn't have to be long-winded or laborious: a quick explanation like the following is perfectly adequate.

"""Choose n*n*dens positions in matrix with the value 0, uniformly at
random, and set them to sp. Return matrix."""

There's no good reason to abbreviate the parameter to dens. It only appears once in the function body, and we're not running out of letters, so call it density.
The function takes a parameter s but this is not used. Better to omit it.
The function uses the global variable n. It would be better to pass this in as a parameter, or even better, deduce it from the size of matrix. Presumably the intention is that matrix is always an n × n array, in which case you should write matrix.size instead of n * n.
The name dispose is vague and does not convey much to me. I think the name populate would be better.
The function always returns matrix, which is useless, as the caller already knows about matrix.
The name matrix is slightly misleading, as Numpy has a matrix class but this parameter does not belong to it! It's actually an ndarray. I would call the parameter array or maybe just a.
The name sp is rather too specific. Presumably it is short for "species", but really nothing about this function depends on this value representing a species. You can imagine other types of lattice simulation where the array entries represent molecules, grains of sand, people, etc. So for generality I'd name this parameter value.
The value 0 is arbitrary and so for extra generality it should be a function parameter (taking the default value 0). Imagine having a "speciation" operation in which you replace some fraction of a species with a descendant. It would be nice to be able to reuse this code.
If dens is greater than 1, then limit will be greater than the number of elements in the array, and so the function will run forever.

Even if dens is less than 1, there might still not be enough positions in the array with value 0, and so the function will run forever.

Perhaps you plan not to fill very much of the array, but even so it is a good idea to write robust code in case you change your plans or want to reuse this code in other programs.
The way that the positions are found is to repeatedly generate random coordinates and check to see if the position is 0. This is inefficient. Imagine that dens is 1, so that all positions must be set to sp. How long will this take, on average? This is the well known "coupon collector's problem" so the expected number of coordinates to be randomly generated is \$ n^2 \log n^2 \$.

Perhaps you plan not to fill very much of the array, but again, it is a good idea to write code with the right asymptotic complexity, in case you change your plans or want to reuse this code in other programs.

So it would be better to find the positions containing 0 (using numpy.flatnonzero), then choose a random sample of those positions without replacement (using numpy.random.choice), and then set all the positions.

2. Performance

Putting all this together, I'd write the function like this:

def populate(array, density, value, original=0):
    """Choose array.size * density positions in array that are equal to
    original, uniformly at random, and set them to value.

    """
    a = array.reshape(-1)
    count = int(a.size * density)
    a[np.random.choice(np.flatnonzero(a == original), count, False)] = value

Note the use numpy.reshape to create a flattened view of array, so that we can use numpy.random.choice, which requires a 1-D array. We then use this view to update the original array.

However, this has unacceptably poor performance compared to dispose when density is small:

>>> a = np.zeros((1000, 1000))
>>> timeit(lambda:dispose(a, 0.01, None, 1), number=1)
0.0184222329990007
>>> timeit(lambda:populate(a, 0.01, 1), number=1)
0.2894051409966778

The reason for this poor performance is that numpy.random.choice computes a sample without replacement by permuting the whole array and using an initial prefix of the permutation. This is expensively wasteful when only a small part of the permutation is needed.

So what about sampling with replacement instead (which is fast), eliminating duplicates using numpy.unique and looping until we have enough choices (as in the original post)? Here's the implementation:

def populate(array, density, value, original=0):
    """Choose array.size * density positions in array that are equal to
    original, uniformly at random, and set them to value.

    """
    a = array.reshape(-1)
    count = int(a.size * density)
    while count:
        p = np.random.choice(a.size, count)
        p = np.unique(p[a[p] == original])
        a[p] = value
        count -= p.size

This is adequately fast: it's about ten times as fast as the original code, and more than a hundred times as fast as my first attempt above:

>>> a = np.zeros((1000,1000))
>>> timeit(lambda:populate(a, 0.01, 1), number=1)
0.0019981740042567253

However, it's not robust: it suffers from the same problems as the original code, discussed in §1.10 and §1.11 above. What we really need is a hybrid implementation: that tries the approach that's fast for small densities, but measures how well it's doing, and switches to the slow but sure approach for large densities. For example:

def populate(array, density, value, original=0):
    """Choose array.size * density positions in array that are equal to
    original, uniformly at random, and set them to value.

    """
    a = array.reshape(-1)
    count = int(a.size * density)
    while count:
        p = np.random.choice(a.size, count)
        p = np.unique(p[a[p] == original])
        a[p] = value
        count -= p.size
        if count > p.size * 10:
            # Not making much progress: switch to robust but slow algorithm
            a[np.random.choice(np.flatnonzero(a == original), count, False)] = value
            return

This remains very fast when density is small:

>>> a = np.zeros((1000, 1000))
>>> timeit(lambda:populate(a, 0.01, 1), number=1)
0.0022136839979793876

but still completes in a reasonable time when density is large:

>>> a = np.zeros((1000, 1000))
>>> timeit(lambda:populate(a, 0.99, 1), number=1)
0.3501391080208123
>>> a = np.zeros((1000, 1000))
>>> timeit(lambda:dispose(a, 0.99, None, 1), number=1)
5.953548110992415

and doesn't go into an infinite loop if there aren't enough spaces remaining:

>>> timeit(lambda:populate(a, 1.1, 1), number=1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  ...
  File "mtrand.pyx", line 1106, in mtrand.RandomState.choice (numpy/random/mtrand/mtrand.c:11223)
ValueError: Cannot take a larger sample than population when 'replace=False'


Get list of unique parents from childrens list Parent has list of Children. I only have access to the list of children. I need to get list of unique parents from the List of Children. Is there a better/faster/more efficient way of doing it than this?

var temp = new HashSet<Parent>(
                        (from child in program.Children
                        select child.Parent).ToList());

Your use of var is good, but names like temp is meaningless. It's a temp what exactly? A more descriptive name is in order here.

The call to ToList appears to be redundant. Hashset's constructor takes an IEnumerable<T> and Linq queries that return collections already return one.

I would also break the query away from the ctor for readability purposes.

var parents = from child in program.Children
                 select child.Parent;

var distinctParents = new HashSet<Parent>(allParents);

However, it's all kind of moot if you use the method syntax snippet that @RobH posted.

var parents = program.Children.Select(c => c.Parent).Distinct();


Refactor/Review JS function which moves a banner on window resize Okay, so I wrote this function yesterday for one of our websites here at the office, the site is responsive, but one of the banners wasn't behaving at a certain screen size (was getting chopped off when screen size was less than 1067px). Basically, this script checks for window size of 1067px (higher than 1068px, banner is fine) to 980px (lower than 981, banner is hidden)

function updateReadyGoBG(){
    var vpWidth, difference, position = 0;
    vpWidth = jQuery(this).width();

    if(vpWidth > 980 && vpWidth < 1067){
        position = jQuery(".ready_go_bg").css("left").replace("px", "")
        difference = (position - (600 - (vpWidth - position) ) ) + 100;
        jQuery(".ready_go_bg").css("left", difference + "px")
    }
}

Any ideas on how to improve this to improve performance or reduce calls? Any review, refactoring or just general thoughts would be highly appreciated. What did I do wrong? What would you change?

Thanks all!

EDIT: adding rudimentary html snipped this acts on

        Ready. Go.

Ok, assuming:

You didn't use media queries for cross-browser reasons
This function is already bound to a resize event, since you didn't include what triggers the function call
Assuming that this is the window object, judging from the way you call it vp as in viewport and getting its width.

Then:

function updateReadyGoBG(){

        //you can actually merge declaration and assignment
    var vpWidth   = jQuery(this).width(),
        //cache the ready_go_bg jQuery object to avoid requerying
        readyGoBg =  jQuery('.ready_go_bg'),
        position  = 0,
        difference;

    if(vpWidth > 980 && vpWidth < 1067){

        //you could use position instead of getting the CSS left
        //to avoid getting a string, and chopping off the "px" part
        position = readyGoBg.position().left;

        difference = (position - (600 - (vpWidth - position) ) ) + 100;

        //sadly, position is just a "getter".
        //you could use .offset(value) but calculating relative
        //to your offset parent would be too much work
        //we use .css(prop,value) to set the left value
        //if unit wasn't specified, jQuery assumes it's in pixels
        readyGoBg.css('left', difference);
    }
}

function updateReadyGoBG() {
    var vpWidth = jQuery(this).width(),
        readyGoBg = jQuery('.ready_go_bg'),
        position = 0,
        difference;
    if (vpWidth > 980 && vpWidth < 1067) {
        position = readyGoBg.position().left;
        difference = (position - (600 - (vpWidth - position))) + 100;
        readyGoBg.css('left', difference);
    }
}

General Advice:

Never forget ; at every line. You may run into issues during minification if you forget them
Though personal preference, but it's cleaner to use ' instead of ".
Unless totally necessary, I'd avoid absolute positioning. Let the layout dictate positions.


Nth Fibonacci number (Bottom-up with dict) This is an algorithm that I wrote to get the nth Fibonacci number using bottom-up dictionary. How can I make this better/more efficient?

memo={1:1,2:1}
f=0

def Fib(n):
    for i in range(3,n+1):
        memo[i]=memo[i-1]+memo[i-2]

    f=memo[n]
    print(f)
    return f

Fib(15)

Before we come to the actual algorithm: python has an official style-guide, PEP8. It recommends using lower_case for variable and function names (and PascalCase for classes). It also recommends avoiding unnecessary blank lines, which means having exactly two blank lines before a function definition (one if it is a class method) and only a single blank line to separate logical code blocks if necessary. Lastly, you should put a blank after the comma in an argument list or a colon ({1: 1, 2: 1} and not {1:1,2:1})

You should learn about decorators!

In the most simple way you could use it like this:

def memoize(func):
    func.memo = {1: 1, 2: 1}
    return func

@memoize
def Fib(n):
    for i in range(3, n+1):
        Fib.memo[i] = Fib.memo[i-1] + Fib.memo[i-2]
    return Fib.memo[n]

print Fib(15)

This makes a memo object bound to the func.

What you really want, though, is a decorator that silently caches every result, whenever you call your function. As it stands, you are also wasting cycles. If you calculate Fib(15) and after that Fib(16) it would be nice if the code took the already computed values of Fib(14) and Fib(15). Your code calculates all intermediate values for every n, though.

Here are two memoization decorators that can do what you need:

import functools

def memoize(func):
    cache = func.cache = {}

    @functools.wraps(func)
    def wrapper(n):
        if n not in cache:
            cache[n] = func(n)
        return cache[n]
    return wrapper

def memodict(f):
    """ Memoization decorator for a function taking a single argument """
    class MemoDict(dict):
        def __missing__(self, key):
            ret = self[key] = f(key)
            return ret
    return MemoDict().__getitem__

@memodict
def fib(n):
    if n == 0:
        return 0
    if n in (1, 2):
        return 1
    return fib(n-1) + fib(n-2)

The first decorator is a bit easier to understand, as it does a bit what you started out doing already. It defines a cache for the function and stores the result of the function in the cache if it is not already in there. It returns the value in the cache.

The second is a more optimized and is slightly faster. It only works with single-valued functions (as yours is).  It uses the fact that the magic method __missing__ is called for a dict if a requested key does not exist, before raising a KeyError. In this method the cache is populated.

Important limitation:
Both of these implementations will raise a RuntimeError with recursion limit exceeded if you want to get a too big Fibonacci number right away (where too big is already somewhere between 250 and 300). Here, you can, however, use what you already have and just calculate all Fibonacci numbers up to that number:

for n in xrange(large_number):  # Use range in python 3.x
    fib(n)
print fib(large_number)


filter out password in xml string We're storing the xml communications with an external service in a text in the db and in a before_create I've got the following:

  # filter opera password
  def remove_password!
    self.request.gsub!  /UserPassword\>[^\<]*\<\//,
                        'UserPassword>[FILTERED]</'
  end

Is there a better, safer way of doing it?

That depends on how confident you are that neither of these cases will be true:

Another tag like  will be introduced that you will want to leave unfiltered
No nested tags will appear within the tag

An example of the last would be:

<UserPassword><for name="boy">Sue</for></UserPassword>

It seems your assumptions are safe, but if you want to be certain, go with

self.request.gsub! /\<UserPassword\>.+?\</UserPassword\>/gm, '<UserPassword>[FILTERED]</UserPassword>'

By eliminating the greediness of the regex, you can match the exact tag you mean and reduce the assumptions that will come back and bite you.


Divide a 3D matrix into multiple smaller 3D matrix I'm trying to build an extension method that will allow me to subdivide a huge array into much smaller ones. My current code is the following but it still takes about 2s to generate with an array of size 512x512x512 and the size of the sub cubes set to 32x32x32
public static T[][][][] Divide<T>(this T[][][] array, int xSize, int ySize, int zSize)
{
    int numberOfCubesInAxisX = array.Length / xSize;
    int numberOfCubesInAxisY = array[0].Length / ySize;
    int numberOfCubesInAxisZ = array[0][0].Length / zSize;
    T[][][][] arrayDivided = new T[numberOfCubesInAxisX * numberOfCubesInAxisY * numberOfCubesInAxisZ][][][];
    int index = 0;
    while (index < arrayDivided.Length)
    {
        int xIndex = index / numberOfCubesInAxisZ / numberOfCubesInAxisX;
        int yIndex = index / numberOfCubesInAxisZ % numberOfCubesInAxisY;
        int zIndex = index % numberOfCubesInAxisZ;
        arrayDivided[index] = new T[xSize][][];
        for (int x = 0; x < xSize; x++)
        {
            arrayDivided[index][x] = new T[ySize][];
            for (int y = 0; y < ySize; y++)
            {
                arrayDivided[index][x][y] = new T[zSize];
                for (int z = 0; z < zSize; z++)
                {
                    arrayDivided[index][x][y][z] = array[x + (xIndex * xSize)][y + (yIndex * ySize)][z + (zIndex * zSize)];
                }
            }
        }
        index++;
    }

    return arrayDivided;
}

If anyone have any clue on how to optimize it, i'll be glad to hear it!
Thanks!

Just a quick shot regarding performance.....
The code is calculating
xIndex * xSize xSize*ySize*zSize times
yIndex * ySize xSize*ySize*zSize times
zIndex * zSize xSize*ySize*zSize times
while it only should be calculated once for each iteration of the while loop.
In addition, x only changes in the first for loop and y only changes in the second for loop.


Is this the correct way to expose a setting for a vb.net class library? I define in the Settings Tab of a VB.Net class library some user scoped settings. I want to expose these settings to other projects that reference the class library. I chose to do this using a public Module. This works, but it requires exposing each and every setting manually. Is there a better way?

Public Module Settings

    Public Property SomeSetting As Integer
        Get
            Return My.Settings.SomeSetting
        End Get
        Set(value As Integer)
            My.Settings.SomeSetting = value
            My.Settings.Save()
        End Set
    End Property

End Module

I think it's a fairly verbose way of exposing app settings. How about this?

(taken from an answer on this SO question)

This essentially comes down to what @KonradRudolph's comment was saying - the reason you "need" your Settings module is probably because the settings' access modifier is set to Internal, which means it's not accessible to other assemblies.

If you need to expose app settings to other assemblies, you just change that access modifier to Public and you're done.

Doing this:

Public Module Settings
    Public Property AppSettings As Settings
        Get
            Return My.Settings
        End Get
    End Property
End Module

Is then totally redundant. Even more so, doing what you've done (exposing each setting individually) becomes more painful to maintain than it needs to be; if the settings are public then you expose the settings class itself and whatever you change is immediately available to client assemblies.


Adding some wait time between API requests to avoid overloading the server In Shopify a product can belong to a collection.
I have written a method which accepts an organisation. It would then get a list of collections that this organisation has from Shopify. Finally, it has to loop through each collection to get the products that belong to this collection.
I don't want to overload Shopify server by sending too many requests, so I have added 75 ms wait between each request. But since this method is async, I am not quite sure if the responses are handled correctly? Do I need any sort of mutext or locking mechanism to ensure each response is being handled correctly?
public async Task<List<ShopifyCollection>> GetShopifyCollections(Organisation organisation)
{
    int waitInMilliSeconds = 50;
    var shopifyCollections = new List<ShopifyCollection>();
    var customCollections = await GetAllCustomCollections(organisation);

    foreach (var cc in customCollections)
    {
        shopifyCollections.AddCollection(cc.Id, cc.Title);
    }

    foreach (var curCollection in shopifyCollections)
    {
        var products = await GetCollectionProducts(organisation, curCollection.CollectionId);
        curCollection.ProductIds = products.Where(p => p.Id != null && p.Id > 0).Select(p => p.Id.Value).ToList();
        await Task.Delay(waitInMilliSeconds);   // <-- do not overload Shopify
    }

    return shopifyCollections;
}

Create throttling for API calls is just one aspect of making your API integration resilient. There are many other types of failures to handle. Instead of weaving the error handling into your code, you should consider using something like Polly https://github.com/App-vNext/Polly
There a more details on it here http://www.thepollyproject.org/2018/03/06/policy-recommendations-for-azure-cognitive-services/


Looking within a directory for a file I have written this code to build a list by looking inside directory within deep to the level it finds a audio file and then move to the next level or next folder.

Can there be a more efficient way since a user can have any number of files? Do you think it can be achieved more robustly in terms of performance?

path = '/Users/ceaser/Music'

import os
allSongs = []
for folder in os.walk(path):
    for item in folder[-1]:
        name, ext = os.path.splitext(item)
        if ext in ['.mp3', '.wma', '.wav', '.ogg']:
            allSongs.append(name)

The performance of this will largely depend on filesystem I/O,
and the number of files and subdirectories in your target folder.

It won't make a difference in practice,
but in theory it will be faster to check the extension against a set instead of a list, for example:

extensions = {'.mp3', '.wma', '.wav', '.ogg'}
for dirpath, dirnames, filenames in os.walk(path):
    for item in filenames:
        name, ext = os.path.splitext(item)
        if ext in extensions:
            allSongs.append(name)

Notice that I changed the for folder in os.walk(path) to the more intuitive for dirpath, dirnames, filenames in os.walk(path),
which makes the inner loop more intuitive too,
for item in filenames instead of the cryptic for item in folder[-1].

But most of all,
I'm wondering if it's really os.walk you want to use.
This function is useful for traversing a directory tree,
down into arbitrary levels of subdirectories.
I'm questioning your intention because you only keep filenames, not their paths.
So for example a song The Four Horsemen.mp3 may exist in both /Users/ceaser/Music/Metallica and in /Users/ceaser/Music/Mix/Oldies-But-Goodies, and then you will have The Four Horsemen.mp3 in allSongs twice.
Not caring about the intermediary directories suggests that perhaps there are no intermediary directories, all songs are simply directly under /Users/ceaser/Music.
In that case, instead of os.walk, using os.listdir would be more appropriate and simpler too.

Lastly, there is an official coding style guide for Python,
called PEP8,
I recommend to give it a good read and follow it in the future.


Simple HTML Website (followup) Followup to this

I have created a simple HTML5/CSS3/JS website with a navbar and no content.

index.html:

<!DOCTYPE html>

  <meta charset="UTF-8" />
  <title>SigmaCubes - Home</title>
  <meta name="author" content="Julian Lachniet,Simon Kwilinski,Jacob Wysko" />
  <meta name="description" content="SigmaCubes is the best website on the internet." />
  <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0" />
  <link href="https://fonts.googleapis.com/css?family=Lato|Noto+Sans|Nunito:600|Material+Icons" rel="stylesheet" type="text/css" />
  <link href="/lib/css/reset.css" rel="stylesheet" type="text/css" />
  <link href="/lib/css/sigmacubes.css" rel="stylesheet" type="text/css" />
  <link href="/favicon.ico" rel="shortcut icon" />

Error:

JavaScript

Error:

Resolution

        About Us
        Projects
        Contact
        Donate

      About Us
      Projects
      Contact
      Donate

reset.css:

html, body, div, span, applet, object, iframe, h1, h2, h3, h4, h5, h6, p,
blockquote, pre, a, abbr, acronym, address, big, cite, code, del, dfn, em, img,
ins, kbd, q, s, samp, small, strike, strong, sub, sup, tt, var, b, u, i,
center, dl, dt, dd, ol, ul, li, fieldset, form, label, legend, table, caption,
tbody, tfoot, thead, tr, th, td, article, aside, canvas, details, embed,
figure, figcaption, footer, header, hgroup, menu, nav, output, ruby, section,
summary, time, mark, audio, video {
  margin: 0;
  padding: 0;
  border: 0;
  font-size: 100%;
  font: inherit;
  vertical-align: baseline;
}

article, aside, details, figcaption, figure, footer, header, hgroup, menu, nav,
section {
  display: block;
}

body {
  line-height: 1;
}

ol, ul {
  list-style: none;
}

blockquote, q {
  quotes: none;
}

blockquote:before, blockquote:after, q:before, q:after {
  content: "";
  content: none;
}

table {
  border-collapse: collapse;
  border-spacing: 0;
}

main.js:

var navEnabled = false;

document.addEventListener('DOMContentLoaded', main);

function main() {
  setDisplay("wrapper", "block");
}

function toggleNav() {
  if (navEnabled) {
    setDisplay("nav-small-links", "none");
  } else {
    setDisplay("nav-small-links", "block");
  }
  navEnabled = !navEnabled;
}

function setDisplay(id, state) {
  document.getElementById(id).style.display = state;
}

What can I try to improve?

title

It’s better for usability to use "page-title - site-name" instead of "site-name - page-title".

But for the homepage, you might want to omit the page-title ("Home"), as the site/domain name only typically represents the homepage.

<title>SigmaCubes</title>

Error messages

You shouldn’t use a heading element for the content, as this creates an outline that’s not useful. Use p instead. And to convey the importance of the error, you could use the strong element.

Error

Old Internet Explorer is not supported

You should add the hidden attribute to your error messages, and remove it as soon as the error becomes relevant. Otherwise the markup affects your outline (i.e., you have all possible errors listed at the top of the document outline).

(For the error about disabled JavaScript, you’d need to add the hidden attribute via JS itself, of course.)

alt for logo

The alt attribute for a logo shouldn’t contain the term "logo" (unless that’s part of the name, of course). Just use the site name.

alt="SigmaCubes"

Navigation

Use the nav element for the navigation.

Use a ul element for the navigation links (without a list, you should use some kind of textual delimiter separating the navigation links, e.g. for text browsers).


Delete folder if it exists in PowerShell In my PowerShell script I'm trying to delete a folder, but only if it exists:

if (Test-Path $folder) { Remove-Item $folder -Recurse; }

I find myself repeating this combination of cmdlets quite a few times, and wished I had something like this:

# Pseudo code:
Remove-Item $folder -Recurse -IgnoreNonExistentPaths

Is there a way to do something like that? A different command, or an option I've missed from the Remove-Item documentation perhaps? Or is the only way to DRY out this bit of code to write my own cmdlet that combines Test-Path and Remove-Item?

I assume you're just trying to avoid the error message in case it doesn't exist.

What if you just ignore it:

Remove-Item $folder -Recurse -ErrorAction Ignore

If that's not what you want, I would recommend writing your own function:

function Remove-ItemSafely {
[CmdletBinding(SupportsShouldProcess=$true)]
param(
    [Parameter(
        Mandatory=$true,
        ValueFromPipeline=$true,
        ValueFromPipelineByPropertyName=$true
    )]
    [String[]]
    $Path ,

    [Switch]
    $Recurse
)

    Process {
        foreach($p in $Path) {
            if(Test-Path $p) {
                Remove-Item $p -Recurse:$Recurse -WhatIf:$WhatIfPreference
            }
        }
    }
}