Getting all possible letter combinations of a word I wrote a simple algorithm for finding all possible letter combinations of a single word. I want to know if my code can be improved in any way, although I'm mostly interested in efficiency.
You can ignore my notes if you wish, they are mostly for myself and the questions in them are not the ones I ask answers for here^^

The code:

#include <iostream>
#include <algorithm>
#include <vector>
#include <fstream>
#include <unordered_set>

/**
    1. Used std::vector to store words, then sorted it and removed duplicates.
        >4s for 10 letters.
    2. Moved to std::unordered_set. Time improved to >2s.

    Notes:
        I used [i] instead of at(i) for efficiency in some cases (at(i) does bound checking
        (right?), and I do that myself, so it's unnecessary
        (or is the performance hit too small to matter?)).
**/

const char* outFile = "wordCombos.txt";

// Only this function should be used in main()
// Gets all possible letter combinations from a single word, prints them to a file
// Does not modify the original string
void unscramble(std::string str);

int main()
{
    // Opening the file to clear it
    std::ofstream out(outFile, std::ios::trunc);
    out.close();

    // Test values
    unscramble("aaa");
    unscramble("     Amazing \t\t\t\n  \n");
    unscramble("   \t\t\n  \n   \n \t\t");
    // This last one already takes 4 seconds. 10 letters
    // More than 3.5 million words
    unscramble("lakdjflakk");
    // 11 letters - 27 seconds
    //unscramble("lakdjlakkpp");
}

// Remove tabs, newlines, spaces
void trimSpaces(std::string& a)
{
    if(a.find_first_not_of(" \n\t") == std::string::npos){
        a = "";
        return;
    }
    int begin = a.find_first_not_of(" \n\t");
    int length = a.find_last_not_of(" \n\t") - begin + 1;
    a = a.substr(begin, length);
}

// Process a string, add all possible letter combos to word
// Assumes that a valid letters vector is given (size > 0)
// Uses recursion
void getLetterCombos(std::unordered_set<std::string>& words,
                     const std::vector<char>& letters,
                     std::string curWord)
{
    if(letters.size() == 1){
        words.insert(curWord + letters[0]);
        return;
    }

    for(int i = 0; i < letters.size(); ++i){
        std::vector<char> temp = letters;
        std::string tWord = curWord + temp[i];
        temp.erase(temp.begin() + i);
        getLetterCombos(words, temp, tWord);
    }
}

void unscramble(std::string str)
{
    trimSpaces(str);

    if(str == "" || str.find_first_of(" \t\n") != std::string::npos){
        std::cout << "Error: Invalid input: no word or more than a single word given.\n";
        return;
    }

    // All combos should be in uppercase
    std::transform(str.begin(), str.end(), str.begin(), toupper);

    std::unordered_set<std::string> words;
    std::vector<char> letters;
    letters.reserve(str.size());
    for(int i = 0; i < str.size(); ++i){
        letters.push_back(str[i]);
    }
    getLetterCombos(words, letters, "");

    std::ofstream out(outFile, std::ios::app);
    out << "Initial word: " << str << ",\nAll possible letter combinations: \n";
    for(std::unordered_set<std::string>::iterator it = words.begin(); it != words.end(); ++it){
        out << *it << "\n";
    }
    out << "\n";
}

I see some things that may help you improve your code.

Consider separating input and output

Right now, the unscramble function does both the permutation and the printing of both outputs and errors. A more modular (and likely more maintainable) approach would separate these into separate functions.

Return something useful from functions

Every single one of the routines is declared as returning void.  Something is wrong there.  For example, the unscramble routine could return the number of permutations or 0 on error.

Throw errors rather than printing error messages

The user of your code may be creating a GUI with no command line available and will not appreciate having your code printing instead of indicating an error to the calling code.  The two ways that C++ programs generally signal an error are either by throwing an exception (if the circumstance really is exceptional) or by returning a value that indicates an error.

Eliminate global variables

Eliminating global variables allows your code to be more readable and maintainable, both of which are important characteristics of well-written code.  Global variables introduce messy linkages that are difficult to spot and error prone.  Either the file name or better, a std::ostream reference, should be passed rather than having the filename be a global variable.  Better still, have subroutines simply operate by passing a string in or out and collect the I/O elsewhere.

Use standard algorithms

You might want to use std::next_permutation to accomplish this.  It would make the code much simpler, and you're already including the required header.


Fast way to count the numbers that are divisible by their own sum of digits inside a range Inspired by this question on Stack Overflow, I wrote a function that returns the length of a list made of numbers that are divisible by their own sum of digits inside a range:

-- Returns the digits of a positive integer as a list, in reverse order.
-- This is slightly more efficient than in forward order.
digitsRev :: Int -> [Int]
digitsRev i = case i of
    0 -> []
    _ -> lastDigit : digitsRev rest
where (rest, lastDigit) = quotRem i 10

-- Returns the digits of a positive integer as a list.
digits :: Int -> [Int]
digits = reverse . digitsRev

-- Returns the sum of digits of a positive integer
sumDigits :: Int -> Int
sumDigits = sum . digits

-- Returns True if a number is divisible by the sum of it's digits and False
-- otherwise.
isDivisibleSumDigits :: Int -> Bool
isDivisibleSumDigits n = n `mod` sumDigits n == 0

-- Returns a range of positive integers in which every element is divisible by
-- the sum of it's digits. Includes the end of the range.
rangeDivisibleSumDigits :: Int -> Int -> [Int]
rangeDivisibleSumDigits range_start range_end =
    [n | n <- [range_start..range_end], isDivisibleSumDigits n == True]

-- Returns the length of the list returned by rangeDivisibleSumDigits
lengthRangeDivisibleSumDigits :: Int -> Int -> Int
lengthRangeDivisibleSumDigits range_start range_end =
    length $ rangeDivisibleSumDigits range_start range_end

Then I used this function inside a main function, like this:

main = putStrLn $ show $ lengthRangeDivisibleSumDigits 1 10000000

According to the file generated by profiling the program (compiling like this: ghc -prof -fprof-auto -rtsopts Main.hs and executing like this: ./Main +RTS -p) it took 14.36 seconds to execute main. That's slow.

Do you have ideas on a faster way to perform the task carried out by this function?

Edit

Thanks to @bisserlis 's answer below, now the code reads:

-- Returns the digits of a positive integer as a list.
digits :: Int -> [Int]
digits = map digitToInt . show

-- Returns True if a number is divisible by the sum of it's digits and False
-- otherwise
isDivisibleSumDigits :: Int -> Bool
isDivisibleSumDigits n = n `mod` (sum (digits n)) == 0

-- Returns a range of positive integers in which every element is divisible by
-- the sum of it's digits. Includes the end of the range.
rangeDivisibleSumDigits :: Int -> Int -> [Int]
rangeDivisibleSumDigits start end = filter isDivisibleSumDigits [start..end]

Compile without profiling, but with optimizations turned on.

bissbook:divSumDigits bisserlis$ ghc -prof -fprof-auto -rtsopts original.hs
[1 of 1] Compiling Main             ( original.hs, original.o )
Linking original ...
bissbook:divSumDigits bisserlis$ ./original +RTS -s
806095
  44,577,697,768 bytes allocated in the heap
      81,431,504 bytes copied during GC
          62,504 bytes maximum residency (2 sample(s))
          23,392 bytes maximum slop
               1 MB total memory in use (0 MB lost due to fragmentation)

                                    Tot time (elapsed)  Avg pause  Max pause
  Gen  0     85945 colls,     0 par    0.48s    0.57s     0.0000s    0.0001s
  Gen  1         2 colls,     0 par    0.00s    0.00s     0.0002s    0.0002s

  INIT    time    0.00s  (  0.00s elapsed)
  MUT     time   22.36s  ( 22.79s elapsed)
  GC      time    0.48s  (  0.57s elapsed)
  RP      time    0.00s  (  0.00s elapsed)
  PROF    time    0.00s  (  0.00s elapsed)
  EXIT    time    0.00s  (  0.00s elapsed)
  Total   time   22.85s  ( 23.35s elapsed)

  %GC     time       2.1%  (2.4% elapsed)

  Alloc rate    1,993,498,737 bytes per MUT second

  Productivity  97.9% of total user, 95.8% of total elapsed

bissbook:divSumDigits bisserlis$ rm *.hi *.o
bissbook:divSumDigits bisserlis$ ghc -O2 original.hs
[1 of 1] Compiling Main             ( original.hs, original.o )
Linking original ...
bissbook:divSumDigits bisserlis$ ./original +RTS -s
806095
  12,176,087,768 bytes allocated in the heap
       4,273,680 bytes copied during GC
          44,416 bytes maximum residency (2 sample(s))
          21,120 bytes maximum slop
               1 MB total memory in use (0 MB lost due to fragmentation)

                                    Tot time (elapsed)  Avg pause  Max pause
  Gen  0     23449 colls,     0 par    0.09s    0.11s     0.0000s    0.0001s
  Gen  1         2 colls,     0 par    0.00s    0.00s     0.0002s    0.0002s

  INIT    time    0.00s  (  0.00s elapsed)
  MUT     time    2.87s  (  2.93s elapsed)
  GC      time    0.09s  (  0.11s elapsed)
  EXIT    time    0.00s  (  0.00s elapsed)
  Total   time    2.96s  (  3.04s elapsed)

  %GC     time       3.0%  (3.7% elapsed)

  Alloc rate    4,237,427,802 bytes per MUT second

  Productivity  97.0% of total user, 94.6% of total elapsed

(Note the Total   time lines, 22.85s and 2.96s.)

I can't appreciably increase the performance any further, but I do have a couple stylistic notes.

Data.Char provides a function digitToInt :: Char -> Int, which makes writing digits much easier.

import Data.Char (digitToInt)

digits :: Int -> [Int]
digits = map digitToInt . show

No need to use list comprehensions and boolean double-checking in rangeDivisibleSumDigits, just write it as a filter on the range.

rangeDivisibleSumDigits :: Int -> Int -> [Int]
rangeDivisibleSumDigits start end = filter isDivisibleSumDigits [start..end]

sumDigits and lengthRangeDivisibleSumDigits are unnecessary functions, their names are pretty much as long as their implementations! They clutter up the top-level, they don't do anything surprising or interesting, I'd just use their implementations wherever you need to instead of adding another function to your mental burden.


Manage a collection of settings in memory with global scope I have a WPF app and a bunch of read/write settings that need to be accessible from various view models. a setting can be any type (string, bool, etc) and all are settable, but only from a 'Settings' view. I also need the view models to be notified when any setting has been updated and which one that was.

Here is an enum with all the settings:

public enum Setting
{
    SettingA,
    SettingB,
    //etc
}

here is the settings class, as a singleton:

  internal sealed class Settings
{
    private static volatile Settings _instance;
    private static object _syncRoot = new object();
    private Dictionary<Setting, object> _settings;

    private Settings()
    {
        _settings = new Dictionary<Setting, object>();
        _settings.Add(Setting.SettingA, 1000);
        _settings.Add(Setting.SettingB, 1000);
    }

    public static Settings Instance
    {
        get
        {
            if (_instance == null)
            {
                lock (_syncRoot)
                {
                    if (_instance == null)
                    {
                        _instance = new Settings();
                    }
                }
            }
            return _instance;
        }
    }

    public void UpdateSetting(Setting key, object value)
    {
        if (_settings[key] != value)
        {
            _settings[key] = value;
            TriggerNotification(key, value);
        }
    }

    public T GetSetting<T>(Setting key)
    {
        return (T)_settings[key];
    }

    public event EventHandler<SettingsUpdatedEventArgs> OnSettingsUpdated;

    private void TriggerNotification(Setting key, object value)
    {
        var handler = OnSettingsUpdated;
        if (handler != null)
        {
            handler(this, new SettingsUpdatedEventArgs(key, value));
        }
    }
}

here is the overrided event args:

 public class SettingsUpdatedEventArgs : EventArgs
{
    public Setting Type { get; private set; }
    public object Value { get; private set; }

    public SettingsUpdatedEventArgs(Setting type, object value)
    {
        Type = type;
        Value = value;
    }
}

and here is how it is used:

    //update a setting value
    Settings.Instance.UpdateSetting(Setting.SettingA,200);

    //retrieve a setting value
    var value = Settings.Instance.GetSetting<int>(Setting.SettingA);

    //register a listener
    Settings.Instance.OnSettingsUpdated += (sender, args) =>
    {
        if (args.Type == Setting.SettingA)
        {
            var result = args.Value;
        }

    };

and now my questions:

Is there a problem having the settings class as a singleton?
Is this a type-safe way to get the setting values?
Is there a much better/simpler/more elegant way of doing this?

Is this a type-safe way to get the setting values?

It's not type-safe in the sense that if you make a mistake in specifying the type parameter of GetSetting, or if someone sets  setting to the wrong type, you're going to get an exception.

I've struggled with this issue before and I have created a solution to this that uses generic keys. It's by no means perfect, but you can use it as an inspiration.


Trigger breakpoint when string is not a Google-owned domain Basically, I want the code to break when strLink doesn't contain any domains in the googleDomains link.

The code seems to be very awkward. I wonder if there is a way to make this more elegant.

Dim googleDomains = {"google.com", "blogger.com", "youtube.com"}
Dim pass = False
For Each domaingoogle In googleDomains
    If strLink.Contains(domaingoogle) Then
        pass = True
    End If
Next
If Not pass Then
    Dim hello2 = 1 'set breakpoint here.
End If

One thing you could look at is Regex. If you're willing to assume that strLink is in fact a valid URL you could try something like

Dim googleDomains = {"google.com", "blogger.com", "youtube.com"}
Dim regexp = "^(?:https?:\/\/)?(?:[^@\/\n]+@)?(?:www\.)?([^:\/\n]+\.com)"
Dim match = Regex.Match(strLink, regexp)
If match.Length <> 0 Then
    Dim domain As String = match.Groups(1).Value
    If Not googleDomains.Contains(domain) Then
        Dim hello2 = 1 'set breakpoint here.
    End If
End If

If you wanted to expand to include ".org" or ".net" you could do something like this:

Dim regexp = "^(?:https?:\/\/)?(?:[^@\/\n]+@)?(?:www\.)?([^:\/\n]+\.(com|org|net|gov))"

Modified Example Regex Demo

In general, I'd say Regex is a very useful tool to have in your arsenal and would be good to learn. Comes in handy when you need to scan large code files or restructure something in a lot of places (as Regex can do replacements as well)

In this example the main benefit is that you can actually extract out the domain from a well-formed URL rather than just see if "google.com" is anywhere in the string

If you want the actual check to truly be String.Contains You could do something with LINQ

Dim domain = googleDomains.FirstOrDefault(Function(d) strLink.Contains(d))
If domain = Nothing Then
    Dim hello2 = 1 'set breakpoint here.
End If

A little bit less verbose, but it doesn't necessarily mean the string is a valid URL to google.com


Trie data structure in C++ with add and search I am learning C++11 and wanted to practice data structures so here is my trie implementation with add and search functions. What are some things to improve?

trie.h

#pragma once
#include <iostream>
#include <string>
#include <vector>
#include <algorithm>
#include <stack>

namespace {
    class Node
    {
    public:
        Node();
        ~Node();
        Node(char c);
        char get_value() const;
        bool get_leaf() const;
        void set_leaf();

        Node* get_child(const char key);
        std::vector<Node *> children;

    private:
        char value;
    };
}

class NodePointerComp
{
public:
    explicit NodePointerComp(const char key);
    char const key_;
    bool operator()(Node const *n) const;

};

class Trie
{
public:
    Trie();
    ~Trie();
    void add_word(std::string const &word);
    bool search(std::string const &word);
    std::vector<std::string> find_suffix(std::string const &s, int max);

private:
    Node *root;
    Node* find_string(std::string const &word);
};

trie.cpp

#include "trie.h"

Node::Node() {}
Node::Node(char c) : value(c) {}

char Node::get_value() const { return value < 0 ? -value : value; }

bool Node::get_leaf() const { return value < 0 ? true : false; }

// a string with char -128 or a nullchar will break this
void Node::set_leaf()
{
    if (value > 0)
        value = -value;
}

NodePointerComp::NodePointerComp(const char key) : key_(key) {}

bool NodePointerComp::operator()(Node const *n) const { return n->get_value() == key_; }

Node* Node::get_child(const char key)
{
    auto x = std::find_if(children.begin(), children.end(), NodePointerComp(key));
    if (x == children.end())
        return nullptr;

    return *x;
}

Node::~Node()
{
    for (auto c : children)
        delete c;
}

Trie::Trie()
{
    root = new Node();
}

void Trie::add_word(std::string const &word)
{
    Node *curr = root;

    uint32_t i = 0;
    for (i; i < word.length(); i++)
    {
        if (curr->get_child(word[i]) == nullptr)
            break;

        curr = curr->get_child(word[i]);
    }

    for (i; i < word.length(); i++)
    {
        curr->children.push_back(new Node(word[i]));
        curr = curr->children.back();
    }

    curr->set_leaf();

}

std::vector<std::string> Trie::find_suffix(std::string const & s, int max)
{
    Node *curr = find_string(s);
    if (!curr || max == 0)
        return {};

    std::vector<std::string> output;
    std::stack<std::pair<Node*, std::string>> st;

    for (auto c : curr->children)
        st.push({ c, ""});

    while (!st.empty())
    {
        auto p = st.top();
        st.pop();

        p.second += p.first->get_value();
        if (p.first->get_leaf())
        {
            output.push_back(p.second);
            if (output.size() == max)
                break;
        }

        for (auto c : p.first->children)
            st.push({ c, p.second });

    }

    return output;
}

Node* Trie::find_string(std::string const & word)
{
    Node *curr = root;
    std::string str;

    for (auto c : word)
        if ((curr = curr->get_child(c)) == nullptr)
            return nullptr;

    return curr;
}

bool Trie::search(std::string const &word)
{
    auto *p = find_string(word);
    if (p) return p->get_leaf();
    return false;
}

Trie::~Trie()
{
    delete root;
}

This is a pretty good implementation from what I see. It's easy to read and looks very straightforward. Nice work!
Naming
For the most part your naming is good. There are a few small things I would change. For example, the get_leaf() method. When I read the name I thought it was going to retrieve a leaf node, but I couldn't see how it could determine which one. In reality, it returns a bool that tells whether it is a leaf or not. I would rename it to is_leaf() to avoid confusion.
I also don't like the word value as it's fairly meaningless. It represents a letter in the set of words formed by that branch of the trie. I would name it character or letter.
Organization
For the default constructor for a Node, in C++11 and later, you don't need to write it out by hand. In the header, you can just declare it as:
Node() = default;

In trie.cpp you have the implementation of 3 classes. I usually break them out into a separate source file per class. Given how small these are, it's not a huge deal to put it all in one file. I would, however, keep all methods for a class together. You have several methods of Node, then 2 methods of NodePointerComp, then more methods of Node. It makes it a little hard to find things when examining the source. So I'd move all methods from a single class together into 1 section. You might also want to add either a comment break between them (just something like //----NodePointerComp methods----) or, if your compiler supports it, something like a #pragma mark NodePointerComp. In my IDE, when the editor sees #pragma mark <Name>, it adds <Name> to the pop-up menu with all the functions in it. That makes it really easy to navigate to those methods quickly.
Simplify
It looks like you're using the convention of a negative value representing whether a node is the end of a word or not. I originally thought that you were trying to define the last node in a branch, but I see from the comments I was mistaken. That's a good indication that the name is too confusing and the implementation too complex.
I'm not an expert on the C++ spec, but I thought that whether char was signed or not (when not explicitly specified) was implementation-dependent. Even if it's not, that's a tenuous way to determine the end of a word. I recommend a bool that says it's the end of a word. It would simplify the get_leaf() and set_leaf methods(). Also they should be renamed to is_word_end() and set_word_end().
Encapsulation
I'm a little uncomfortable with Node::children being public. That's an implementation detail, and I think it would be a good idea to hide that detail from users of the class. It would make the logic in Trie slightly less straightforward, but I think the tradeoff would be worth it in the long run. Right now, Trie can modify that variable and Node won't necessarily be aware of it. It also makes it more difficult to use in a multithreaded environment, should you need to do that. If it were private, you could ensure that it was never accessed outside of some sort of lock (or mutex, whatever) inside the class.


Creating submenus with buttons I have a menu, where a user selects one out of 4, and another 2 or 4 options will appear. I used big buttons; a picturebox + label to create the buttons.

The 4 main buttons are fixed, but the 4 other buttons depend on the first choice. Text and image will change. I programmed this, but I kinda feel this must be possible more easily.

I have 4 structs which contain the image path and label text. like this: (I'll only show with 2, because the rest won't add anything, it is just the same.)

private const string _mainButton1 = "ConfigDP";
private const string _mainButton2 = "ConfigADP";

struct ConfigDPSubmenu
{
    public const string _SubMenuBtn1Text = "text submenu 1 button 1";
    public const string _SubMenuBtn2Text = "text submenu 1 button 2";
    public const string _SubMenuBtn1Img = "DevPortal.png";
    public const string _SubMenuBtn2Img = "CustomDevPortal.png";
}
struct ConfigADPSubmenu
{
    public const string _SubMenuBtn1Text = "text submenu 2 button 1";
    public const string _SubMenuBtn2Text = "text submenu 2 button 2";
    public const string _SubMenuBtn1Img = "ADP.png";
    public const string _SubMenuBtn2Img = "CustomADP.png";
}

Then I have the function where I make the buttons: (it is called when press on the main buttons)

private void CreateSubButtons(string sender)
{
     pictureBoxSubBtn3.Image = null;
     pictureBoxSubBtn4.Image = null;
     SubBtn3Text.Text = "";
     SubBtn4Text.Text = "";
     panelSubBtn3.BorderStyle = BorderStyle.None;
     panelSubBtn4.BorderStyle = BorderStyle.None;
     switch (sender)
     {
          case _mainButton1:
              SubBtn1Text.Text = ConfigDPSubmenu._SubMenuBtn1Text;
              SubBtn2Text.Text = ConfigDPSubmenu._SubMenuBtn2Text;
              pictureBoxSubBtn1.Image = Image.FromFile(ConfigDPSubmenu._SubMenuBtn1Img);
              pictureBoxSubBtn2.Image = Image.FromFile(ConfigDPSubmenu._SubMenuBtn2Img);
              panelSubBtn1.BorderStyle = BorderStyle.FixedSingle;
              panelSubBtn2.BorderStyle = BorderStyle.FixedSingle;
              break;
          case _mainButton2:
              SubBtn1Text.Text = ConfigADPSubmenu._SubMenuBtn1Text;
              SubBtn2Text.Text = ConfigADPSubmenu._SubMenuBtn2Text;
              pictureBoxSubBtn1.Image = Image.FromFile(ConfigADPSubmenu._SubMenuBtn1Img);
              pictureBoxSubBtn2.Image = Image.FromFile(ConfigADPSubmenu._SubMenuBtn2Img);
              panelSubBtn1.BorderStyle = BorderStyle.FixedSingle;
              panelSubBtn2.BorderStyle = BorderStyle.FixedSingle;
              break;
     }
}

A couple of points that may help you.

When you see yourself copy and pasting or writing duplicated code, consider refactoring.  I could only see one difference in your code in the case statements.  You could move all that to a method and pass in the necessary struct.  Even further, once you have done this you will see further code duplication you could probably further refactor into seperate methods.
I'm not sure about having exposed fields on the struct start with _.  You might want to read Jon Skeets Answer on stack overflow.
I would probably consider removing your struct and using a single class.  Then I would create 4 instances of that class with the values required.  I might even consider populating the text for those from a resource file.
I'm not really sure on this, but in old versions (Vb 6....) you could create controls which were arrays of each other.  So instead of panelSubBtn1, panelSubBtn2 etc you would just have panelSubBtn and reference it by panelSubBtn[0] etc  If this was possible you could seriously simplify your code.  I'll let you figure that out though...

These are a few quick things which may or may not help.


Functional approach to splitting list into sub lists Here is my functional approach to split a list into sub lists, each having non-decreasing  numbers, so the input

\$[1; 2; 3; 2; 4; 1; 5;]\$

computes to

\$[[1; 2; 3]; [2; 4]; [1; 5]]\$

The code works, but I need a while to understand it.  I would prefer a solution that is more clear. Is there a way to make this more elegant or readable?  Everything is allowed, F# mutables as well. Or do I just get used more to functional code reading?

The subsets are called "run"s in the code below.

// returns a single run and the remainder of the input
let rec takerun input =
    match input with
    | [] -> [], []
    | [x] -> [x], []
    | x :: xr -> if x < xr.Head then
                     let run, remainder = takerun xr
                     x :: run, remainder
                 else [x], xr

// returns the list of all runs
let rec takeruns input =
    match takerun input with
    | run, []  -> [run]
    | run, rem -> run :: takeruns rem

let runs = takeruns [1; 2; 3; 2; 4; 1; 5;]
> val runs : int list list = [[1; 2; 3]; [2; 4]; [1; 5]]

Edit:

Considering the helpful feedback I ended up with this reusable code. And got more used to functional programming, comparing imperative alternatives I meanwhile find the pure functional approach more readable. This version is good readable, although not tail recursive. For the small lists I had to deal with, readability was preferred.

// enhance List module
module List =

    // splits list xs into 2 lists, based on f(xn, xn+1)
    let rec Split f xs =
        match xs with
        | []  -> [],  []
        | [x] -> [x], []
        | x1 :: x2 :: xr when f x1 x2 -> [x1], x2 :: xr // split on first f(xn, xn+1)
        | x :: xr -> let xs1, xs2 = Split f xr
                     x :: xs1, xs2

// Now takruns becomes quite simple
let rec takeruns input =
    match List.Split (>) input with
    | run, []  -> [run]
    | run, rem -> run :: takeruns rem

let runs = takeruns [1; 2; 3; 2; 4; 1; 5;]

Haskell:

import Data.List.HT -- or Data.List.Grouping

takeruns xs@(x:xss) = map (map fst) pss
  where pss = segmentAfter (uncurry (>)) $ zip xs xss

EDIT
Example:

xs = [1, 2, 3, 2, 4, 1, 5]
xss = [2, 3, 2, 4, 1, 5]
zip xs xss = [(1, 2), (2, 3), (3, 2), (2, 4), (4, 1)]
uncurry (>) (1, 2) == 1 > 2 = False
segmentAfter ... = [
    [(1, 2) /* False */, (2, 3) /* False */, (3, 2) /* True */],
    [(2, 4) /* False */, (4, 1) /* True */],
    []
]
map (map fst) (segmentAfter ...) = [[1, 2, 3], [2, 4], []]

And, it turns out that my function is wrong :)
Correct version:

takeruns xs = map (map snd) pss
  where pss = segmentAfter (uncurry (>)) $ zip (minBound:xs) xs


This code is a non-static C# class for rolling a number of dice Hello Everyone!
Fellow human beings, today I present to you my dice rolling script! Behold, in my amateurish attempt at some raw C# code. Perhaps this will remain here regardless of the dozens of other dice rolling scripts out there.
Contains a 'for' loop, a 'switch', and a method that returns a value.
using System;

namespace C_
{
    class RollDice : Program
    {

        //Variables

        int numOfDice;
        int numOfSides;

        int maxDice = 100;
        int minDice = 1;
        int maxSides = 100;
        int minSides = 1;
        int totalRolled;

        //Start method for number of Dice to throw - take the player input in InputMethod() which returns an int. If returning 0, then restart. Clamp the value between min/max. Continue to next function.

        public void GetDice()
        {
            Console.WriteLine("Welcome to Roll Dice!");
            Console.WriteLine("How many dice do you want to throw?");
            numOfDice = InputMethod();
            if(numOfDice == 0)
            {
                GetDice();
            }
            if (numOfDice > maxDice || numOfDice < minDice)
            {
                numOfDice = Math.Clamp(numOfDice, minDice, maxDice);
                Console.WriteLine($"Number of Dice set to {numOfDice}");
            }

            GetSides();
        }

        // Get the sides of each dice by using a similar process. Roll() for each dice, then sum the totals in RollTotal().

        public void GetSides()
        {
            Console.WriteLine("How many sides does each dice have?");
            numOfSides = InputMethod();
            if(numOfSides == 0)
            {
                GetSides();
            }
            if (numOfSides > maxSides || numOfSides < minSides)
            {
                numOfSides = Math.Clamp(numOfSides, minSides, maxSides);
                Console.WriteLine($"Number of Sides set to {numOfSides}");
            }

            for (int x = 1; x <= numOfDice; x++)
            {
                Roll();
            }

            RollTotal();
        }

        // Create a new Random() instance for the number of sides and add it to the totalRolled variable.

         public void Roll()
        {
            Console.WriteLine(".....");
            Console.WriteLine("Rolling");
            Random r = new Random();
            var result = r.Next(1, numOfSides + 1);
            Console.WriteLine($"You rolled {result}");
            totalRolled += result;
        }

        // Output the total and prompt to roll again or quit. Reset the total..

        public void RollTotal()
        {
            Console.WriteLine($"Total sum rolled is {totalRolled}");
            Console.WriteLine(" ");
            Console.WriteLine("Enter 1 to re-roll or 2 to quit.");
            var answer = InputMethod();

            switch (answer)
            {
                case 0:
                RollTotal();
                break;

                case 1:
                totalRolled = 0;
                GetDice();
                break;

                case 2:
                totalRolled = 0;
                Environment.Exit(0);
                break;

                default:
                Environment.Exit(0);
                break;
            }
        }

        //Convert the input to an integer if valid and return the value. Else return 0.

        public int InputMethod()
        {
            var input = Console.ReadLine();
            if(!Int32.TryParse(input, out int convertedStringToInt))

            {
                Console.WriteLine("You need to enter a valid number!");
                return 0;
            }

            else

            {
                return Convert.ToInt32(input);
            }

        }

    }
}

I have few tips:

Infinite recursion.
The app would never finish until you call Environment.Exit because you have infinite recursion. The result of the infinite recursion is always StackOveflowException, sooner or later. Take the trigger: if you forced to use Environment.Exit to stop the app, then something went wrong. About correct usage of the recursion: on some level of calling the recursive method must return not make a recursive call again. Then the recursion would finish normally. Btw, you need no recursion here.

Naming
Give all the items: classes, namespaces, variables, fields, etc. names that explains its purpose or containing value. It would make the code maintaneable. Also I use the following standard.
private int _fieldName;

public int PropertyName { get; set; }

public void DoSomething()
{
    int localVariableName = 0;
}

It allows explicitly differ fields and local variables in a code even if I'm reading the code outside of IDE editor.

Incapsulation.
The OOP principle that helps you in development to avoid popular mistakes. What method should I call when I create an instance of RollDice class? All the methods are public. Leave public only that class components which you allow to access outside of the class.

Small tips.
Remove inheritance class RollDice : Program, it does nothing. Let the methods and classes do some single job. (see S in SOLID principles for details).

Here's some alternative implementation of the same to play.
class Program
{
    static void Main(string[] args)
    {
        RollDice dice = new RollDice();
        dice.Run();
        Console.ReadKey();
    }
}

public class RollDice
{
    private const int maxDice = 100;
    private const int minDice = 1;
    private const int maxSides = 100;
    private const int minSides = 2;

    private Dice[] _dices;

    private List<string> _menuItems = new List<string>
    {
        "Exit.",
        "Setup Roll Dice.",
        "Roll!"
    };

    public void Run()
    {
        Console.Write("Welcome to Roll Dice!");
        while (true)
        {
            ShowMenu(_menuItems);
            switch (ConsoleHelper.ReadInteger("Enter your selection: ", 0, _menuItems.Count - 1))
            {
                case 1:
                    Setup();
                    break;
                case 2:
                    Roll();
                    break;
                case 0:
                    Console.WriteLine("Bye.");
                    return;
                default:
                    Console.WriteLine("Error: Unexpected menu input!"); // this would never happen
                    break;
            }
        }
    }

    private void Roll()
    {
        if (_dices == null)
        {
            Console.WriteLine("Please setup the Roll Dice first.");
            return;
        }

        Console.WriteLine("Rolling...");
        for (int i = 0; i < _dices.Length; i++)
        {
            _dices[i].Roll();
        }
        Console.WriteLine($"You rolled: {string.Join(", ", (object[])_dices)}");

        int total = 0;
        for (int i = 0; i < _dices.Length; i++)
        {
            total += _dices[i].Value;
        }
        Console.WriteLine($"Total sum rolled is {total}");
    }

    private void Setup()
    {
        int diceCount = ConsoleHelper.ReadInteger("How many dice do you want to throw? ", minDice, maxDice);
        int sides = ConsoleHelper.ReadInteger("How many sides does each dice have? ", minSides, maxSides);
        _menuItems[1] = $"Change Roll Dice setup. (currently {diceCount} dice of {sides} sides).";
        _dices = CreateDice(diceCount, sides);
    }

    private Dice[] CreateDice(int diceCount, int sides)
    {
        Dice[] dices = new Dice[diceCount];
        for (int i = 0; i < diceCount; i++)
        {
            dices[i] = new Dice(sides);
        }
        return dices;
    }

    private void ShowMenu(List<string> items)
    {
        Console.WriteLine();
        for (int i = 1; i < items.Count; i++)
            Console.WriteLine($"[{i}] {items[i]}");
        Console.WriteLine($"[0] {items[0]}");
    }
}

public class Dice
{
    private static Random rndGen = new Random(); // one random generator for all dices would be enough

    private int _sides;

    public int Value { get; private set; }

    public Dice(int sides)
    {
        _sides = sides;
    }

    public void Roll()
    {
        Value = rndGen.Next(0, _sides) + 1;
    }

    public override string ToString()
    {
        return $"[{Value}]";
    }
}

public static class ConsoleHelper
{
    public static int ReadInteger(string prompt, int min, int max)
    {
        while (true)
        {
            Console.Write(prompt);
            if (int.TryParse(Console.ReadLine(), out int option) && option >= min && option <= max)
                return option;
            Console.WriteLine($"You need to enter a valid number from {min} to {max}!");
        }
    }
}

Note for other reviewers: I'm not including const or readonly warnings in the review to leave it as simple as possible.
No recursion above.


Zebra puzzle in Prolog I have attempted to a zebra puzzle in prolog and I was seeking some feedback as to the way I went about solving the puzzle.

Here is the puzzle:

  Two weeks ago, four enthusiasts made sightings of objects in the sky
  in their    neighborhood. Each of the four reported his or her
  sightings on a different day. The FBI came and was able to give each
  person a different explanation of what he or she had "really" seen.
  Can you determine the day (Tuesday through Friday) each person sighted
  the object, as well as the object that it turned out to be?

  Mr. K made his sighting at some point earlier in the week than the one who saw the balloon, but at some point later in the week, than the
  one who spotted the Kite (who isn't Ms. G).
  Friday's sighting was made by either Ms. Barn or the one who saw a plane (or both).
  Mr. Nik did not make his sighting on Tuesday.
  Mr. K isn't the one whose object turned out to be a telephone pole.

Here is a representation of how I found the solution to the problem:

I know K didn't spot the balloon (1) or the kite (2) or the telephone pole (6), so K spotted the plane. Also, K's day can't be Friday (1), so Friday was B's day (4).

I also know the kite wasn't spotted by K (2) or G (3). Nor was it spotted by B, since B's day was Friday and the kite can't have been spotted on Friday (2). So N spotted the kite. Also, we know kite day is two days before balloon day (1,2), so kite day must have been Tuesday or Wednesday; but N's day wasn't Tuesday (5), so kite day was Wednesday, which means K's day was Thursday (2) and balloon day was Friday (1).

And the rest I got by elimination. Summing up:

G spotted a telephone pole on Tuesday.
N spotted a kite on Wednesday.
K spotted a plane on Thursday.
B spotted a balloon on Friday.

And here is my prolog code:

before(X,Y,Ds) :-
    remainder(X,Ds,Rs),
    member(Y,Rs).

remainder(X,[X|Ds],Ds).
remainder(X,[_|Ds],Rs) :- remainder(X,Ds,Rs).

members([],_).
members([X|Xs],Ds) :-
member(X,Ds),
members(Xs,Ds).

puzzle :-
Days = [[tuesday,_,_],[wednesday,_,_],[thursday,_,_],[friday,_,_]],
before([_,mr_klien,_],[_,_,balloon],Days),
before([_,_,frisbee],[_,mr_klien,_],Days),
(member([friday,ms_barnum,_],Days);
    member([friday,_,clothesline],Days);
    member([friday,ms_barnum,clothesline],Days)),
members([[_,mr_klien,_],[_,ms_barnum,_],[_,ms_green,_],[_,mr_niven,_]],Days),
members([[_,_,balloon],[_,_,frisbee],[_,_,clothesline], [_,_,water_tower]],Days),
member([_,NOT_ms_green,frisbee],Days), NOT_ms_green \= ms_green,
member([tuesday,NOT_mr_niven,_],Days), NOT_mr_niven \= mr_niven,
member([_,NOT_mr_klien,water_tower],Days), NOT_mr_klien \= mr_klien,
write(Days),
nl,
fail.

This is quite nice, but there are still several ways that would improve this.

Here are a  few ideas:

Relations instead of side-effects

First, I recommend to avoid side-effects. Instead, think in terms of relations. By this I mean that you should not use write/1 to print a solution, but express what constitutes a solution, as in the following:

solution(Days) :-
        Days = [[tuesday,_,_],[wednesday,_,_],[thursday,_,_],[friday,_,_]],
        etc.

Here, I have made Days available for reasoning as a predicate argument. This way, you can explicitly reason about solutions, and for example write actual test cases by stating what should and should not hold about given solutions. You cannot do this if the solution only occurs in the form of output on the system terminal.

If you need to print a solution in any way, you can use the more general form above to still do that. For example:

?- solution(Ds),
   maplist(writeln, Ds).
[tuesday,ms_green,water_tower]
[wednesday,mr_niven,frisbee]
[thursday,mr_klien,clothesline]
[friday,ms_barnum,balloon]

Use higher-order predicates

Suppose I give you the following auxiliary predicate:

list_member(Ls, E) :- member(E, Ls).

Then you can use maplist/2 to express members/2 as follows:

members(Es, Ls) :- maplist(list_member(Ls), Es).

When you encounter predicates that reason over lists, check the available higher-order predicates (especially maplist/N and foldl/N) to see if they can simplify your code.

Use dif/2 instead of (\=)/2

You are currently using the impure predicate (\=)/2 in your code. This prevents you from using Prolog to its fullest potential.

Use dif/2 instead to benefit from the declarative nature of true relations: You can then pull these goals towards the beginning of the clause or at least earlier, and benefit from their pruning in addition to the added flexibility of placing the goal anywhere you like in principle.

Indentation

Never put (;)/2 at the end of a line. It looks too similar to (',')/2, which typically occurs at that position.

In total, a solution could look like (in addition to the points mentioned previously):

solution(Days) :-
        Days = [[tuesday,_,_],[wednesday,_,_],[thursday,_,_],[friday,_,_]],
        before([_,mr_klien,_],[_,_,balloon],Days),
        before([_,_,frisbee],[_,mr_klien,_],Days),
        (   member([friday,ms_barnum,_],Days)
        ;   member([friday,_,clothesline],Days)
        ;   member([friday,ms_barnum,clothesline],Days)
        ),
        members([[_,mr_klien,_],[_,ms_barnum,_],[_,ms_green,_],[_,mr_niven,_]], Days),
        members([[_,_,balloon],[_,_,frisbee],[_,_,clothesline],[_,_,water_tower]], Days),
        dif(NOT_ms_green, ms_green),
        member([_,NOT_ms_green,frisbee],Days),
        dif(NOT_mr_niven, mr_niven),
        member([tuesday,NOT_mr_niven,_],Days),
        dif(NOT_mr_klien, mr_klien),
        member([_,NOT_mr_klien,water_tower],Days).

Now we can query:

?- solution(Ds).
Ds = [[tuesday, ms_green, water_tower], [wednesday, mr_niven, frisbee], [thursday, mr_klien, clothesline], [friday, ms_barnum, balloon]] ;
false.

And also ask something different, for example:

?- Ds = [[monday, ms_green|_]|_],
   solution(Ds).
false.

Thus, the system tells us: No, that's not a solution!

This is now easily possible because we can reason explicitly about solutions, having made them available as a predicate argument.


Capping values in a map while keeping one fixed I'm working with some Clojurescript code that reacts to user input over three sliders. These sliders get stored in a map, and I need to make sure that the sum of all is <= a max value. Every time a value from the map is modified I have the guarantee that it will be in the [0, max] range, and I must reduce if necessary the others so that the total sum is still <= max.

I implemented it, but as I don't have a lot of experience with Clojure and Clojurescript I'm sure it can be improved. I would like to listen to any comment about making it shorter, more concise, more idiomatic, easier to read, etc. Thanks!

PS: I know that the names of the functions don't make much sense, I'm still in mid development and changing things all the time. alloc stands for allocation.

(def MaxAlloc 100)

(defn verify-alloc
  [alloc fixed-id]
  (let [total (reduce + 0 (vals alloc))]
    (if (<= total MaxAlloc)
      alloc
      (let [others (dissoc alloc fixed-id)]
        (assoc
          (de-alloc (dissoc alloc fixed-id) (- total MaxAlloc))
          fixed-id (get alloc fixed-id)) ) ) ) )

verify-alloc checks if there is an excess, and if so removes the fixed element from the map and calls de-alloc to remove the excess.

(defn de-alloc
  [alloc excess]
  (let [pair (first alloc)
        k (first pair)
        v (second pair)]
    (if (> v excess)
      (assoc alloc k (- v excess))
      (assoc (de-alloc (dissoc alloc k) (- excess v)) k 0) ) ) )

de-alloc works by removing as much excess as it can from the first value in the map, and if there is still some excess value, calls itself recursively removing the 'zeroed' element from the map.

And it would be used like this:

(verify-alloc {:a 0 :b 30 :c 40} :c)
{:a 0, :b 30, :c 40}

(verify-alloc {:a 10 :b 40 :c 70} :a)
{:a 10, :b 20, :c 70}

(verify-alloc {:a 90 :b 40 :c 70} :b)
{:b 40, :a 0, :c 60}

I would also like to change the algorithm so that instead of completely decreasing elements one after the other, it would distribute this decrease in equal parts among the remaining elements. But I could not find my way around it. Ideas?

Your verify-alloc function is badly named: it doesn't verify, it normalises.

A normalisation function that preserves the proportions of the other values and limits the sum to MaxAlloc is

(defn normalise [m k]
  (let [remains (dissoc m k)
        allowance (- MaxAlloc (m k))
        weight (->> remains
                    vals
                    (apply +))
        factor (/ allowance weight)]
    (if (<= weight allowance)
      m
      (merge m (zipmap (keys remains) (map #(* factor %) (vals remains)))))))

This is written to be concise, not to be fast.

If there'd nothing to be done, everything is hunky-dory:

(normalise {:a 0 :b 30 :c 40} :c)
;{:a 0, :b 30, :c 40}

But if the values need scaling, we can run into problems with arithmetic:

(normalise {:a 90 :b 40 :c 70} :b)
;{:a 135/4, :b 40, :c 105/4}

If you want these as integers, you may have to forego some precision. How you do this is up to you.


What is the best way to format a large has_many through line? This question is about code style.

I have this line in one of my models:

has_many :owning_artists, :through => :artist_tracks, :source => :artist, :conditions => { :artist_tracks => { :artistic_role_id => 1, :another_memeber => 42 } }

What is the best way to break this up on multiple lines, and indented of course, and what would it would look like?

I'd love to hear a more general recommendation/guideline for how to break up and indent nested hashes too.

This is what I would do:

has_many :owning_artists, {
  :through => :artist_tracks,
  :source => :artist,
  :conditions => {
    :artist_tracks => {
      :artistic_role_id => 1,
      :another_memeber => 42
    }
  }
}


Replacing spaces with tabs. Exercise 1-21 in "The C Programming Language" I'm looking for feedback on the code I came up with for exercise 1-21 in "The C Programming Language" by K&R.

  Write a program entab that replaces strings of blanks with the minimum number of tabs and blanks to achieve the same spacing. Use the same stops as for detab . When either a tab or a single blank would suffice to reach a tab stop, which should be given preference?

The feedback I would mainly like is on readability, conventions and general good (or bad) practices.

This was a fun problem to solve and I think I came up with a efficient solution.

#include <stdio.h>

#define TABSIZE 8
#define MAXLENGTH 80

void entab(char s[]);
int _getline(char s[], int lim);

int main()
{
    int len;
    char line[MAXLENGTH];

    while ((len = _getline(line, MAXLENGTH)) > 0)
        entab(line);
}

void entab(char s[])
{
    int i, c, nb;

    nb = 0;

    for (i = 0; (c = s[i]) != '\0'; ++i)
    {
        if (c == ' ')
        {
            if (++nb > 1)
            {
                if (i + 1 % TABSIZE == 0)
                {
                    putchar('\t');
                    nb = 0;
                }
            }
        }

        else
        {
            if (nb > 0)
            {
                for (; nb > 0; --nb)
                    putchar('x');
            }

            putchar(c);
        }
    }
}

int _getline(char s[], int lim)
{
    int c, i;

    for (i = 0; i < lim - 1 && (c = getchar()) != EOF && c != '\n'; i++)
        s[i] = c;
    if (c == '\n')
    {
        s[i] = c;
        ++i;
    }
    s[i] = '\0';

    return i;
}

The function _getline() was given by the book.
Only practices encountered in the book up until this assignment are used, I have not yet read about things like pointers and other more advanced features

My own thoughts

Use (more) comments when coding
Use more "self-explaining" variable names
Input longer than 80 characters isn't handled correctly

Welcome to Code Review! I'm always glad to see someone who reads the classics.

You made some interesting observations about the program.

The statement Use more "self-explaining" variable names is absolutely correct: the variable name nb could definitely be improved. I would like to point out that self-documenting might be better than self-explaining.

If the variable and symbolic constant names are improved it may not need any additional comments.

The header file <stdio.h> includes a symbolic constant BUFSIZ that might be a better length than 80. In some cases BUFSIZ may be the maximum line length for the system; this was true on older Unix systems.

Possible Bug

This code seems out of scope for the problem as it is defined, I would expect to see that it was outputing blanks.

            for (; blankCount > 0; --blankCount)
                putchar('x');

A Good Coding Practice

Code needs to be maintained. This may include adding additional lines to control structures such as if statements and loops. In C and C++ a good programming practice is to have code blocks (complex statements) in all if, else and loop statements even if it isn't currently necessary.

if (CONDITION)
{
    one statement
}
else
{
    one statement
}

while (CONDITION)
{
    one statement
}

Alternate Solution with Simplified Functions

It might be easier to read, write and modify entab() if it called a function to count all the blanks and print the necessary tabs and blanks. Programming in many cases is breaking down a problem into smaller and smaller pieces until each piece is easy to implement. While this may make the entire program a little more complex, each function is simplified.

This example uses a concept you haven't gotten to in the book yet called pointers.

void print_tabs_or_spaces(int tab_count, int out_value)
{
    for (int i = 0; i < tab_count; i++)
    {
        putchar(out_value);
    }
}

char* count_blanks_and_output_tabs_and_spaces(char *c)
{
    int blank_count = 0;

    while (*c == ' ')
    {
        ++blank_count;
        c++;
    }
    int tab_count = blank_count / TABSIZE;
    int space_count = blank_count % TABSIZE;

    print_tabs_or_spaces(tab_count, '\t');
    print_tabs_or_spaces(space_count, ' ');

    return c;
}

void entab(char str[])
{
    char *c = &str[0];
    while(*c != '\0')
    {
        if (*c == ' ')
        {
            c = count_blanks_and_output_tabs_and_spaces(c);
        }
        else
        {
            putchar(*c);
            c++;
        }
    }
}


Function that loads a random image using an array of paths to images I have created a program that generates a random image from a folder of all the world's flags. First, it generates an image as it loads, then it can keep randomizing the shown image if the user clicks on a button. Currently, I have created a method called "ImageGenerator" and call that method both in Form1() and my button click method (view code: https://pastebin.com/qzWXqP0P).

public partial class Form1 : Form
{
    public void ImageGenerator()
    {

        string[] flagImageSources = new string[196];

        StreamReader flagImageSourceReader = File.OpenText("flagImageSourceFile.csv");

        for (int i = 0; i < flagImageSources.Length; i++)
        {
            string s = flagImageSourceReader.ReadLine();
            flagImageSources[i] = s;
        }

        Random numberGenerator = new Random();
        int imageNumber = numberGenerator.Next(0, 196);

        string flagImageSrc = flagImageSources[imageNumber];

        flagPictureBox.ImageLocation = @flagImageSrc;
    }

    public Form1()
    {
        InitializeComponent();

        ImageGenerator();
    }

    private void submitBtn_Click(object sender, EventArgs e)
    {
        ImageGenerator();
    }
}

To me it feels like the linked code is rather inefficient as it recreates the array flagImageSources (which contains the paths to each flag image) every time a new image is to be generated in the picture box. How can I run the code on lines 6 through 13 only once, and then just run the code on lines 16 through 21 when randomizing images?

To answer your immediate question, you need to initialise your flagImageSources once, and store it as a field in the class. That allows you to split the two actions into separate methods - one to get your list of flags, and one to get a random flag.

public partial class Form1 : Form
{
    private string[] flagImageSources;

    private void InitializeImageSources()
    {
        flagImageSources = new string[196];
        StreamReader flagImageSourceReader = File.OpenText("flagImageSourceFile.csv");

        for (int i = 0; i < flagImageSources.Length; i++)
        {
            string s = flagImageSourceReader.ReadLine();
            flagImageSources[i] = s;
        }
    }

    private string GetRandomImageLocation()
    {
        Random numberGenerator = new Random();
        int imageNumber = numberGenerator.Next(0, 196);

        return flagImageSources[imageNumber];
    }
}

But I think we have some other concerns here.

Firstly, you open your file but forget to close it again. C# has a handy way of making sure this doesn't happen - something that gets opened and closed should implement IDisposable (and StreamReader does just that). So you can change your code a little to make use of that, like this:

        using (StreamReader flagImageSourceReader = File.OpenText("flagImageSourceFile.csv"))
        {
            //do stuff with flagImageSourceReader
        }

Secondly, you're using the magic number of 196 in a couple of places in your code - both to set the initial size of your array, and to set the upper bound of your random number generator. I would suggest that rather than using a fixed-size array, you should use an IList, which is dynamically sized. Then, if North and South Korea should merge, or Scotland or Catalonia or Tibet become independent countries, your code will still work even as you add or remove flags :)

Here is my final code that solves both of those problems:

public partial class Form1 : Form
{
    private IList<string> flagImageSources = new List<string>();

    private int NumberOfFlags => flagImageSources.Count;

    public Form1()
    {
        InitializeComponent();

        InitializeImageSources();
        flagPictureBox.ImageLocation = GetRandomImageLocation();
    }

    private void InitializeImageSources()
    {
        using (StreamReader flagImageSourceReader = File.OpenText("flagImageSourceFile.csv"))
        {
            while (!flagImageSourceReader.EndOfStream)
            {
                flagImageSources.Add(flagImageSourceReader.ReadLine());
            }
        }
    }

    private string GetRandomImageLocation()
    {
        Random numberGenerator = new Random();
        int imageNumber = numberGenerator.Next(0, NumberOfFlags);

        return flagImageSources[imageNumber];
    }

    private void submitBtn_Click(object sender, EventArgs e)
    {
        flagPictureBox.ImageLocation = GetRandomImageLocation();
    }
}


Reading lines from a file in random order I originally wrote this as an answer to a question on Stack Overflow, but it turned out so nicely that I decided to post it here to see if I can make it even better.

(defn char-seq
  "Returns a lazy sequence of characters from rdr. rdr must implement
  java.io.Reader."
  [rdr]
  (let [c (.read rdr)]
    (if-not (neg? c)
      (cons (char c) (lazy-seq (char-seq rdr))))))

(defn line-offsets
  "Returns a lazy sequence of offsets of all lines in s."
  [s]
  (if (seq s)
    (->> (partition-all 3 1 s)
         (map-indexed
          (fn [i [a b c]]
            (cond
              (= b \newline) (if c (+ 2 i))
              (= a \return) (if b (inc i)))))
         (filter identity)
         (cons 0))))

(defn ordered-line-seq
  "Returns the lines of text from raf at each offset in offsets as a lazy
  sequence of strings. raf must implement java.io.RandomAccessFile."
  [raf offsets]
  (map (fn [i]
         (.seek raf i)
         (.readLine raf))
       offsets))

Example usage:

(let [filename "data.txt"
      offsets (with-open [rdr (clojure.java.io/reader filename)]
                (shuffle (line-offsets (char-seq rdr))))]
  (with-open [raf (java.io.RandomAccessFile. filename "r")]
    (run! println (ordered-line-seq raf offsets))))

I'm interested in any way to improve this code, but here are some specific things I'm looking for:

Since RandomAccessFile obviously uses byte offsets, and line-offsets returns character offsets, this code won't work for Unicode files. Is there a way to fix that problem without adding a lot of complexity?
Are a, b, and c good names for the parameters of the anonymous function in line-offsets?
What exactly are the guidelines for parameter ordering in named functions like ordered-line-seq?

Regarding your questions:

You're reading bytes.  Fortunately, the only three line ending
conventions all involve Newline and Return, both a single byte.
There's nothing more to it.  So if you stay with an encoding using
single bytes for (most of) their characters, such as UTF-8, there will
be no problem.  For other Unicode encodings you'd have to read more
than one byte per character and also do some decoding.

Frankly even without encodings this is a hairy topic,
c.f. Newline on Wikipedia.
Though line-offsets uses the same convention as readLine, so I
guess that's just FYI.
I'd at least not use short/one-letter variable names for function
parameters, because they also serve as documentation.  E.g. r
doesn't tell the reader anything, rdr is barely english, but
reader is clear and doesn't cost that much more in terms of
characters.  Same with s and sequence.  For a, b, c in
line-offsets I'd even say that c1 to c3 would be more helpful.
It'd be best to orient yourself at standard functions.  It's probably
better to have the most important "subject" first, then subsequently
the rest unless there's a good reason not to.  Parameters at the ends
can also very easily be curried, maybe that's another hint to think
about it.

Other than that, yeah, looks good.  I don't quite see why the laziness
is necessary, since it's calling shuffle anyway, but perhaps it's
useful in some situations?


Flatten iterator for nested list Given a list which can can contain elements as well as lists, write an iterator to flatten a nested list. Please make this code better and suggest any improvements.

public class FlattenIterator implements Iterator {

    private final Stack<Iterator> iteratorStack;

    private Object next;

    public FlattenIterator(List list) {
        if (list == null) {
            throw new NullPointerException();
        }

        iteratorStack = new Stack<Iterator>();
        iteratorStack.push(list.iterator());
    }

    public void remove() {
        /* Not implemented */
    }

    private void moveToNext() {
        if ((next == null) && !iteratorStack.empty()) {
            if (iteratorStack.peek().hasNext()) {
                final Object next = iteratorStack.peek().next();
                if (next instanceof Iterator) {
                    iteratorStack.push((Iterator) next);
                    moveToNext();
                } else if (next instanceof Iterable) {
                    iteratorStack.push(((Iterable) next).iterator());
                    moveToNext();
                } else if (next instanceof Array) {
                    iteratorStack.push(Arrays.asList((Array) next).iterator());
                    moveToNext();
                } else {
                    this.next = next;
                }
            } else {
                iteratorStack.pop();
                moveToNext();
            }
        }
    }

    public boolean hasNext() {
        moveToNext();
        return next != null;
    }

    public Object next() {
        moveToNext();
        if (next == null) {
            throw new NoSuchElementException();
        } else {
            Object objectToReturn = next;
            next = null;
            return objectToReturn;
        }
    }
}

You should probably review: com.google.common.collect.Iterators, especially com.google.common.collect.Iterators.concat() (com.google.guava:guava).  That implementation uses a member variable to hold and track the current iterator, in addition to the container of ordered iterators to inspect.

If you initialize current iterator with an empty iterator value (again, see guava), you'll eliminate some problem edge cases.

moveToNext() should not use recursion.  Instead, it ought to be a while loop that is determining the correct current iterator to use, and then returning the status of that iterator.  After all, you've already got a Stack, why do you need to use the stack also?

Take a good look at that constructor again - you happen to be getting an iterator from List.iterator(), but there's no reason at all to care if it came from List, or some other collection.

private final Stack<Iterator> iteratorStack = new Stack<Iterator>();
public FlattenIterator(List list) { this(list.iterator);  }
public FlattenIterator(Iterator iterator) { iteratorStack.push(iterator); }

The Dependency Injection pattern would perhaps suggest that you should have a constructor that accepts a Stack<Iterator>, and factory methods that create the Stack from the initial source iterator.  Not immediately clear - here, the stack is an implementation detail, so maybe it's not important.  On the other hand, if you can imagine sometimes swapping in a different implementation, that will run extra logging/instrumentation/debug code, then maybe you'll want to have that constructor.

Why not FlattenIterator<T> extends Iterator<T>?  Yes, the source is going to be mixed type, but you don't need to propagate that forward forever.

You might consider whether List is actually the right idea; here, you are using List<Object> as a poor man's Composite<T> (see Design Patterns, Gamma et al).  What you are really doing, after all, is visiting all leaves of a tree, so maybe the tree should be explicit, rather than implicit in the list.  If you have control over the caller, that's a design choice you might consider walking backwards.

It's not clear if remove() should be a no-op, as it is here, or throw an UnsupportedOperationException.  At a minumum, there should be a comment explaining why it doesn't throw.


Using IQueryable.GroupBy to count related database entities Given a DbSet<Client> from an Entity Framework 6 DbContext, I need to count the number of related Tickets of each type: Open, Responded, and Resolved.

The below code works, and results in only one query (according to DbContext.Database.Log), as desired. However, the fact that the call to GroupBy results in an IGrouping<Client, ICollection<Ticket>>, which is also an IEnumerable<ICollection<Ticket>>, requires me to call FirstOrDefault(), even though there is always only one ICollection<Ticket>.

I've never used GroupBy before, so I'm willing to bet I'm just misusing it.

public IEnumerable<DashboardItemViewModel> GetItems(IQueryable<Client> clients)
{
    return clients
        .Where(cli => cli.IsActive)
        .GroupBy(cli => cli, cli => cli.Tickets)
        .Select(grp => new DashboardItemViewModel
        {
            Id = grp.Key.Id,
            Name = grp.Key.Name,
            Open = grp.FirstOrDefault().Count(t => t.Status == StatusType.Open && !t.IsArchived),
            Responded = grp.FirstOrDefault().Count(t => t.Status == StatusType.Responded && !t.IsArchived),
            Resolved = grp.FirstOrDefault().Count(t => t.Status == StatusType.Resolved && !t.IsArchived),
        });
}

public enum StatusType
{
    Open,
    Responded,
    Resolved,
};

public class DashboardItemViewModel
{
    public int Id { get; set; }
    public string Name { get; set; }
    public int Open { get; set; }
    public int Responded { get; set; }
    public int Resolved { get; set; }
}

public class Client
{
    public int Id { get; set; }
    public string Name { get; set; }
    public bool IsActive { get; set; }
    public virtual ICollection<Ticket> Tickets { get; set; }
}

public class Ticket
{
    public int Id { get; set; }
    public StatusType Status { get; set; }
    public bool IsArchived { get; set; }
    public virtual Client Client { get; set; }
}

You don't need the grouping at the Client level. No grouping at all, actually. Don't worry, this is a common mistake when people want grouping/aggregation in child collections. Per client, the Tickets can be counted:

return clients
    .Where(cli => cli.IsActive)
    .Select(cli => new DashboardItemViewModel
    {
        Id = cli.Id,
        Name = cli.Name,
        Open = cli.Tickets.Count(t => t.Status == StatusType.Open && !t.IsArchived),
        Responded = cli.Tickets.Count(t => t.Status == StatusType.Responded && !t.IsArchived),
        Resolved = cli.Tickets.Count(t => t.Status == StatusType.Resolved && !t.IsArchived),
    });

Or in query syntax, so we can benefit from the let statement:

return from cli in clients
    where cli.IsActive
    let activeTickets = cli.Tickets.Where(t => !t.IsArchived)
    select new DashboardItemViewModel
    {
        Id = cli.Id,
        Name = cli.Name,
        Open = activeTickets.Count(t => t.Status == StatusType.Open),
        Responded = activeTickets.Count(t => t.Status == StatusType.Responded),
        Resolved = activeTickets.Count(t => t.Status == StatusType.Resolved)
    };


Reading binary files in XTF format I have a few thousand binary files that have corresponding structs in them, followed by any number of bytes (this exact number of bytes is given in one of the fields in the structs). I read in data from the binary files to a struct, and assign variables from certain fields in those structs. I was wondering if there is any significant improvement I could make to speed things up, specifically regarding the binary file reading?

Note: I'm reading in 256 bytes at a time to a struct, and in that struct is a number that says how many bytes follow until the next struct. So there isn't a static pattern to the data that I can follow.

const int STRUCT_HEADER_SIZE = 256; //256 bytes
const int FILE_HEADER_SIZE = 1024; //1024 bytes

[StructLayout(LayoutKind.Sequential, Pack = 1, CharSet = CharSet.Ansi)]
public struct STRUCTHEADER
{
    //Sample fields
    public ushort MagicNumber;
    public byte SubChannelNumber;
    public ushort NumChansToFollow;

    [MarshalAs(UnmanagedType.ByValArray, SizeConst = 2)]
    public ushort[] Reserved1;

    public int NumBytesThisRecord; //This value says how many bytes there are total in the current 'packet' which includes this struct header.

    public ushort Year;
    public byte Month;
    public byte Day;
    public byte Hour;
}

FileStream stream1;
STRUCTHEADER testStruct = new STRUCTHEADER();
List<string> filePaths = new List<string>();

foreach (string filePath in filePaths) //for each binary file
{
    ReadBinaryFile(filePath); //Read the binary file
}

public void ReadBinaryFile(string filePath)
{
    try
    {
        stream1 = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.None);
    }
    catch(Exception ex)
    {
    }

    try
    {
        stream1.Position = FILE_HEADER_SIZE; //Start stream after the file header

        while (stream1.Position < stream1.Length) //While not at the end of the stream
        {
            testStruct = ReadRecFromStream<STRUCTHEADER>(stream1); //read data from binary file into STRUCTHEADER type struct

            //assigning fields here
            //ex: int year = testStruct.year;

            stream1.Position += Math.Abs(testStruct.NumBytesThisRecord - STRUCT_HEADER_SIZE); //Advance to the end of current packet
        }
        stream1.Close();
        //Adding field values to a textbox
        //ex: textbox1.Text += year;
    }
    catch
    {
        stream1.Close();
    }
}

//Used to read in data in binary file to a struct
private T ReadRecFromStream<T>(Stream stream) where T : struct
{
    byte[] buffer = new byte[Marshal.SizeOf(typeof(T))];
    stream.Read(buffer, 0, STRUCT_HEADER_SIZE);
    GCHandle handle = GCHandle.Alloc(buffer, GCHandleType.Pinned);
    try
    {
        return (T)Marshal.PtrToStructure(handle.AddrOfPinnedObject(), typeof(T));
    }
    finally
    {
        handle.Free();
    }
}

Here's a link to two sample .xtf binary files (they are the same type I'm reading in):
https://sourceforge.net/projects/imagejforxtf/files/?source=navbar

An alternative solution could be a binary reader (see also: this code project article)

Implementation: (I just replaced the 2 try catch blocks with one using):

public static void ReadBinaryReader(string filePath)
{
    using (var stream1 = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.None))
    {
        stream1.Position = FILE_HEADER_SIZE; //Start stream after the file header

        using (var reader = new BinaryReader(stream1))
        {
            while (stream1.Position < stream1.Length) //While not at the end of the stream
            {
                var testStruct = FromReader(reader); //read data from binary file into STRUCTHEADER type struct
                stream1.Position += Math.Abs(testStruct.NumBytesThisRecord - STRUCT_HEADER_SIZE); //Advance to the end of current packet
            }
        }
    }
}

private static STRUCTHEADER FromReader(BinaryReader reader)
{
    STRUCTHEADER result = new STRUCTHEADER();
    result.MagicNumber = reader.ReadUInt16();
    result.SubChannelNumber = reader.ReadByte();
    result.NumChansToFollow = reader.ReadUInt16();
    result.Reserved1 = new []
    {
        reader.ReadUInt16(),
        reader.ReadUInt16(),
    };
    result.NumBytesThisRecord = reader.ReadInt32();
    result.Year = reader.ReadUInt16();
    result.Month = reader.ReadByte();
    result.Day = reader.ReadByte();
    result.Hour = reader.ReadByte();
    return result;
}

It is a little bis faster, but not significat... However, if you have to use a binary reader for reading the other content, maybe it is a reasonable alternative to PtrToStructure.

Reading the 6 mb file 60.000 times takes:

  PtrToStructure

  1) 3791 ms

  2) 3807 ms

  3) 3794 ms

  BinaryReader

  1) 3650 ms

  2) 3645 ms

  3) 3668 ms


Simple trie implementation in JavaScript After experimenting with a sorted trie implementation in C, I felt that I understood tries pretty well, but was having trouble explaining how they work. Since the C code was based on existing code, I wanted to try it again in another language, from scratch.

Here's what I came up with. I tried to make it as minimal as possible. These tries aren't sorted, and nodes only reference a child and next node; parent and prev aren't used. I did give the "leaves" of the trie a name property, which is the full key. This isn't currently used, but it's useful for debugging, and it would be useful if a trie iterator were added (like Trie_dump from the question linked above).

I'm interested in feedback on the code itself this time, but not so much on things like white space, braces, semicolons, etc. I'm comfortable with the formatting the way it is. I'd be interested in an alternative to that labeled loop, though, and all other feedback is welcome.

Trie structure

Each node in the trie can have a key, a value, a next node, and a child node. In this diagram, each node is represented by a circle. The "root" node is at the left, the green "leaf" nodes are at the right, and the "branch" nodes are in the middle.

Each key is a single character (circled letters below). The "branches" have keys, while each "leaf" holds a value.

In the diagram, nodes appear to have multiple children. For example, the first "r" node has "u" and "o" children. Internally, nodes just have a child property; the first child. The next child is available in the first child's next property, and so on. It may help to think of child as "first child" and next as "next sibling."

trie.js

function Trie(parent, prev, key, value) {
    if (key !== void 0)
        this.key = key;      // single-character key
    if (value !== void 0)
        this.value = value;  // user-defined value
    if (prev)
        prev.next = this;    // next sibling node
    else if (parent)
        parent.child = this; // first child node
}

// put a key/value pair in the trie
Trie.prototype.put = function(name, value) {
    var i = 0, t = this, len = name.length, prev, parent;
    down: while (t.child) {
        parent = t;
        t = t.child;
        // if first child didn't match, get next sibling
        while (t.key != name[i]) {
            if (!t.next) {
                prev = t;
                t = parent;
                break down;
            }
            t = t.next;
        }
        // key already exists, update the value
        if (++i > len) {
            t.value = value;
            return;
        }
    }
    // found any existing parts of the key, add the rest
    t = new this.constructor(t, prev, name[i]);
    while (++i <= len)
        t = new this.constructor(t, null, name[i]);
    t.name = name;
    t.value = value;
};

// get a value from the trie at the given key
Trie.prototype.get = function(name) {
    var i = 0, t = this.child, len = name.length;
    while (t) {
        if (t.key == name[i]) {
            if (i == len)
                return t.value;
            t = t.child;
            ++i;
        } else {
            t = t.next;
        }
    }
};

Demos

Here are links to a few demos. The simple demo puts some test data in a trie, gets it back out, and dumps it to the console. The fancy demo puts the same test data in a trie and draws a diagram like the one above.

Simple demo: http://jsfiddle.net/4Yttq

Fancy demo: http://jsfiddle.net/4Yttq/1/

Well, first of all, my optimization is an overhaul of your method. The concept is still tries. But I went for the JS route for several reasons:

We're in JavaScript. Linked lists in C would be like Arrays/Objects in JS. Since we have them available, why not use them instead?
In tries, you use a key for each entry. You'd have to loop over and find it when using linked lists or arrays. In JS, we have objects that are key-value pairs, which make it a bit easier to implement.
Performance is a bit better, except on Opera browsers. Performance may be due to the lesser loops and property access.
The code is shorter

The structure:

{
  "t": {
    "key": "t",
    "r": {
      "key": "r",
      "u": {
        "key": "u",
        "e": {
          "key": "e",
          "value": "yes",
          "name": "true"
        },
        "c": {
          "key": "c",
          "k": {
            "key": "k",
            "value": "vehicle",
            "name": "truck"
          }
        }
      },
      "o": {
        "key": "o",
        "w": {
          "key": "w",
          "e": {
            "key": "e",
            "l": {
              "key": "l",
              "value": "dig",
              "name": "trowel"
            }
          }
        }
      }
    }
  },
  "h": {
    "key": "h",
    "a": {
      "key": "a",
      "t": {
        "key": "t",
        "value": "head",
        "name": "hat"
      },
      "l": {
        "key": "l",
        "t": {
          "key": "t",
          "value": "hold it",
          "name": "halt"
        }
      },
      "m": {
        "key": "m",
        "value": "pig",
        "name": "ham",
        "m": {
          "key": "m",
          "e": {
            "key": "e",
            "r": {
              "key": "r",
              "value": "nail",
              "name": "hammer"
            }
          }
        }
      }
    }
  }
}

As for the code that operates this, notes are on the comments:

function Trie(key) {
  this.key = key;
  this.value;
  //children are merged with this object since collision is minimal
}

Trie.prototype.put = function (name, value) {

  var node = this,
    nameLength = name.length,
    i = 0,
    currentLetter;

  //the only major change is this single loop which zips through the collection
  //if the node exists, make it current and proceed
  //if not, we create it, make it current and proceed
  for (i = 0; i < nameLength; i++) {
    currentLetter = name[i];
    node = node[currentLetter] || (node[currentLetter] = new Trie(currentLetter));
  }

  node.value = value;
  node.name = name;

};

Trie.prototype.get = function (name) {

  var node = this,
    nameLength = name.length,
    i, node;

  //same idea, zip through the collection
  //in this case we break if we hit a dead end
  for (i = 0; i < nameLength; i++) {
    if (!(node = node[name[i]])) break;
  }

  //only when the loop went over all letters will we find a value
  //if not, well, we don't find anything
  return (i === nameLength) ? node.value : 'not found';
};


Finding common strings among 2 arrays of strings of length 1, sorted alphabetically in O(n) time complexity The problem:

  Start with two arrays of strings, a and b, each in alphabetical order,
  possibly with duplicates. Return the count of the number of strings
  which appear in both arrays.

  commonTwo({"a", "c", "x"}, {"b", "c", "d", "x"}) → 2

  commonTwo({"a", "c", "x"}, {"a", "b", "c", "x", "z"}) → 3

  commonTwo({"a", "b", "c"}, {"a", "b", "c"}) → 3

I tried to solve the problem with \$O(n)\$ time complexity:

    public  int commonTwo(String[] a, String[] b) {
    int result=0;
    String prev="";
    for(int i=0,j=0;(i<a.length)&&(j<b.length);)
    {
        if(a[i].charAt(0)<b[j].charAt(0))
        {
            i++;
            continue;
        }
        if(a[i].charAt(0)==b[j].charAt(0))
        {
            if(!(a[i].equals(prev)))
                result++;
            prev=a[i];
            i++; j++;
            continue;
        }
        j++;

    }

    return result;

}

It seems to be working but the code looks somewhat ugly. Please feel free to review or if you have any other elegant solution would appreciate you include it in the review.

Java is not my strong-point, but there are a few things that stand out.

Spacing is non-standard.
There are unnecessary parentheses, e.g. in

(i<a.length)&&(j<b.length)

and

if(!(a[i].equals(prev)))

Optional braces should always be included, e.g. in

if(!(a[i].equals(prev)))
    result++;

Statements should always be on separate lines, e.g. don't do

i++; j++;

There is a more elegant solution using a HashSet<String>, which I believe has the same time complexity.

public int commonTwo(String[] a, String[] b) {
    Set<String> common = new HashSet<>(Arrays.asList(a));
    common.retainAll(new HashSet<>(Arrays.asList(b)));
    return common.size();
}


An object exposed to directives that need to track changes on it This is an object exposed to the view and is being used on directives within the same view. The directives need to track changes on the ctrl.filters.applied object. It's a simple part of a project at my work and I'd like to know if I'm doing it right.

Other properties and methods are just being used on the view itself.

ctrl is the controller, since I'm using controllerAs syntax.

var ctrl = this;

Filters object:

ctrl.filters = (function filters() {

    var currentFilters,
        appliedFilters,
        initialFilters = {
            companies: [],
            user: null,
            workDate: {
                initial: null,
                final: null
            }
        };

    currentFilters = angular.copy(initialFilters);
    appliedFilters = angular.copy(currentFilters);

    function apply() {
        appliedFilters = angular.copy(currentFilters);
    };
    function clear() {
        currentFilters = angular.copy(initialFilters);
        appliedFilters = angular.copy(currentFilters);
    };

    return function filtersReturn() {
        return {
            current: currentFilters,
            applied: appliedFilters,
            apply: apply,
            clear: clear
        };
    };

})();

Directive example:

<wkp-my-directive
    wkp-filters="ctrl.filters().applied">
</wkp-my-directive>

return function filtersReturn() {
    return {
        current: currentFilters,
        applied: appliedFilters,
        apply: apply,
        clear: clear
    };
};

This piece of code looks odd to me,  could you simply return the object itself, like this:

var returnFilters = {
    current: currentFilters,
    applied: appliedFilters,
    apply: apply,
    clear: clear
;}
return returnFilters;

I just don't see the point of returning a function that doesn't do "function" stuff,  it really returns a function that returns an object, so why not just return the object to begin with?

One more thing, I didn't like the name of the function, so when I changed it to an object I changed the naming as well.  The name returnFilters sounds better than filtersReturn  I mean when I say "filters return"  I am thinking "filters return ... what?"  but these are the return filters so I want the name to express that for me so when I read the code later I don't get confused as to what this filter is doing.


Scraping content from a javascript enabled website with load more button The script I've written is able to scrape name, address, phone and web address from a webpage using python and selenium. The main barrier I had to face was to exhaust the load more button to get the more content until all are displayed. I know the way I have written xpaths in the script is fragile but they serve the purpose for now. I ran my script and found the results as I expected. I hope there will be any better way to improve the design and performance of my script. Thanks in advance for taking care of it. Here is the full code:

from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.support.ui import WebDriverWait
from selenium.webdriver.support import expected_conditions as EC

driver = webdriver.Chrome()
wait = WebDriverWait(driver, 10)

driver.get("https://www.zebra.com/us/en/partners/partner-application-locator.html")
driver.find_element_by_xpath('//div[@class="ft-dropdown"]/input[@placeholder="City, State/Province, Country"]').clear()
driver.find_element_by_xpath('//div[@class="ft-dropdown"]/input[@placeholder="City, State/Province, Country"]').send_keys("Colorado, USA")

wait.until(EC.visibility_of_element_located((By.XPATH, '//a[contains(@class,"ng-binding")]')))
driver.find_element_by_xpath('//a[contains(@class,"ng-binding")]').click()

while True:
    try:
        wait.until(EC.visibility_of_element_located((By.CLASS_NAME, 'showmore-bg')))
        driver.find_element_by_class_name('showmore-bg').click()
    except Exception:
        break

wait.until(EC.visibility_of_element_located((By.XPATH, '//div[contains(@class,"padding0")]')))
for items in driver.find_elements_by_xpath('//div[contains(@class,"padding0")]'):
    try:
        name = items.find_element_by_xpath('.//a[@class="ng-binding"]').text
    except Exception:
        name = ""
    try:
        address = items.find_element_by_xpath('.//div[contains(@class,"fullDetail-cmpAdres")]//p[@class="ng-binding"]').text
    except Exception:
        address = ""
    try:
        phone = items.find_element_by_xpath('.//div[contains(@class,"fullDetail-cmpAdres")]//p[contains(@class,"ng-scope")]').text
    except Exception:
        phone = ""
    try:
        website = items.find_element_by_xpath('.//a[contains(@class,"ng-scope")]').get_attribute("href")
    except Exception:
        website = ""

    print(name ,address, phone, website)

driver.quit()

There are a few major things that immediately caught my eye:

bare except clauses should be avoided in favor of catching more specific exceptions
element locators quality - you should NOT be relying on layout-oriented classes like padding0 and pure-technical Angular classes like ng-binding and ng-scope
wait.until() calls in your case return a WebElement instance - you don't have to re-find an element after the wait
I think the code may benefit from comments explaining the logic behind the browser element interactions

Improved code:

from selenium import webdriver
from selenium.common.exceptions import TimeoutException, NoSuchElementException
from selenium.webdriver.common.by import By
from selenium.webdriver.support.ui import WebDriverWait
from selenium.webdriver.support import expected_conditions as EC

def get_field_text_if_exists(item, selector):
    """Extracts a field by a CSS selector if exists."""
    try:
        return item.find_element_by_css_selector(selector).text
    except NoSuchElementException:
        return ""

def get_link_if_exists(item, selector):
    """Extracts an href attribute value by a CSS selector if exists."""
    try:
        return item.find_element_by_css_selector(selector).get_attribute("href")
    except NoSuchElementException:
        return ""

driver = webdriver.Chrome()
wait = WebDriverWait(driver, 10)

driver.get("https://www.zebra.com/us/en/partners/partner-application-locator.html")

location = driver.find_element_by_css_selector('.partnerLocation input')
location.clear()
location.send_keys("Colorado, USA")

# select the first suggestion from a suggestion dropdown
dropdown_suggestion = wait.until(EC.visibility_of_element_located((By.CSS_SELECTOR, 'ul[id^=typeahead] li a')))
dropdown_suggestion.click()

# click more until no more results to load
while True:
    try:
        more_button = wait.until(EC.visibility_of_element_located((By.CLASS_NAME, 'showmore-bg'))).click()
    except TimeoutException:
        break

# wait for results to load
wait.until(EC.visibility_of_element_located((By.CSS_SELECTOR, '.seclection-result .partners-detail')))

# parse results
for result in driver.find_elements_by_css_selector('.seclection-result .partners-detail'):
    name = get_field_text_if_exists(result, 'a')
    address = get_field_text_if_exists(result, '.fullDetail-cmpAdres')
    phone = get_field_text_if_exists(result, '.fullDetail-cmpAdres p[ng-if*=phone]')
    website = get_link_if_exists(result, 'a[ng-if*=website]')

    print(name, address, phone, website)

driver.quit()


Find number of inversions in a given array Can you please give me a hint on how to improve performance of the following algorithm meant to find the number of inversions in a given array?

/* *
 * INPUT:
 * 1. t : number of test cases; t test cases follow
 * 2. n : number of elements to consider in each test case
 * 3. ar[i] : n numbers, elements of considered array
 * */

import java.util.*;

public class Inversions1 {

    public static long[] ar;
    public static long[] buff;

    // Merges arrays left[] and right[] into ar[], returns number of
    // inversions found in the process
    public static long merge(int low, int middle, int high) {
        int i = low;
        int j = middle + 1;
        int k = low;
        long count = 0;

        for(int l = low; l <= high; l++) {
                buff[l] = ar[l];
        }

        while (i <= middle && j <= high) {
            if (buff[i] <= buff[j]) {
                ar[k] = buff[i];
                i++;
            } else {
                ar[k] = buff[j];
                j++;
                count += middle-i+1;
            }
                k++;
        }

        while (i <= middle) {
            ar[k] = buff[i];
            i++; k++;
        }
        while(j <= high) {
            ar[k] = buff[j];
            j++; k++;
        }
        return count;
    }

    // Traditional merge sort on arr[], returns number of inversions
    public static long invCount(int low, int high) {
        if(low < high) {

            int middle = low + (high - low)/2;

            return invCount(low, middle) + invCount(middle+1,high) + merge(low, middle, high);
        }
        return 0;
    }

    public static void main (String args[]) {
        int t, n;
        Scanner sc = new Scanner(System.in);
        t = sc.nextInt();
        while(t-- > 0) {
            n = sc.nextInt();
            ar = new long[n];
            buff = new long[n];
            for(int i = 0; i < n; i++) {
                ar[i] = sc.nextLong();
            }
            System.out.println(invCount(0,n-1));
        }
    }
}

Here are two things that can speed up your code:

Using custom scanner instead of java.util.Scanner. You can create it using BufferedReader and StringTokenizer. It speeds up input significantly.
Avoiding allocation of left and right arrays inside invCount functions. To do it, you can rewrite your merge and invCount function so that they always use your initial array and take lower and upper bound indices. Merging would require additional buffer anyway, but it can be allocated only once (before  any merge is executed). So there would be only two array allocations per test case. It would probably make you code slightly longer and less readable, but it should make it faster.


all_products

At the top, you initialize all_products = [].  Then you call get_horizontal(), which appends products of four_elements to all_products.  Then you get and return the maximum.

Then you call get_left() which does the same thing ... but continues appending to the all_products list.  When the maximum is computed, it is the maximum of both the horizontal and the left diagonals!

In other words, you don't need to find the maximum of the horizontal, left, right and vertical results.  That is already being accidentally done.

get_horizontal()
get_left()
get_right()
print(get_vertical())    # Still returns the maximum over all 4 directions!

You should initialize all_products = [] inside the get_left, get_right, get_horizontal and get_vertical functions, and remove it from global scope.

Float or integer?

Your results ends in .0.  All the numbers are integer numbers.  You could load the grid as an integer matrix:

grid = np.loadtxt("grid.txt", dtype='i')

And now the answer is generated without a decimal point.

Looping

You've written code like this 4 times:

    start, finish = 0, 4
    while finish <= 20:
        four_elements = ... start:finish ...
        ...
        start += 1
        finish += 1

This is a simple loop from finish=4 to finish=20 (or finish=len(dgnl)).  Start and end always differ by 4, so you could use finish - 4 instead of start.  Or loop over start and use start + 4 for finish.

Eg)

    for start in range(16 + 1):
        four_elements = ... start : start+4 ...
        ...

With 4 copies of the that code, 3 less lines per copy, you've saved 12 lines.

Flip, and flip, and flip

def get_left():
    for i in range(-16, 16):
        dgnl = np.diagonal(np.flipud(grid), i)

Each time though this loop, you flipud(grid).  That's 32 identical flips.  Maybe you'd want to save this as a temporary result:

def get_left():
    flipped_grid = np.flipud(grid)
    for i in range(-16, 16):
        dgnl = np.diagonal(flipped_grid, i)

Question: Why 32?  Perhaps you meant for i in range(-16, 16 + 1):?  You were missing a diagonal!!!   Twice!!!

Call functions more than once

Now, get_left() and get_right() are identical, save for that initial flip.  You could pass the grid into get_right(), and get_left() could pass a flipped copy to get_right():

def get_right(grid):
    for i in range(-16, 16):
        dgnl = np.diagonal(grid, i)
        for start in range(len(dgnl) - 3):
            four_elements = dgnl[start:start + 4]
            all_products.append(np.prod(four_elements))
    max_right = max(all_products)
    return max_right

def get_left(grid):
    return get_right(np.flipud(grid))

Call functions more than once (reprise)

get_horizontal() and get_vertical() are duplicates also ... save for grabbing either the ith row or column.  If you transposed the matrix, columns would become rows, and you could again use the same function.

Flip, Transpose ... Rotate!

If you flip the matrix, one left-up diagonals become left-down ones, but rows and columns are still rows and columns.  If you transpose the matrix, rows become columns, columns become rows, but the left-up diagonals are still left-up diagonals.

If you rotate the matrix 90 degrees, rows become columns and left-up diagonals become left-down diagonals.

rotated = np.rot90(grid)
print(max(get_horizontal(grid), get_horizontal(rotated), get_right(grid), get_right(rotated)))

Using NumPy

All you've really used NumPy for is loading the grid from a file, flipping it, and extracting diagonals.  It is much more powerful than that.

import numpy as np

grid = np.loadtxt("grid.txt", dtype='i')

print(max(np.max(grid[ :  , :-3] * grid[ :  , 1:-2] * grid[ :  , 2:-1] * grid[ :  , 3:]),
          np.max(grid[ :-3, :  ] * grid[1:-2,  :  ] * grid[2:-1,  :  ] * grid[3:  ,  :]),
          np.max(grid[ :-3, :-3] * grid[1:-2, 1:-2] * grid[2:-1, 2:-1] * grid[3:  , 3:]),
          np.max(grid[3:  , :-3] * grid[2:-1, 1:-2] * grid[1:-2, 2:-1] * grid[ :-3, 3:])))

Note: this works for 20 x 20 matrices, and 200 x 200 matrices equally well.

Okay, perhaps we should explain exactly what is going on there.  Let's start with a 7x7 matrix, with random single-digit values:

>>> import numpy as np
>>> grid = np.random.randint(0, 10, (7, 7))
>>> grid
array([[8, 2, 3, 9, 9, 9, 3],
       [6, 2, 8, 0, 9, 4, 3],
       [0, 6, 9, 5, 6, 8, 8],
       [8, 5, 2, 6, 3, 0, 8],
       [5, 8, 0, 6, 7, 0, 3],
       [4, 3, 1, 0, 2, 5, 5],
       [4, 9, 5, 7, 2, 6, 0]])

grid[3:, :-3] will extract all the values starting at the 3rd row, and all the columns except the last 3 columns.

>>> grid[3:, :-3]
array([[8, 5, 2, 6],
       [5, 8, 0, 6],
       [4, 3, 1, 0],
       [4, 9, 5, 7]])

The grid[1:-2, 2:-1] extracts the 4x4 matrix one row up and one column to the right of the first:

>>> grid[2:-1, 1:-2]
array([[6, 9, 5, 6],
       [5, 2, 6, 3],
       [8, 0, 6, 7],
       [3, 1, 0, 2]])

And two rows up, two columns to the right:

>>> grid[1:-2, 2:-1]
array([[8, 0, 9, 4],
       [9, 5, 6, 8],
       [2, 6, 3, 0],
       [0, 6, 7, 0]])

And finally three rows up, three columns to the right ... in other words the top 4 rows, and the rightmost four columns:

>>> grid[:-3, 3:]
array([[9, 9, 9, 3],
       [0, 9, 4, 3],
       [5, 6, 8, 8],
       [6, 3, 0, 8]])

We can take these matrices, and multiply respective elements together:

>> grid[3:, :-3] * grid[2:-1, 1:-2] * grid[1:-2, 2:-1] * grid[:-3, 3:]
array([[3456,    0,  810,  432],
       [   0,  720,    0,  432],
       [ 320,    0,  144,    0],
       [   0,  162,    0,    0]])

For instance, the top-left corners contain 8, 6, 8 and 9, and the product of those is 8*6*8*9 = 3456.  Looking at the original matrix, we can see these values in a diagonal starting at row 3, column 0 and going up to row 0, column 3.

The next diagonal contains 5, 5, 9, 0, 9.  5*5*9*0 = 0 and 5*9*0*9 = 0, which is the next diagonal of our product matrix.

The largest product in that matrix?

>>> np.max(grid[3:, :-3] * grid[2:-1, 1:-2] * grid[1:-2, 2:-1] * grid[:-3, 3:])
3456

The other 3 expressions take different adjacent rectangular regions, multiply them together, and find the maximums for the other diagonal and the rows and columns.


Legendre's polynomials and functions I would like to improve this function to reduce the run time and optimize it.

function [Pl,Plm] = funlegendre(gamma)

    Plm = zeros(70,71);
    Pl = zeros(70,1);
    P0 = 1;
    Pl(1) = gamma;
    Plm(1,1) = sqrt(1-gamma^2);

    for L=2:70
        for M=0:L

                if(M==0)
                    if (L==2)
                    Pl(L) = ((2*L-1)*gamma*Pl(L-1)-(L-1)*P0)/L;
                    else
                    Pl(l) = ((2*l-1)*gamma*Pl(l-1)-(l-1)*Pl(l-2))/l;
                    end
                elseif(M<L)
                    if(L==2)
                        if(M==1)
                        Plm(L,M) = (2*L-1)*Plm(1,1)*Pl(L-1);
                        else
                        Plm(L,M) = (2*M-1)*Plm(1,1)*Plm(L-1,m-1);
                        end
                    else
                        if(M==1)
                        Plm(L,M) = Plm(L-2,m) + (2*L-1)*Plm(1,1)*Pl(L-1);
                        else
                        Plm(L,m) = Plm(L-2,m) + (2*L-1)*Plm(1,1)*Plm(L-1,M-1);
                        end
                    end

                elseif(M==L)
                    Plm(L,M) = (2*L-1)*Plm(1,1)*Plm(L-1,L-1);
                else
                    Plm(L,M) = 0;
                end
        end
    end
    Pl = sparse(Pl);
    Plm = sparse(Plm);
    end

At first glance I can see a few points where you can start to optimize your polynom generation.
First thing I would refactor is the "heavy" use of conditional cases. There are only two well defined situation in which M==0 or M==L. Therefore you could extract both cases and implement them within the outer loop. If you don't care for code duplication copy/paste the functionality but if you care you have to write another function to call, which might be slow again. After you extracted the two special cases you cann ommit the remaining condition when you let L begin at 1 and run till L-1. Another point of optimization is to precompute values like 2*L-1 in the outer loop to have them "cached" for further access. As I'm not sure if sparse matrices are computationally faster than full matrices I'm not sure if I should recommend to use them from the beginning and not to convert the results into sparse matrices. Now I would try to vectorize as many operations as possible and let highly optimized code do the job for you.
And at last step @Elpezmuerto is fully right try to turn it into a mex function. This will precompile the function and speed up the execution time even further.