Tic Tac Toe in C++11 I wrote this to sort of gauge my understanding of the bare basics of C++.Could you guys give me any pointers? (Ha, get it?). I would happy to hear what you have to say about the code. Also is it neat (aesthetically as well as functionally)?

#include <iostream>
bool end = false;
int w_combinations[8][3] = {{0,1,2},{3,4,5},{6,7,8},{0,3,6},{1,4,7},{2,5,8},{0,4,8},{2,4,6}};
int pos[] = {1,2,3,4,5,6,7,8,9};
void check (int x) {
    if ( x == 10) {
        std::cout << "X|";
    }
    else if (x == 11) {
        std::cout << "O|";
    }
    else {
        std::cout << x << "|";
    }
}
void printMatrix() {
    std::cout << "|" ; check(pos[0]) ; check(pos[1]) ; check(pos[2]);std::cout << "\n";
    std::cout << "|" ; check(pos[3]);check(pos[4]) ; check(pos[5]);std::cout << "\n";
    std::cout << "|" ; check(pos[6]) ; check(pos[7])  ; check(pos[8]); std::cout << "\n";
}
void input(int z , int player) {
    if (player == 1 && pos[z-1] != 10 && pos[z-1] != 11) {
        pos[z-1] = 10;
    }
    else if (player == 2 && pos[z-1] != 10 && pos[z-1] != 11){
        pos[z-1] = 11;
    }
    else {
        std::cout << "\nInvalid! Try again: ";
        int y;
        std::cin >> y;
        input(y,player);

    }
}

//Winning mechanism: Simply cylce through all combos.
bool win() {
    for (int i = 0 ; i < 8 ; ++i) {
        if (pos[w_combinations[i][0]] == pos[w_combinations[i][1]] && pos[w_combinations[i][0]] == pos[w_combinations[i][2]])
        {
            return true;
            break;
        }
    }
    return false;
}
int main() {
    int count = 0;          //Cycle through draws
    while (1)
    {
        int z;
        printMatrix();
        std::cout << "1[X]:";
        std::cin >> z;
        input(z,1);
        if (win()) {std::cout << "Player 1 wins!\n";break;}
        ++count;
        printMatrix();
        if (count == 9) {
            std::cout << "Draw!";
            break;
        }
        std::cout << "2[O]:";
        std::cin >> z;
        input(z,2);
        if (win()) {std::cout << "Player 2 wins!\n";break;}
        ++count;
        if (count == 9) {
            std::cout << "Draw!";
            break;
        }
    }
    std::cout << "Play again[Yes = 1, No = 0]: ";
    int permission;
    std::cin >> permission;
    if (permission == 1) {
        std::cout << "\n\n";
        main();
    }
}

I cannot check all because I use a phone at the moment, but here are some quick observations I saw skimming thru the code:

Never call main ()

-It is not defined in C++ what happens if you call main from another function. It is undefined behavior.

You could instead put your while (1) loop in a function and call it instead of main.

Use of C++ Features

-Consider using std::array or std::vector instead of using plain C style arrays.

Formating

-Formatting seem to be off in some places. Add a space between functions and don’t put several instructions on one line.

e.g.

if (win()) {std::cout << "Player 2 wins!\n";break;}
    ++count;

Should become:

if (win()) {
    std::cout << "Player 2 win\n";
    break;
}
++count;

Also you should avoid having to long lines. Get used to break the code after max 80 lines. It has the advantage that you can view two source codes next to each other.

for example make this:

if (pos[w_combinations[i][0]] == pos[w_combinations[i][1]] && pos[w_combinations[i][0]] == pos[w_combinations[i][2]])
{
    return true;
    break;
}

Into this:

if (pos[w_combinations[i][0]] == pos[w_combinations[i][1]]
    && pos[w_combinations[i][0]] == pos[w_combinations[i][2]])
{
    return true;
    break;
}

or this

void printMatrix() {
std::cout << "|" ; check(pos[0]) ; check(pos[1]) ; check(pos[2]);std::cout << "\n";
std::cout << "|" ; check(pos[3]);check(pos[4]) ; check(pos[5]);std::cout << "\n";
std::cout << "|" ; check(pos[6]) ; check(pos[7])  ;

It’s very important for code to be easy to read. You may be only write a program once but most of the time you read it while debugging or maintaining it. If the code is good formatted this task becomes a lot easier.

Avoid redundancy

In you’re while loop you use the same instructions except the messages for player 1 and 2.
Make them a function with parameters for the messages. This way you make sure you handle both players the same.

Avoid global variables

Don’t use global variables they make the code hard to maintain. They can lead to obscure bugs.
Consider passing them between you’re functions or even better check out OOP and work with a class who stores the state of the Board.

Functions should do one thing

You’re function check () should only return the string not also print it. It should become this:

std::string check (int x)
{
    if ( x == 10) {
        return "X|";
    }
    else if (x == 11) {
        return "O|";
    }
    return std::to_string (X)
}

This function now gets called and the string is directly printed. No need to change anything in the call code.

Also consider using switch instead of a long if else chain if there are many cases.

Calls to std::cout

No need to call std::cout several times. Just do this :

std::cout << '|' + check(pos[0]) + check(pos[1]) + check(pos[2]) + '\n';

Single chars should be quoted ' ' not " " to avoid getting a more expensive string use instead of a char.

Handle invalid input

Try to enter something which is not a the expected number. What happens? The program probably loops endless or crashes because you read no int.
You should handle this case.
One approach would be reading the in put as a std::string and check this string if it is a digit.
If it is not prompt an invalid input and let the user enter again.

But wait. you can also break the program if it is an int but not in range. Handle also wrong range. for example when you input 1234 the program will crash currently as well. You could do all this in a function:

int get_input();

So you can write:

int x = get_input();

Use descriptive names

Function names like Check or Input are very mysterious to me about what they are doing.

The same goes for variable names like x, y  or w_combinations.
Try to self document youre code by using clear names which describe what the variable or function does.

Ask yourself if you check this code in 1 or 2 months yourself if you still can get what the program does. With good naming’s and a clear structure it becomes a lot easier.

Use C++ Features like Classes

Youre code looks very C like. Make more use of C++ features like classes etc.
I rewrote the program with classes to demonstrate a more C++ Style as an inspiration (maybe I should also post it for review?).
Don't get overwhelmed it takes a lot of time and practice to get into C++...

tic_tac_toe.h

#ifndef TIC_TAC_TOE_020120180815
#define TIC_TAC_TOE_020120180815

#include <array>
#include <string>

namespace tic_tac_toe
{
    class TicTacToe final{
    public:
        TicTacToe() = default;
        ~TicTacToe() = default;

        // delete copy and move mechanism, we don't want to
        // copy a running game
        TicTacToe(const TicTacToe&) = delete;
        TicTacToe(TicTacToe&& other) = delete;
        TicTacToe& operator=(const TicTacToe& other) = delete;
        TicTacToe& operator=(TicTacToe&& other) = delete;

        void print_state_of_board() const;
        bool draw(int field);
        bool board_full() const;
        bool player1_win() const
        {
            return check_win_condition(FieldState::player1);
        }

        bool player2_win() const
        {
            return check_win_condition(FieldState::player2);
        }
    private:
        enum class FieldState {
            empty,
            player1, // X
            player2, // O
        };

        bool check_win_condition(FieldState state) const;
        char field_state_to_char(FieldState state) const;

        std::array<FieldState, 9> m_board{ FieldState::empty };
        bool m_player1_active{ true };

        static constexpr char m_player1_token{ 'X' };
        static constexpr char m_player2_token{ 'O' };
    };

    int get_user_input(const std::string& user_message);

    void play_game();   // main routine to run the game logic;
}  // namespace tic_tac_toe
#endif

tic_tac_toe.cpp

#include "tic_tac_toe.h"

#include <algorithm> // std::find
#include <cctype> // std::stoi
#include <iostream>
#include <vector>

namespace tic_tac_toe
{
    void TicTacToe::print_state_of_board() const
        /*
        Print the board. e.g:
        |X| |O|
        | |X| |
        |O| | |
        */
    {
        for (auto i = 0; i < m_board.size(); ++i) {
            if (i % 3 == 0 && i != 0) {
                std::cout << "|\n";
            }
            auto token = field_state_to_char(m_board.at(i));
            std::cout << '|' << token;
        }
        std::cout << "|\n";
    }

    bool TicTacToe::draw(int field)
        /*
        Tries to draw the next symbol in the field.
        Each time the function is called the player is changed.
        The user input has to be done out side. This way also a bot
        could play the game.
        If the selected field can not be set because its already
        occupied by player1 or player2 or out of range the return
        value becomes false
        */
    {
        if (field < 1 || field > m_board.size() ||
            m_board.at(field - 1) != FieldState::empty) {
            return false;
        }
        if (m_player1_active) {
            m_board.at(field - 1) = FieldState::player1;
            m_player1_active = false;
        }
        else { // player 2 active
            m_board.at(field - 1) = FieldState::player2;
            m_player1_active = true;
        }
        return true;
    }

    bool TicTacToe::board_full() const
        /*
        search for a empty field in the board
        indicating that board is full if no empty field available.
        */
    {
        auto it = std::find(
            m_board.begin(), m_board.end(), FieldState::empty);

        return it == m_board.end();
    }

    bool TicTacToe::check_win_condition(FieldState state) const
    {
        constexpr std::array<std::array<int, 3>, 8> combinations =
        {
            std::array<int, 3>{0,1,2},
            std::array<int, 3>{3,4,5},
            std::array<int, 3>{6,7,8},
            std::array<int, 3>{0,3,6},
            std::array<int, 3>{1,4,7},
            std::array<int, 3>{2,5,8},
            std::array<int, 3>{0,4,8},
            std::array<int, 3>{2,4,6}
        };

        for (const auto& combination : combinations) {
            if (m_board.at(combination[0]) == state &&
                m_board.at(combination[1]) == state &&
                m_board.at(combination[2]) == state) {
                return true;
            }
        }
        return false;
    }

    char TicTacToe::field_state_to_char(FieldState state) const
    {

        if (state == FieldState::player1) {
            return m_player1_token;
        }
        if (state == FieldState::player2) {
            return m_player2_token;
        }
        return ' ';
    }

    int get_user_input(const std::string& user_message)
    {
        while (true) {
            std::cout << user_message;
            std::string input;
            std::cin >> input;
            /*
            If input is not an integer the stoi function will raise
            an exception. We use this to determine if the input was
            an int
            */
            try {
                return std::stoi(input);
            }
            catch (std::invalid_argument&) {
                std::cout << "\nInput is not a number. Try again:";
            }
        }
    }

    void play_game()
        /*
        Main routine to play the game with 2 players
        */
    {
        while (true) {
            TicTacToe game;
            bool player1_active{ true };
            while (!game.board_full() &&
                !game.player1_win() && !game.player2_win()) {

                game.print_state_of_board();

                std::string user_message;
                if (player1_active) {
                    user_message = "1[X]:";
                }
                else {  // player2 active
                    user_message = "2[O]:";
                }
                if (!game.draw(get_user_input(user_message))) {
                    std::cout << "\nInvalid! Try again: \n";
                }
                else {
                    player1_active = !player1_active;
                }
            }

            game.print_state_of_board();

            if (game.player1_win()) {
                std::cout << "Player 1 wins!\n";
            }
            else if (game.player2_win()) {
                std::cout << "Player 2 wins!\n";
            }
            else {
                std::cout << "Draw!\n";
            }

            int choice{};
            while (true) {
                choice = get_user_input(
                    "Play again[Yes = 1, No = 0]: ");

                if (choice == 0) {
                    return;
                }
                if(choice == 1) {
                    break;
                }
            }
        }
    }
}  // namespace tic_tac_toe

main.cpp

#include "tic_tac_toe.h"

#include <iostream>

int main()
try {
    tic_tac_toe::play_game();
}
catch (std::runtime_error& e) {
    std::cerr << e.what() << "\n";
    std::getchar();
}
catch (...) {
    std::cerr << "unknown error " << "\n";
    std::getchar();
}


Listing my StackEchange communities user id Extending this answer solution, I've wrote the following bash script to get the html of my StackExchange accounts page and parse it to a YAML file. The objective was list for each community I participate its url and my user id.

get-stackexchange-info.sh

#!/bash/bin

NETWORK_PROFILE_URL=$1 # https://stackexchange.com/users/3581081/artu-hnrq
read -r ID USER <<< \
    $( cut -d'/' -f5,6 <<< $NETWORK_PROFILE_URL \
        | sed 's/\// /g' \
    )

OUTPUT_FILE=stackexchange.yml

echo \
"user: $USER
stackexchange:
  url: http://stackexchange.com
  id: $ID" \
> $OUTPUT_FILE

parse(){
    local HTTPS="https:\/\/"
    local COMMUNITY="[^\.]*"
    local DOMAIN="[^\/]*"
    local PROFILE_ID="\d*"

    local MODEL="\1:\n  url: $HTTPS\1\2\n  id: \3"

    sed -E "s/($COMMUNITY)($DOMAIN)\/($PROFILE_ID)/$MODEL/"
}

curl "https://stackexchange.com/users/$ID/$USER?tab=accounts" \
    | grep "account-container" -A1 \
    | grep "http" \
    | cut -d"/" -f3,5 \ # stackoverflow.com/3581081
    | parse \
    >> $OUTPUT_FILE

I would like to receive some opinions about the parse process (usage of grep, cut and sed commands) and about control flow management could be improved to the script

output example:

user: artu-hnrq
stackexchange:
  url: https://stackexchange.com
  id: 3581081
askubuntu:
  url: https://askubuntu.com
  id: 689894
stackoverflow:
  url: https://stackoverflow.com
  id: 2989289
codereview:
  url: https://codereview.stackexchange.com
  id: 213097

Thanks a lot!

I see 2 errors and have some more advice.
The Shebang is wrong.
#!/bash/bin
should be
#!/bin/bash

Do not put comment after the \ escape for a new line
| cut -d"/" -f3,5 \ # stackoverflow.com/3581081
# should be
| cut -d"/" -f3,5 \

I would move all functions (in your case only parse()) to the top of the file.
Use lowercase variables (uppercase is reserved for system variables like PATH).
NETWORK_PROFILE_URL=
# change to
network_profile_url=

Combine cut and sed
read -r ID USER <<< \
    $( cut -d'/' -f5,6 <<< $NETWORK_PROFILE_URL \
        | sed 's/\// /g' \
    )
# can be replaced by
read -r ID USER < <(awk -F "/" '{print $(NF-1) " " $NF }' <<< $NETWORK_PROFILE_URL)
# which is easier to read then
source <(sed -r 's#.*/(.*)/(.*)#ID=\1; USER=\2#' <<< $NETWORK_PROFILE_URL)

When you end a line with a pipe, you do not need to escape the newline with \.
curl "https://stackexchange.com/users/$ID/$USER?tab=accounts" \
    | grep "account-container" -A1 \
    | grep "http" \
    | cut -d"/" -f3,5 \
    | parse
# can be changed into
curl "https://stackexchange.com/users/$ID/$USER?tab=accounts" |
    grep "account-container" -A1 |
    grep "http" |
    cut -d"/" -f3,5 |
    parse

You had a hard job parsing the curl output with different tools. In this situation you won't need to parse large files, the performancy penalty is very small. It is worth the effort for other cases to look for a solution with awk.
The grep -A1 is replaced by setting a flag for parsing the next line.
Fields are found with the delimiter '/'.
Combining substr is only slightly more complex:
curl -s "https://stackexchange.com/users/$ID/$USER?tab=accounts" |
   awk -F "[/]" '
      parse==1 {
         print substr($3,1,index($3,".")-1) ":";
         printf("  url: https://%s\n", $3);
         printf("  id: %s\n", $5);
         parse=0;
      }
      /account-container/ {parse=1}
    '


Method to return the lowest filename number not being used I have a Django site where users upload and have the option to delete files. There will be a lot of files that will have the same logical name so I append numbers to them:

foo/bar/file.mp3 foo/bar/file2.mp3...

I needed a method that would return the lowest file number not currently being used. I have a working example here, but it feels wrong and ugly and I don't know if there is an easier way of doing it. If I ran the method on the examples above, it would return "1" so I know I can make the next file foo/bar/file1.mp3.

def get_num(self, files):
        """function to extract the lowest file number to save as, returns string. Takes a list as input"""
        nums = []
        for f in files:
            # Call the unicode function which should return the string path
            match = re.findall("([\d]+)\.[\w]+", f.__unicode__())
            if match:
                nums.append(int(match[0]))
            else:
                nums.append(0)

        for i in range(len(nums) + 1):
            if i not in nums:
                if i == 0:
                    return ""
                else:
                    return str(i)

When you don't need indexes or ordering, it is generally a good idea to use a set over a list because its membership testing is more efficient.  That means, use nums = set() instead of nums = [], and use nums.add(...) instead of nums.append(...).  That's all you need to change.

I'm not sure why you are accessing the special method directly.  Why not use unicode(f) instead of f.__unicode__()?

Python has its own version of something that is often called a ternary operator.  Its format is truth_value if some_condition else false_value.  Therefore, you can replace your first if and else blocks with this:

nums.append(int(match[0]) if match else 0)

(It would be nums.add(...) if you decide to use a set.)

and your second if and else blocks with this:

return "" if i == 0 else str(i)


How to handle same code parts in Factory Method? I have classes:

public abstract class House{
   public string Name {set;get;}
   public SomeClass Property1 {set;get;}
   public OtherClass Property2 {set;get;}
}

public class WoodenHouse:House{
   public string WoodType {set;get;}
   public int WoodAge {set;get;}
}

public class StoneHouse:House{
  public string StoneType{set;get;}
}

And trying to create Factory Method patthern for this:

    abstract class Creator
    {
        public abstract HouseInfo Info { get; set; }

        public Creator()
        {
        }

        public abstract House FactoryMethod();
    }

    class WoodenHouseCreator : Creator
    {
        public override HouseInfo Info { get; set; }

        public WoodenHouseCreator(WoodenHouseInfo info)
        {
            Info = info;
        }

        public override House FactoryMethod()
        {
            var info = Info as WoodenHouseInfo;

            var woodenHouse = new WoodenHouse();
            woodenHouse.Name = info.Name;
            woodenHouse.Floors = info.Floors;
            woodenHouse.RoofType = info.RoofType;
            woodenHouse.WoodType = info.WoodType;
            woodenHouse.WoodAge = info.WoodAge;
            return woodenHouse;
        }
    }

    class StoneHouseCreator : Creator
    {
        public override HouseInfo Info { get; set; }

        public StoneHouseCreator(StoneHouseInfo info)
        {
            Info = info;
        }

        public override House FactoryMethod()
        {
            var info = Info as StoneHouseInfo;

            var stoneHouse = new StoneHouse();
            stoneHouse.Name = info.Name;
            stoneHouse.Floors = info.Floors;
            stoneHouse.RoofType = info.RoofType;
            stoneHouse.StoneType = info.StoneType;
            return stoneHouse;
        }
    }

Here is classes what contains information to create house:

    class HouseInfo
    {
        public string Name { set; get; }
        public int Floors { set; get; }
        public string RoofType { set; get; }
    }

    class WoodenHouseInfo : HouseInfo
    {
        public string WoodType { set; get; }
        public int WoodAge { set; get; }
    }

    class StoneHouseInfo : HouseInfo
    {
        public string StoneType { set; get; }
    }

And Usage:

        var houseInfo = new WoodenHouseInfo{
            Name = "HouseName",
            Floors = 2,
            RoofType = "Triangle",
            WoodType = "Pine",
            WoodAge = 100
        };

        House house;

        if(houseInfo is WoodenHouseInfo)
        {
            var creator = new WoodenHouseCreator(houseInfo);
            house = creator.FactoryMethod();
            Console.Write((house as WoodenHouse).WoodAge);
        }

Full code fiddle.

My problem that how to handle code duplication. I mean here is a lot of lines what fills base House object properties. How can i write that code only once?
Or i should not to use Factory Method?

Add Populator Class

class HousePopulator
    {
        public void PopulateHouse(HouseInfo info, House house)
        {
            house.Name = info.Name;
            house.Floors = info.Floors;
            house.RoofType = info.RoofType;
        }
    }

And usage:

abstract class Creator
{
    public abstract HouseInfo Info{get;set;}
    public HousePopulator HousePopulator {get;set;}
    public Creator()
    {
        HousePopulator = new HousePopulator();
    }
    public abstract House FactoryMethod();
}

class WoodenHouseCreator : Creator
{
    public override HouseInfo Info{get;set;}

    public WoodenHouseCreator(WoodenHouseInfo info)
    {
        Info = info;
    }
    public override House FactoryMethod()
    {
        var info = Info as WoodenHouseInfo;

        var woodenHouse = new WoodenHouse();
        HousePopulator.PopulateHouse(Info, woodenHouse);
        woodenHouse.WoodType = info.WoodType;
        woodenHouse.WoodAge = info.WoodAge;
        return woodenHouse;
    }
}

Currently your factories instantiate the new objects and then fill in all of their properties with the right values. You could split instantiation from property value assignment. Your StoneHouseCreator could instantiate a StoneHouse, use a HousePopulator that populates the values that all objects of type House have in common, and then the StoneHouseCreator could populate the rest of the values that are exclusive to a StoneHouse. That same HousePopulator could also be used by your WoodenHouseCreator, which would then proceed to populate the WoodenHouse-specific properties.

Actually, there are two ways of doing this. Either you keep it as it was originally (i.e. using inheritance), give your base class a PopulatorMethod(...) that populates the properties of a House, and call base.PopulatorMethod(...) from your override of PopulatorMethod(...) in the child classes. Or you drop the inheritance between your factories completely, you make your FactoryMethod() implementations accept instances of WoodenHouseInfo or StoneHouseInfo depending on the implementing class, and have them use a HousePopulator as you have done.

If you go down the second route, you don't need an abstract parent class, although an interface would be nice, and you can move the HousePopulator = new HousePopulator() assignment to the individual factory classes.

If you want to philosophise about this at a higher level, these are the problems that we run into because of inheritance. Factories, that is the logical separation of object use from object creation, are more naturally suited to cases where you use composition over inheritance. If you are interested more in this, I would recommend reading this excellent article on the topic.


Is this Date class well-written? I'm a C++ beginner and have made a simple class. But I'm not sure if this is well-written. It's basically just a Date class.

#include <iostream>

using namespace std;

class Date{
    int d, m, y;    //Day, Month, Year
public:
    Date(int dd = 1, int mm = 1, int yy = 1);

    int addDay(const int &dd) { d += dd; }
    int addMonth(const int &mm) { m += mm; }
    int addYear(const int &yy) { y += yy; }

    int day() const { return d; }
    int month() const { return m; }
    int year() const { return y; }

    void display() const;   //Print to screen

};

void Date::display() const {
    cout << d << "." << m << "." << y << endl;
}

Date::Date(int dd, int mm, int yy){
    d = dd;
    m = mm;
    y = yy;
}

For example, the arguments of addDay(const int &dd);. Is that good, or should it just be an integer without const and without reference?

For built-in types, there is no point in passing it as a const& - prefer a plain int (or a const int) instead.

Other comments:

Your add- functions are said to return int even though you don't have a return statement. I don't think that would or should even compile. Change them to void.
Take the time to write out day, month and year. The extra seconds in typing is nothing compared to the readability gain.
Are you sure you want a default date?
Consider overloading operator<< to allow printing your class.
Consider changing your display() function into a to_string function instead, and have it return a std::string.
Make your class easy to use correctly, and hard to use incorrectly. What if month is 12 and someone calls addMonth()? What is someone passes in June as month, but 31 as day? Is day day the day of the month, or day of the year? Adhere to the principle of least astonishment. Your class should behave the way users of the class expect it to.
Don't pollute the global namespace with using namespace std.
Split the code into a header and an implementation file. Even though you won't need it now, it's good practice. That makes point 7 even more crucial.

You need to make some judgments to design a Date class properly. I won't give you any further suggestions, because it is a good exercise to think about it. Revise your code, and think about the pros and cons of various approaches. Apart from that and the points above, your code looks OK.


Monitor Downloads folder and sort files as they are downloaded I have written my first shell script that uses inotify-tools to monitor my Downloads directory. What I intend to do with the script is that, as soon as a file is downloaded, it moves that file to a folder named after its extension. For example: if it is test.pdf it will be moved to a folder named pdf.

The script is here:

#!/bin/bash

cd ~/
basepath=Downloads/InternetDownloads/
while true ; do
filename=$(inotifywait -e create --format "%f" Downloads/InternetDownloads)
arr=(${filename//./ })
if [ ! -d ${arr[1]} ];
    then mkdir ${basepath}${arr[1]}
fi

separator='/'

newpath=${arr[1]}
mv $basepath${filename} $basepath$newpath${separator}
done

The program is working correctly, but I am having trouble ending it elegantly. I have obtained a solution for working here. I want to know if my code is correct since the normal procedure to end scripts of this sort is just to fine the process if and using kill pid

There's a bunch of small items to consider, and then some larger items too. Starting small....

Sweating the small stuff....

cd ~/ can be shortened to just cd. Not a big deal one way, or the other, but still....
Try to quote string values, even when they do not have spaces. Your basepath variable may end up having a space in it at some point ("My Downloads" at some point?), and the quotes make it clear. Again, this is a minor item...
Having declared the basepath variable, why are you not using it? Here you have:

filename=$(inotifywait -e create --format "%f" Downloads/InternetDownloads)

which should be:

filename=$(inotifywait -e create --format "%f" "$basepath")

... note the quotes around the variable in case it ends up with spaces again.

File Extensions

You pull the extension off the file with: arr=(${filename//./ }). This replaces the dots in the filename, with spaces, then loads that space-separated list in to an array (which considers the spaces to be item-separators). Then, you pull the extension from the array with ${arr[1]}....

Ouch. This works only if there's only one . in the file name, and the file has no spaces in the name.

There's a much simpler pattern-match you can do to alleviate this problem:

extension=${filename##*.}

From the documentation:

  the result of the expansion is the expanded value of parameter with the shortest matching pattern (the # case)  or the  longest matching pattern (the ## case) deleted.

With that, there's no arrays, and the extension is the last part (or the whole filename if there are no '.' values in the file.... you need to check for that case too. There's also the possibility that a file may end with a '.', leaving an empty-value extension.... you need to check that too.

inotify

Currently you have a number of problems with inotify:

you are letting it dump messages to STDERR:

Setting up watches.
Watches established.

This is repeated for each file. It's unnecessary, and can be removed with the -q option to inotifywait.
You are using the create event to track. This is triggered when the file is created... but is that what you want? A download may take a while to complete, so moving the file part way through a download may not make sense. Many programs will continue based only on the filehandle, which does not depend on the file location, but not all programs will behave well if the file is moved part way through.

You should consider tracking the close_write event instead. This will only trigger when a file handle is clsed after a write to the file handle. I expect this will track the 'end of download' rather than the 'start of download'
inotifywait (the way you have it set up) will only notify when the first file is created. This leaves you open to race conditions where a download may happen when you are copying the file to a new location. This new download will not be triggered on the next inotifywait cycle because it started before the wait. As a result, you are at risk of losing events.

I believe the best strategy for your job would be to declare a function for the jobs you want to do with each file.... and then pipe a 'monitor' stream in to it. I have put together the following:

#!/bin/bash

function relocate {
    local filename=$1
    local extension=${filename##*.}
    # need -d to text for directories we ourselves create
    if [[ -d "$filename" || "$extension" = "" || "$extension" = "$filename" ]]
    then
        # Directory, No extension, or empty extension.
        echo "Skipping: $filename"
        return
    fi

    if [ ! -d "$extension" ]
    then
        echo "Creating directory: $extension"
        mkdir "$outdir/$extension"
    fi

    echo "Relocate: $filename -> $outdir/$extension"
    mv "$filename" "$outdir/$extension/"

}

path=${1:-.}
outdir=${2:-.}

cd $path
echo "Working in $PWD with relocate to $outdir"

inotifywait --quiet --monitor --event close_write --format "%f" .  | while read line
do
    relocate "$line"
done


Russian Peasant Multiplication Algorithm For university, we have just started to learn C++ as a language, and one of our challenges to do in our spare time is to write an algorithm for 'Peasant multiplication'. In the challenge, we have been told that we cannot use the multiplication operator *.

My code for it is as follows.

#include <iostream>
#include <vector>
#include <cmath>
#include <string>
#include <limits>
using namespace std;

vector<int> half(float numberToHalf) {
    vector<int> steps;
    while (numberToHalf != 1) {
        steps.push_back(numberToHalf);
        numberToHalf = floorf(numberToHalf / 2);
    }
    steps.push_back(1);
    return steps;
}

vector<int> repeatedDouble(int number, int limit) {
    vector<int> doubleList;
    for (int i = 0; i < limit; i++)
    {
        doubleList.push_back(number);
        number += number;
    }
    return doubleList;
}

int adding(vector<int> halfList, vector<int> doubleList) {
    int total = 0;
    for (int i = 0; i < halfList.size(); i++) {
        if (halfList[i] % 2 != 0) {
            total += doubleList[i];
        }
    }
    return total;
}

int verify(string timeRound) {
    bool roundAgain = true;
    int number;
    do {
        cout << "Enter the " << timeRound << " number: " << endl;
        cin >> number;
        if(cin.good() && 0 < number) { //if number entered is positive and an integer
            roundAgain = false;
        }else {
            //Make cin ready to take another input
            cin.clear();
            cin.ignore(numeric_limits<streamsize>::max(),'\n'); //Snippet taken from https://stackoverflow.com/questions/16934183/integer-validation-for-input
            cout << "A valid input is a POSITIVE, whole, number. No letters, words, or negatives" << endl;
        }
    } while (roundAgain);
    return number;
}

int main() {
    int num1 = verify("First");
    int num2 = verify("Second");

    vector<int> halved = half(num1);
    vector<int> doubled = repeatedDouble(num2, halved.size());
    cout << "The result is: " << adding(halved, doubled);
}

Any help in optimising the code to make it look and run even better would be appreciated! Also, if any of you could give some tips for robustness or similar that would be great too!

I am completely new to C++ so any tricks and tips from experienced coders would be amazing!

It looks to me that you're being too literal.  When trying to convert an analog process to code it's easy to fall into that trap.

Presently you're using 2 loops to make 2 lists.  This is unnecessary.  You can do everything including calculate the answer in one loop.

You're also not checking for which number is the lesser one and which is the greater one.  This is integral to the base algorithm, that you're trying to emulate.

A simplified version could look something like this:

int PeasantMultiply(int num1, int num2)
{
    auto pair = std::minmax(num1, num2);
    int min = pair.first;
    int max = pair.second;
    int total = 0;
    if (min != 0)
    {
        do
        {
            if (min % 2 == 1)
            {
                total += max;
            }
            min /= 2;
            max += max;
        } while (min > 0);
    }
    return total;
}

After looking at this code again, I came upon an optimization.  Instead of using the modulus operator(%), I could accomplish the same thing with and extra int variable and use subtraction instead:

int PeasantMultiply(int num1, int num2)
{
    auto pair = std::minmax(num1, num2);
    int oldMin = pair.first;
    int max = pair.second;
    int total = 0;
    int newMin = 0;
    if (oldMin != 0)
    {
        do
        {
            newMin = oldMin / 2;
            if (oldMin - newMin != newMin)
            {
                total += max;
            }
            oldMin = newMin;
            max += max;
        } while (oldMin > 0);
    }
    return total;
}

In my tests this saves about 10% in time.


Insert a Node at the Tail of a Linked List I've solved this question, but have a small question. In the while loop we check if (temp->next != NULL), why wouldn't work if we check (temp != NULL). Isn't it the same, both checks will get us to the end of the linked list? Also any code review is appreciated!

  You are given the pointer to the head node of a linked list and an
  integer to add to the list. Create a new node with the given integer.
  Insert this node at the tail of the linked list and return the head
  node. The given head pointer may be null, meaning that the initial
  list is empty.

/*
  Insert Node at the end of a linked list
  head pointer input could be NULL as well for empty list
  Node is defined as
  struct Node
  {
     int data;
     struct Node *next;
  }
*/
Node* Insert(Node *head,int data)
{
  // creates a temp to hold the last node and set last's data and next
    Node *last = new Node;
    last->data = data;
    last->next = NULL;
    // if the linked list is empty then set head = last
    if (head == NULL) {
        head = last;
    } else { // if the linked list is not empty
        // creates a temp node and sets it to head
        Node *temp = new Node;
        temp = head;
        // uses temp to find the last node
        // Why do we have to use temp->next here instead of temp? wouldn't it be the same?
        while (temp->next != NULL) {
            temp = temp->next;
        }
        // appends the last node with last
        temp->next = last;
    }
    // returns head of linked list
    return head;
}

Memory Leak
These lines here create a memory leak, because you allocate a node and never free it:

    // creates a temp node and sets it to head
    Node *temp = new Node;
    temp = head;

You don't need to allocate another node there, because you aren't actually ever using it.  You can just set the temp pointer directly to head:
    Node *temp = head;


Fizz Buzz function that returns an array I am a newbie to JS and I am working on a FizzBuzz challenge. Below is what I have tried. Essentially when the function takes a value then it would iterate from 1 up to the value and an array would result.
I am seeking advice on adding conditional checking such as:

If the value of n is less than 4, it will return the string: "Please insert a value greater than or equal to 4".
If the function is passed no value, it should return an empty array.

I am not sure where to add and should I be using if...else or simply if for each conditional checking.
var fizzBuzz = function(n, arr = []) {
  for (let i = 1; i < n; i++) {
  if (n === 1) {
    arr.push('1');
    return arr.reverse();
  } else {
    if (n % 4 === 0 && n % 5 === 0) {
      arr.push('FIZZBUZZ');
    } else if (n % 5 === 0) {
      arr.push('Buzz');
    } else if (n % 4 === 0) {
      arr.push('Fizz');
    } else {
      arr.push('' + n);
    }
    return fizzBuzz(n - 1, arr);
  }
  }
}

I am hoping to seek advice on adding conditional checking such as: 1) if the value n is less than 4, it will return the string: "Please insert a value greater than or equal to 4" and 2) if the function is passed no value, it should return an empty array. I am not sure where to add and should I be using if...else or simply if for each conditional checking.

If we check for n < 4 and returns a fixed string "Please insert a value greater than or equal to 4" anyway, do we really need to do anything before it? If we don't need to do anything before it, where should we put it?
Same goes for 2) if the function is passed no value, it should return an empty array

For the code itself, some points
1. Let's look at the loop
for (let i = 1; i < n; i++) {
  if (n === 1) {
    // blah
    return arr.reverse();
  } else {
    // blah
    return fizzBuzz(n - 1, arr);
  }
}

On every iteration this will return something. The for loop won't even need to go through the next iteration, it will always break and return on any iteration(!). You are doing recursion and loop at the same time.
2. arr.push('' + n);
Maybe use toString() method for numbers arr.push(n.toString());
3. if-else and returns
if (n === 1) {
    arr.push('1');
    return arr.reverse();
} else {
    // blah
}

Since the if block will always return, we don't need the else. This can be rewritten as
if (n === 1) {
    arr.push('1');
    return arr.reverse();
}

// the else blah
```


Data processing task for bioinformatics I threw together this C program today to handle a bioinformatics data processing task. The program seems to work correctly, but I wanted to know if anyone has suggestions regarding how the input data are parsed and how I've used control structures in the main processing loop.

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

#define BUFFERSIZE 1024

FILE *fileopen(const char *filename, const char *mode)
{
  FILE *fh = fopen(filename, mode);
  if(fh == NULL)
  {
    fprintf(stderr, "error: unable to open file '%s'\n", filename);
    exit(1);
  }
  return fh;
}

void *memalloc(size_t size)
{
  void *memory = malloc(size);
  if(memory == NULL)
  {
    fprintf(stderr, "error: unable to allocate memory\n");
    exit(1);
  }
  return memory;
}

int main(int argc, const char **argv)
{
  // Parse command line arguments
  char seqbuffer1[BUFFERSIZE];
  char seqbuffer2[BUFFERSIZE];
  char qualbuffer1[BUFFERSIZE];
  char qualbuffer2[BUFFERSIZE];
  char *seqbufferout;
  FILE *seqinfile;
  FILE *seqoutfile;
  FILE *qualinfile;
  FILE *qualoutfile;
  int seqlength;
  if(argc != 6)
  {
    fprintf(stderr, "error: 5 arguments required, %d provided\n", argc - 1);
    exit(1);
  }
  seqinfile   = fileopen(argv[1], "r");
  qualinfile  = fileopen(argv[2], "r");
  seqoutfile  = fileopen(argv[3], "w");
  qualoutfile = fileopen(argv[4], "w");
  seqlength   = atoi(argv[5]);
  assert(seqlength > 0);

  // Process reads
  seqbufferout = (char *)memalloc( sizeof(char) * (seqlength + 1) );
  seqbufferout[seqlength] = '\0';
  while(fgets(seqbuffer1, BUFFERSIZE, seqinfile))
  {
    if(strlen(seqbuffer1) > 0)
    {
      int ambiguous;
      int i;

      assert(strncmp(seqbuffer1, ">", 1) == 0);
      if(fgets(seqbuffer2, BUFFERSIZE, seqinfile) == NULL)
      {
        fprintf(stderr, "error: file ends with a fasta header\n");
        exit(1);
      }

      if( fgets(qualbuffer1, BUFFERSIZE, qualinfile) == NULL ||
          fgets(qualbuffer2, BUFFERSIZE, qualinfile) == NULL )
      {
        fprintf(stderr, "error: no quality entry corresponding to sequence '%s'\n", seqbuffer1);
        exit(1);
      }

      ambiguous = 0;
      for(i = 0; i < seqlength; i++)
      {
        if(seqbuffer2[i] == '.')
        {
          ambiguous = 1;
          break;
        }
        seqbufferout[i] = seqbuffer2[i];
      }

      if(!ambiguous)
      {
        fprintf(seqoutfile,  "%s%s\n", seqbuffer1, seqbufferout);
        fputs(qualbuffer1, qualoutfile);

        i = 0;
        char *tok = strtok(qualbuffer2, " ");
        while(tok != NULL && i < seqlength - 1)
        {
          if(i > 0)
            fputs(" ", qualoutfile);
          fputs(tok, qualoutfile);
          tok = strtok(NULL, " ");
          i++;
        }
        fputs("\n", qualoutfile);
      }
    }
  }

  fclose(seqinfile);
  fclose(qualinfile);
  fclose(seqoutfile);
  fclose(qualoutfile);
  free(seqbufferout);

  return 0;
}

Instead of the big indent block you make by doing if(strlen(seqbuffer1) > 0), do: if (strlen(seqbuffer1) <1) continue;, so the entire rest of the loop can be outdented a level, making it more readable.

Similarly, you can turn the if(!ambiguous) into if (ambiguous) continue; and outdent that block as well.

Depending on what version of C you're using, ambiguous could be a boolean instead of an int.

You could be a little more consistent with your NULL comparisons to the result of fgets() - the one in the while() condition is implicitly != NULL but the ones in the if() statements are explicitly == NULL. IMO you should be either explicit everywhere or implicit everywhere - either choice is fine, but mixing styles makes your code a bit more unclear.

assert(strncmp(seqbuffer1, ">", 1) == 0); is the same as the more-easily-understood assert(seqbuffer1[0] == '>'), right?

Also, you've got at least one potential bug: What if BUFFERSIZE is smaller than input line length and then seqlength > BUFFERSIZE?


Does this function cache correctly? I want to limit the load so I rewrite the most expensive method and add caching, both memcache and cache-control:

class GeoRSS(webapp.RequestHandler):

    def get(self):
        start = datetime.datetime.now() - timedelta(days=60)
        count = (int(self.request.get('count'
                 )) if not self.request.get('count') == '' else 1000)
        memcache_key = 'ads'
        data = memcache.get(memcache_key)
        if data is None:
            a = Ad.all().filter('modified >',
                                start).filter('published =',
                    True).order('-modified').fetch(count)
            memcache.set('ads', a)
        else:
            a = data
        template_values = {'a': a, 'request': self.request,
                           'host': os.environ.get('HTTP_HOST',
                           os.environ['SERVER_NAME'])}
        dispatch = 'templates/georss.html'
        path = os.path.join(os.path.dirname(__file__), dispatch)
        output = template.render(path, template_values)
        self.response.headers["Cache-Control"] = "public,max-age=%s" % 86400
        self.response.headers['Content-Type'] = 'application/rss+xml'
        self.response.out.write(output)

Will this limit the load on my app? Did I prioritize correctly when I see the load on the app:

You can simplify (and sometimes speed up) things like this

try:
    a = memcache.get('ads')
except KeyError:
    a = Ad.all().filter('modified >',
                            start).filter('published =',
                True).order('-modified').fetch(count)
    memcache.set('ads', a)

It's (often) slightly faster in Python to avoid the if statement.

After measuring that with timeit, you should rename the variable a.  That's a poor name.


Auditing a complex mix of reasonably small tables I plan to use the following approach to audit the tables that represent user-editable configuration of an automated system (over the course of system's life these will be inevitably extended in their numbers and stuffed with more columns). This approach will not be applied to main data tables as those are automatically updated.

SQL Server 2005+, based on this. Suggestions?

CREATE TABLE [dbo].[audit] (
      [id] [bigint] IDENTITY(1,1) NOT NULL,
      [time] [datetime] NOT NULL,
      [username] [nvarchar](128) NOT NULL,
      [useraddress] [nvarchar](48) NULL,
      [tablename] [nvarchar](128) NULL,
      [operation]  AS (case when [oldvalue] IS NULL then 'I' when [newvalue] IS NULL then 'D' else 'U' end),
      [oldvalue] [xml] NULL,
      [newvalue] [xml] NULL
)
GO

ALTER TABLE [dbo].[audit] ADD CONSTRAINT [DF_audit_time] DEFAULT (sysutcdatetime()) FOR [time]
GO

CREATE PROCEDURE [dbo].[save_audit]
      @old_values xml,
      @new_values xml,
      @tablename nvarchar(128)
AS
BEGIN
      SET NOCOUNT ON;

      IF @old_values IS NOT NULL OR @new_values IS NOT NULL
      BEGIN
            DECLARE
                  @username nvarchar(128),
                  @useraddress varchar(48)

            SELECT @username = original_login_name FROM sys.dm_exec_sessions WHERE session_id = @@SPID
            SELECT @useraddress = client_net_address FROM sys.dm_exec_connections WHERE session_id = @@SPID

            INSERT INTO audit (username, useraddress, tablename, oldvalue, newvalue)
            VALUES (@username, @useraddress, @tablename, @old_values, @new_values)
      END
END
GO

CREATE TRIGGER [dbo].[audit_tablename] ON [dbo].[tablename]
    AFTER INSERT,DELETE,UPDATE
AS
BEGIN
    SET NOCOUNT ON;

    DECLARE @i xml, @d xml, @t nvarchar(128)
    SET @i = (SELECT * FROM inserted FOR XML AUTO)
    SET @d = (SELECT * FROM deleted FOR XML AUTO)
    SET @t = (SELECT OBJECT_NAME(parent_object_id) FROM sys.objects WHERE object_id = @@PROCID)
    EXEC save_audit @d, @i, @t
END

EDIT: Table name detection was found unreliable on 2005, fixed.

EDIT2: Not all versions of SQL Server 2005 have original_login_name column in sys.dm_exec_sessions (it was added with SP2, apparently), will have to use login_name if targeting all versions (the meaning is different, though).

Looks Like a nice, straight forward solution.

The only suggestion that I have , and while this is an unlikely situation, if the database were hosted on a failover cluster and the user were to connect to the instance locally (i.e. from anyone of the servers in the cluster), you wouldn't know which computer they logged in from. The value of @useraddress would be set as '<local machine>'.

This would also be the case when a user connects locally to the SQL Server instance, but if there's only one instance (i.e. not in a cluster), then it's not really a big deal, since you know which computer that is.

Although, to play it safe, why not modify the code to set the value for @useraddess like this, which will give you the computer name instead of '<local machine>':

DECLARE @useraddress nvarchar(128); --<== Originally set as varchar(48)

SELECT @useraddress = REPLACE(client_net_address
                     ,'<local machine>'
                     ,CAST(SERVERPROPERTY('MachineName') AS nvarchar(128)))
  FROM sys.dm_exec_connections
 WHERE session_id = @@SPID;

See:

SERVERPROPERTY (Transact-SQL)


Ruby - class definition for connecting to external API I've defined a Client class to connect to an external API.  The Client class will return XML responses for various resources.  Here's what it looks like now:

require 'nokogiri'

class Client
  include Exceptions

  CONFIG = YAML.load_file(File.join(Rails.root, 'config', 'mark.yml'))

  def self.get_leagues_xml(league, start_date, end_date)
    raise ArgumentError, "No args can be nil." if league.nil? || start_date.nil? || end_date.nil?

    start_date = format_date(start_date)
    end_date = format_date(end_date)

    @url = build_url_for(:leagues)
    @url_params = {'League' => league, 'StartDate' => start_date, 'EndDate' => end_date}

    call_http_service
  end

  def self.get_seasons_xml(season)
    raise ArgumentError, "No args can be nil." if season.nil?

    @url = build_url_for(:seasons)
    @url_params = {'Season' => season}

    call_http_service
  end

  protected

  def self.call_http_service
    begin
      conn = create_connection
      resp = get_response(conn)
      resp_xml = Nokogiri::XML(resp.body)
    rescue Faraday::Error::ClientError => ce
      raise MarkClientError.new("Client error occurred trying to access Mark feed at #{@url}: #{ce.message}.")
    rescue => e
      raise e
    end
  end

  def self.get_response(connection)
    connection.get do |req|
      req.params['Username'] = CONFIG['api']['username']
      req.params['Password'] = CONFIG['api']['password']
      @url_params.each_pair { |key, val| req.params[key] = val}
      req.options = {:timeout => 30} # open/read timeout in secs
    end
  end

  def self.create_connection
    Faraday.new(:url => @url) do |builder|
      builder.request  :url_encoded
      builder.response :logger
      builder.adapter  :net_http
    end
  end

  def self.build_url_for(resource)
    CONFIG['api']['url'] + CONFIG['resource']["#{resource.to_s}"]['url']
  end

  def self.format_date(date)
    date = Chronic.parse(date) if date.is_a?(String)
    date.strftime('%m/%d/%Y')
  end

end

My primary question is whether or not my use of the class instance variables (@url and @url_params) is an anti-Rubyism and will get me into trouble when multiple concurrent class method calls are being made?  I've considered making Client a module or a superclass and then having separate classes for each type of API call that needs to made, for ex, LeaguesClient and SeasonsClient.  That way I could make those instantiated classes and initialize with url and url_params.  I'm starting to experiment with the distinct nuances of Ruby but want to check with you guys as to how you would approach.  Any thoughts you have would be welcome.  Thanks!

CONFIG = YAML.load_file(File.join(Rails.root, 'config', 'mark.yml'))

This does not belong inside the class, it makes it impossible for others to change the configuration depending on environment (dev, test, prod). You can either pass in a map of options to the constructor or say in Rails use an initializer class to read in the appropriate configuration and apply it.

raise ArgumentError, "No args can be nil." if league.nil? || start_date.nil? || end_date.nil?

It's good to validate your arguments, however this message doesn't help the developer/user of the class because it doesn't indicate which argument was nil. As tedious as it is I would validate each arg separately and indicate which argument was nil in the message.

Regarding the use of @url and @url_params sgmorrison is correct and you should not be using class instance variables especially in a static class. If their values were immutable then fine but that is not the case. A simple change would be to pass both url and urlparams as options/args to call_http_service and so forth.

Another option would be to separate the two concerns of this class into individual classes. One that is a service class and one that is a connection class. Then you can encapsulate all connection logic into one (likely none static) class, and the user can interface with the service class. Should you choose this method you could then use a factory and/or mock out the connection to make unit testing easier.


Parsing a string to get the values of certain attributes I have written a small method that extracts certain information from a string. An example of such a string is

"Some text BLA/123/5345/349230498 some more text PNR: 12345678, Name: John, CallName: Peter, TimeStamp: 01.10.2015"

Now I need certain information from this string, e.g. the PNR, Name, CallName, and TimeStamp (as string). Right now the method looks like this:

/// <summary>
/// Reads the value of a specified attribute from the log entry.
/// </summary>
/// <param name="identifier">The prefix used in the string (e.g. "Name" in "Name: John").</param>
/// <returns>The value of the attribute (e.g. "John" bei "Name: John".</returns>
private string GetValueFromMessage(string identifier)
{
  int index = this.Message.IndexOf(identifier) + identifier.Length + 2;

  if (index != -1)
  {
    char c = this.Message[index];
    string result = string.Empty;

    while (c != ',')
    {
      result += c;
      index++;
      if (index < this.Message.Length)
      {
        c = this.Message[index];
      }
      else
      {
        break;
      }
    }

    return result;
  }

  return null;
}

Some parts that I don't like about my code, and don't know if they're done correctly:

It looks too long/not elegant to me. Specifically, I think the loop could be done more efficiently. I tried different loops, also tried with a line of the form c = this.Message[++index], but then I ran into problems at the end of the string.
I want to get rid the double usage of c = this.Message[index] somehow.
Is it even good style to return null if there is no occurrence of the attribute? (As info: this can happen, but maybe it would be smarter to return string.Empty then?)
Should I use this.Message instead of just Message when it is a public property of the surrounding class?
I also want to get rid of the if statement (and the ugly break) inside the loop if possible, but didn't really find a way yet.

Maybe something along the lines of

while (index < this.Message.Length && c != ',')

would work?

I think that you should include the space before the identifier and the separator after the identifier, that way it will find the right identifier even if the string contains properties in a different order.

If the string contains for example CallName: Peter, NameAlias: Johnny, Name: John you would want the last property when looking for Name rather than the first property because it ends with Name or the value "ias: Johnny" from the second property.

(Including the space before the identifier naturally won't find an identifier if it starts at the first character of the string, but that doesn't seem to be the case with your data.)

You don't need a loop to find the end of the value, you can use IndexOf with a start index.

private string GetValueFromMessage(string identifier) {
  identifier = " " + identifier + ": ";
  int index = this.Message.IndexOf(identifier) + identifier.Length;

  if (index != -1) {
    int index2 = this.Message.IndexOf(",", index);
    if (index2 == -1) {
      index2 = this.Message.Length;
    }
    return this.Message.Substring(index, index2 - index);
  }

  return null;
}

Additionally, you can use .IndexOf(identifier, 0, StringComparison.OrdinalIgnoreCase) if you want to make a case insensetive match on the identifier.

Using this.Message or Message is a matter of convention, neither is clearly better than the other. While this.Message is more specific, Message should usually be clear enough, but you have to be a bit more careful when naming members to avoid conflicts, but on the other hand you should always avoid those conflicts anyway.


Fast way of searching for a string in a text file I have written this code to search for a string in a .txt file. Is it possible to optimize the code so that it searches for the string in fastest manner possible? Assuming the text file would be a large one (500MB-1GB)

I don't want to use regex.

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;

public class StringFinder {

public static void main(String[] args)
{
    double count = 0,countBuffer=0,countLine=0;
    String lineNumber = "";
    String filePath = "C:\\Users\\allen\\Desktop\\TestText.txt";
    BufferedReader br;
    String inputSearch = "are";
    String line = "";

    try {
        br = new BufferedReader(new FileReader(filePath));
        try {
            while((line = br.readLine()) != null)
            {
                countLine++;
                //System.out.println(line);
                String[] words = line.split(" ");

                for (String word : words) {
                  if (word.equals(inputSearch)) {
                    count++;
                    countBuffer++;
                  }
                }

                if(countBuffer > 0)
                {
                    countBuffer = 0;
                    lineNumber += countLine + ",";
                }

            }
            br.close();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    } catch (FileNotFoundException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    System.out.println("Times found at--"+count);
    System.out.println("Word found at--"+lineNumber);
}
}

Fast comes at a price.... code complexity and perhaps readability.

Assuming that your code produces the right results now.... and it is a big assumption because:

it expects the word to be at the beginning/end of the line, or surrounded by spaces (not commas, punctuation, etc.)
it does not look for the word inside another string, it will match 'are', but not 'bare'.

OK, a much faster way (keeping it as Java), is to do the following:

Convert the search string ('are') to a byte-array in the same encoding as the file.
Open a memory-mapped byte-buffer from a File-Channel on the file.
Scan the ByteBuffer, looking for matches to the search byte-array
count the newlines as you go.
close the ByteBuffer

If the file is larger than your memory, you will have to re-position the byte-buffer occasionally. I recommend using a emopry-mapped size of about 4MB plus the size of the search-string. That way you can search the 4MB window, and then start the next window at the next 4mb boundary.

Once you get in to it, it will make sense.

This system will be fast because you will never have to copy the file's data in to Java. Everything will actually happen in the native side of things.

There is a lot to read to make it work.

I would start with a tutorial....

StudyTrails
Yaldix
LinuxTopia

of course, if you want really fast, use grep.

Here is some example code that may get you started:

import java.io.IOException;
import java.nio.MappedByteBuffer;
import java.nio.channels.FileChannel;
import java.nio.channels.FileChannel.MapMode;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardOpenOption;

public class NIOGrep {

    public static void main(String[] args) throws IOException {
        if (args.length != 2) {
            throw new IllegalArgumentException();
        }
        String grepfor = args[0];
        Path path = Paths.get(args[1]);

        String report = searchFor(grepfor, path);

        System.out.println(report);

    }

    private static final int MAPSIZE = 4 * 1024 ; // 4K - make this * 1024 to 4MB in a real system.

    private static String searchFor(String grepfor, Path path) throws IOException {
        final byte[] tosearch = grepfor.getBytes(StandardCharsets.UTF_8);
        StringBuilder report = new StringBuilder();
        int padding = 1; // need to scan 1 character ahead in case it is a word boundary.
        int linecount = 0;
        int matches = 0;
        boolean inword = false;
        boolean scantolineend = false;
        try (FileChannel channel = FileChannel.open(path, StandardOpenOption.READ)) {
            final long length = channel.size();
            int pos = 0;
            while (pos < length) {
                long remaining = length - pos;
                // int conversion is safe because of a safe MAPSIZE.. Assume a reaosnably sized tosearch.
                int trymap = MAPSIZE + tosearch.length + padding;
                int tomap = (int)Math.min(trymap, remaining);
                // different limits depending on whether we are the last mapped segment.
                int limit = trymap == tomap ? MAPSIZE : (tomap - tosearch.length);
                MappedByteBuffer buffer = channel.map(MapMode.READ_ONLY, pos, tomap);
                System.out.println("Mapped from " + pos + " for " + tomap);
                pos += (trymap == tomap) ? MAPSIZE : tomap;
                for (int i = 0; i < limit; i++) {
                    final byte b = buffer.get(i);
                    if (scantolineend) {
                        if (b == '\n') {
                            scantolineend = false;
                            inword = false;
                            linecount ++;
                        }
                    } else if (b == '\n') {
                        linecount++;
                        inword = false;
                    } else if (b == '\r' || b == ' ') {
                        inword = false;
                    } else if (!inword) {
                        if (wordMatch(buffer, i, tomap, tosearch)) {
                            matches++;
                            i += tosearch.length - 1;
                            if (report.length() > 0) {
                                report.append(", ");
                            }
                            report.append(linecount);
                            scantolineend = true;
                        } else {
                            inword = true;
                        }
                    }
                }
            }
        }
        return "Times found at--" + matches + "\nWord found at--" + report;
    }

    private static boolean wordMatch(MappedByteBuffer buffer, int pos, int tomap, byte[] tosearch) {
        //assume at valid word start.
        for (int i = 0; i < tosearch.length; i++) {
            if (tosearch[i] != buffer.get(pos + i)) {
                return false;
            }
        }
        byte nxt = (pos + tosearch.length) == tomap ? (byte)' ' : buffer.get(pos + tosearch.length);
        return nxt == ' ' || nxt == '\n' || nxt == '\r';
    }
}


Disjoint set implementation as linked list I have just 'wrote' a linked list implementation of Disjoint Sets. This is the changed version of Bob Tian's implememnation. Can you take a look and tell me if there is something missing ? For instance memory leaks while destroying nodes. And please judge it also in performance matter.

I would like to change the long NodeAddress[9999] = {0}; to something better but:
this implementation is used for Connected-component labeling so every time I examine the pixel that needs to be added to labels array I add it. But in the end I do not know how many of them will be there and I need some way to access them (in case i would need to use them later on - for instance union it with another label)

#include <iostream>

using namespace std;

long NodeAddress[9999] = {0};
int n=0;

class ListSet {
private:
    struct Item;
    struct node {
        int val;
        node *next;
        Item *itemPtr;
    };
    struct Item {
        node *hd, *tl;
    };

public:
    ListSet() { }
    long makeset(int a);
    long find (long a);
    void Union (long s1, long s2);
};

long ListSet::makeset (int a) {
    Item *newSet = new Item;
    newSet->hd = new node;
    newSet->tl = newSet->hd;
    node *shd = newSet->hd;
    NodeAddress[a] = (long) shd;
    shd->val = a;
    shd->itemPtr = newSet;
    shd->next = 0;
    return (long) newSet;
}

long ListSet::find (long a) {
    node *ptr = (node*)a;
    return (long)(ptr->itemPtr);
}

void ListSet::Union (long s1, long s2) {
    node *ptr1 = (node*)s1;
    node *ptr2 = (node*)s2;

    Item *set2 = ptr1->itemPtr;
    node *cur = set2->hd;

    Item *set1 = ptr2->itemPtr;

    while (cur != 0) {
        cur->itemPtr = set1;
        cur = cur->next;
    }
    //join the tail of the set to head of the input set
    (set1->tl)->next = set2->hd;
    set1->tl = set2->tl;
    delete set2;
}

int main () {

    ListSet a;
    long s1, s2, s3, s4;
    s1 = a.makeset(13);
    s2 = a.makeset(25);
    s3 = a.makeset(45);
    s4 = a.makeset(65);
    cout<<s1<<' '<<s2<<' '<<s3<<' '<<s4<<endl;
    cout<<"a.find(NodeAddress[65]): "<<a.find(NodeAddress[65]) <<endl;
    cout<<"a.find(NodeAddress[45]): "<<a.find(NodeAddress[45]) <<endl;
    cout<<"a.Union(NodeAddress[65], NodeAddress[45]) "<<endl;

    a.Union(NodeAddress[65], NodeAddress[45]);

    cout<<"a.find(NodeAddress[65]): "<<a.find(NodeAddress[65]) <<endl;
    cout<<"a.find(NodeAddress[45]): "<<a.find(NodeAddress[45]) <<endl;
}

#include <assert.h> // for assert
#include <string.h> // for memset

#include <iostream>

using namespace std;

// OMIT these as not very useful. Anyway, don't needlessly expose globals.

long NodeAddress[9999] = {0};
int n=0;

/* I've kept the class names for now, for the sake of recognition, but
   they've each got different problems.

ListSet does a poor job of describing the class' purpose.
Since its purpose is to generate, find, and merge disjoint sets,
call it something like DisjointSetSource.

Item is too generic and not very descriptive of the class' function. It represents a disjoint set, but to reserve the name DisjointSet for its exported opaque equivalent, just call it Set, or since it's represented as a linked list of member nodes, maybe SetNodeList or SetMemberList.

node is also very generic, but it's a little more apt. Also, it's name should be capitalized by convention and for consistency. So, Node is OK, but better might be Member or since it's main features are to refer to its parent list and successor, call it ListMember.
*/

class ListSet {
private:
    typedef int ValueType;
    struct Item;

/* There's a non-trivial smattering of node and item processing cluttering up the ListMember methods. They really want to be full-fledged classes with a constructor, (possibly) destructor, and (possibly) mutators. Item needs write access to node::next and ListMember needs write access to itemPtr, so I'd keep the node members public. */

    struct node {
        ValueType val;
        node *next;
        Item *itemPtr;

// So, add:

        node (const ValueType& a);
        ~node ();
     };

    struct Item {

// So, add:

        Item(node *shd) : hd(shd), tl(shd) {}
        void append(const Item*);
        // returns head node as "sign of life"
        node* remove(node* old);
    private:
        node *hd, *tl;
    };

/* Avoid exposing nodeAddress, especially as a global. It SHOULD use std::vec for dynamic growth or std::map if values are sparse. Wrap this in accessors to stay flexible, especially WRT ValueType, opening the door for (re)templatizing as in the originally posted code. */

private:
    node* nodeAddress[9999];

public:

    void setNodeAddress(const ValueType& a, node* shd) {
        nodeAddress[a] = shd;
    }

    node* getNodeAddress(const ValueType& a) {
        return nodeAddress[a];
    }

    // removeAll requires or maybe IS a third form of nodeAddress accessor

/* Why does a ListSet object ever need to be created?
   When the original implementation used the global NodeAddress as its state,
   all functions could be static and no ListSet instance ever created.
   This would also be true if NodeAddress was a static member.

Yet, it was easy enough to improve on the design by making nodeAddress a normal data member. This provides a useful purpose for multiple ListSet instances.
   Each managing its own disjoint sets, completely independently.
   All the more reason to pull NodeAddress out of the API, where it is just noise. See main.
*/

    ListSet() {
        // zero out nodeAddress in some type appropriate way
        memset(nodeAddress, 0, sizeof(nodeAddress));
    }

    ~ListSet() {
        removeAll();
    }

/* "long" is too general a type to use for "handles" like the return type, here. Better to make up a unique fictitious type:
*/

    class DisjointSet;

    DisjointSet* exportSet(Item* result) { return (DisjointSet*) result; }
    Item* importSet(DisjointSet* input) { return (Item*) input; }

/* Use camelCase or under_scoring to makemultiwordnamesreadable. */

    DisjointSet* makeSet(const ValueType& a);
    DisjointSet* find (const ValueType& a);
    void remove(const ValueType& a);
    void removeAll();

// Use suffix word rather than breaking lowercase convention to get around reserved words

    void unionSets (const ValueType& a1, const ValueType& a2) {
        DisjointSet* s1 = find(a1);
        DisjointSet* s2 = find(a2);
        if (s1 && s2) {
            (void) unionSets(s1, s2);
        }
        // else error?
    }

    DisjointSet* unionSets (DisjointSet* s1, DisjointSet* s2);
};

ListSet::DisjointSet* ListSet::makeSet (const ValueType& a) {
    assert(!getNodeAddress(a));
    node *shd = new node(a);
    Item *newSet = new Item(shd);
    setNodeAddress(a, shd);
    shd->itemPtr = newSet;
    return exportSet(newSet);
}

ListSet::DisjointSet* ListSet::find (const ValueType& a) {
    node* ptr = getNodeAddress(a);
    if (ptr)
        return exportSet(ptr->itemPtr);
    // else error?
    return 0;
}

/* This used to be needlessly confusing with lines like:

Item *set2 = ptr1->itemPtr;
Item *set1 = ptr2->itemPtr;

I'm really still not sure the revision, below maintains the original intent, because of the set1/set2 naming confusion, but it does what the old code did.

Aside from avoiding such confused naming, use of an Item method
makes this code much easier to read. */

ListSet::DisjointSet* ListSet::unionSets (DisjointSet* s1, DisjointSet* s2) {
    Item *set1 = importSet(s1);
    Item *set2 = importSet(s2);

    set2->append(set1);
    delete set1;
}

void ListSet::Item::append (const Item* other) {
    //join the tail of the set to head of the other set
    tl->next = other->hd;
    tl = other->tl;
    for (node* cur = other->hd; cur; cur = cur->next) {
        cur->itemPtr = this;
    }
}

ListSet::node::node (const ValueType& a) : val(a), next(0), itemPtr(0)
{ }

/* Here's mutator code that allows values to be incrementally dropped as operations continue.
Note that after "union(1,2); union(2,3); remove(2);",
1 and 3 are still considered to be in the same set.
*/

// Returns hd as a sign of life -- null if empty/dead.
ListSet::node* ListSet::Item::remove(node* old) {
    if (old == hd) {
        if (old == tl) {
            assert(! old->next);
            return 0;
        }
        assert(old->next);
        hd = old->next;
    } else {
        node* prev;
        for (prev = hd; prev->next != old; prev = prev->next) {
            assert(prev->next);
            ;
        }
        if (old == tl) {
            assert(! old->next);
            //
            tl = prev;
            prev->next = 0;
        } else {
            assert(old->next);
            prev->next = old->next;
        }
    }
    return hd;
}

ListSet::node::~node() {
    if (itemPtr) {
        if (! itemPtr->remove(this)) {
            // Don't leak an empty set.
            delete itemPtr;
        }
    }
}

void ListSet::remove(const ValueType& a) {
    node* ptr = getNodeAddress(a);
    if (ptr) {
        setNodeAddress(a, 0);
        delete ptr;
    }
    // else error?
}

void ListSet::removeAll() {
    assert( 0 ); // TBD
    // This just needs to iterate over nodeAddress
    // and call delete on each remaining node*.
}

/* There's no reason to expose anything other than the original elements
   (raw numbers), the ListSet and the DisjointSets it returned.
   So don't clutter the interface with Item*s disguised as longs,
   node*s disguised (indistinguishably) as longs, or NodeAddress.
   A cleaner, safer API results:

int main () {

    ListSet a;
    ListSet::DisjointSet *s1, *s2, *s3, *s4;
    s1 = a.makeSet(13);
    s2 = a.makeSet(25);
    s3 = a.makeSet(45);
    s4 = a.makeSet(65);

    /* If this really needs to work, either define a stream io operator
       for our DisjointSet type or let it get dumped as a hex value. */
    cout<<s1<<' '<<s2<<' '<<s3<<' '<<s4 <<endl;

    cout<<"a.find(65): "<<a.find(65) <<endl;
    cout<<"a.find(45): "<<a.find(45) <<endl;
    cout<<"a.Union(65, 45) "<<endl;

    // The next line is a safer way to express a.unionSets(s3, s4);
    // which may crash if s3 or s4 had been previously merged/deleted,
    // but it's also a little slower.
    a.unionSets(65, 45);

    cout<<"a.find(65): "<<a.find(65) <<endl;
    cout<<"a.find(45): "<<a.find(45) <<endl;
}


Lazy man's IEnumerable extension verification method I often find myself doing this (or similar) with Lists and other collection types:

if (someList != null && someList.Count > 0)
    //take some action on the list

so I got lazy and wrote the following extension methods to do it for me:

    public static bool IsNullOrEmpty(this IEnumerable self)
    {
        //Convert to collection here because IEnumerable doesn't have a count property
        ICollection c = self as ICollection;
        return c == null || c.Count < 1;
    }

    public static bool IsNotNullOrEmpty(this IEnumerable self)
    {
        return !self.IsNullOrEmpty();
    }

    public static bool IsNotNullAndContains<T>(this IEnumerable<T> self, T item)
    {
        return self.IsNotNullOrEmpty() && self.Contains(item);
    }

Other than being lazy is there anything wrong with this or should I have just stuck with the manual check every time?

Edit:

Based on the answer from Jeroen Vannevel:

I originally had the first extension method (IsNullOrEmpty) defined as such:

public static bool IsNullOrEmpty<T>(this IEnumerable<T> self)
{
    return self == null || self.Count < 1;
}

which eliminated the need for the cast to ICollection, as IEnumerable<T> implements the count property. I removed the type parameter though because I wasn't actually using it for anything. Should I have just kept it like that to prevent the cast and a potential type cast exception?

In my opinion, there shouldn't be a need for this method to begin with. There are very few cases I can think of that would involve passing around a null IEnumerable (or List<T> for that matter). IEnumerable can still express a lack of things to iterate, so you don't often see IEnumerable members returning null. I often see null returned for IEnumerable when the better option would have been to fail or more explicitly inform the caller of an invalid operation. Take this Repository example:

public class Repository<T>
{
    public IEnumerable<T> GetItems()
    {
        bool canConnectToDb = false;
        bool recordsReturned = false;

        // Case 1:
        if(!canConnectToDb)
            return Enumerable.Empty<T>();

        // Case 2:
        if(!canConnectToDb)
            return null;

        // Case 3:
        if(!recordsReturned)
            return null;
    }
}

In "Case 1", the result is misleading. Sure, no records can be returned, but the reason isn't a lack of records to return. Instead, there is an error case that must be handled and an exception should be thrown. In "Case 2", null is being used to show an error state, which is misleading. What was the error? How can I differentiate between a null response, and error handling code using try..catch? In "Case 3", the correct return value would be an empty IEnumerable, not null.

Which leads me to your extension methods...Usually checks like these are trying to cover up  issues rather than failing early and identifying them. For example, lets apply your extension method to refactor the existing Count() extension method:

public static int Count<T>(this IEnumerable<T> iterator)
{
    if(iterator.IsNullOrEmpty())
        return 0;
    else
        // Iterate and count...
}

You just end up perpetrating the issue to create "safe" code that really only hides the issue (that iterator shouldn't have been passed in as null to begin with). The actual implementation probably looks more like:

public static int Count<T>(this IEnumerable<T> iterator)
{
    if(iterator == null)
        throw new ArgumentException("iterator");

    // Iterate and count...
}

This allows it to fail early if it's improperly used, and doesn't make it this methods responsibility to make sure it can always execute given poor conditions.

Lastly, if you do plan on using something like your extension methods, you should know that the extension methods that come with the .NET framework already make performance checks similar to your check if the object is an ICollection. That said, there is no performance check they can do on a custom type that implements IEnumerable, so if you want the count the only option is to iterate through the whole IEnumerable. You don't really need the count though, you just need to know if there is anything being returned, which the Any() extension method does. Instead of iterating through the whole collection, it'll just iterate once and immediately return true if there is anything, which can be a huge performance saver.

A rework of your method:

public static bool IsNullOrEmpty(this IEnumerable iterator)
{
    return iterator == null || !iterator.Any();
}

Maybe I could be convinced by seeing a good application of this extension method, but I can't think of one.

Edit - To address the concern of using another library you can't entirely trust, you have some other options:

1) Use the Facade Pattern to create wrapper objects that do perform to your standards.

2) Treat the issue at the call site to these external libraries and not further into your own code. For example:

var items = BadLibrary.GetItems<T>() ?? Enumerable.Empty<T>();
InternalGoodMethod(items);

public static void InternalGoodMethod(IEnumerable<T> items)
{
    if(items == null)
        throw new ArgumentException("items");

    ....


Stop connecting to a server after n unsuccessful attempts I am trying to connect to a server.
If the connection goes through, then print an apt message.
If the connection does not go through, try for a total of n times. And quit after the nth attempt with an apt message.

I set up a mock server like this -

class Server:
    def __init__(self):
        self.connection_status = random.choice([True, False, False, False])

    def get_connection(self):
        if self.connection_status:
            return "Connected"
        else:
            raise EnvironmentError("Connection Error")

And then I tested it as follows -

def connect_to_server():
    s = Server()
    try:
        return s.get_connection()
    except EnvironmentError as e:
        return str(e)

def main():
    count = 0
    while count < 3:
        connection = connect_to_server()
        if connection == "Connection Error":
            count += 1
            print(connection, count)
            if count == 3:
                print("No more connection attempts")
        else:
            count = 0
            print(connection)

So, after 3 re-connection attempts, the program will terminate.

Is there a better / elegant way to do this?

The smart and polite thing to do when retrying is to wait some random (and possibly increasing) interval between retries.  The last thing you want to do to a failing server or network is to overwhelm it with connections as fast as the clients are able to generate them.

In connect_to_server(), squashing the EnvironmentError into a string is a bad idea.  Just letting the exception propagate would better convey the fact that a failure occurred.  Besides, what if the server really wanted to send the string "Connection Error" as its message?  Changing the client's behaviour based on the payload might even be considered a mild denial-of-service vulnerability.

The idiomatic way to write a counting loop in Python is to use range().

Defining a retry() function would be worthwhile for three reasons:

It makes your code self-documenting
It declutters your main()
You might be able to reuse the retry logic if it's needed elsewhere

One solution is to define a @retry decorator for functions:

from functools import wraps
from random import random
from time import sleep

def retry(max_tries=3, max_retry_interval=2, exceptions=[Exception],
          failure_callback=lambda exc, attempt, max_tries: None):
    """Tries calling the decorated function up to max_tries times until
    it succeeds, and returns its result.

    Any exceptions of the specified types thrown by the decorated function
    will trigger a retry.  The failure_callback is called with each failure,
    with the exception, the attempt number (starting with 0), and max_tries
    max_tries as callback parameters.

    There is a random delay of up to max_retry_interval seconds between
    tries.

    If max_tries attempts all result in failure, then the last encountered
    exception is raised. """
    def decorator(f):
        @wraps(f)
        def retry_wrapper(*args, **kwds):
            exception = None
            for attempt in range(max_tries):
                try:
                    return f(*args, **kwds)
                except Exception as e:
                    if not next((e for cls in exceptions if isinstance(e, cls)), None):
                        # Unexpected type of exception
                        raise
                    failure_callback(e, attempt, max_tries)
                    exception = e
                    if attempt < max_tries - 1:
                        sleep(max_retry_interval * random())
            if exception is not None:
                raise exception
        return retry_wrapper
    return decorator

Then your code would look like:

@retry(3, 5, [EnvironmentError], lambda exc, attempt, max_tries: print("Connection Error"))
def connect_to_server():
    …

def main():
    try:
        connection = connect_to_server()
        print(connection)
    except EnvironmentError as e:
        print(e)
        print("No more connection attempts")

Or, without hard-coding the retry parameters in a decorator:

def connect_to_server():
    …

def main():
    try:
        retry3times = retry(3, 5, [EnvironmentError],
                            lambda _, _, _: print("Connection Error"))
        connection = (retry3times(connect_to_server))()
        print(connection)
    except EnvironmentError as e:
        print(e)
        print("No more connection attempts")


Higher Order Functions in VBA, the dirty way I have been building a small library of VBA functions.  I have a small section with higher order functions that I'd like to share.  It uses strings as delegates/function pointers and Application.Run to actually call the functions.

This isn't by any means a true implementation; just a quick and dirty implementation without connecting external libraries or creating objects for every function.  I haven't found real drawbacks yet. Application.Run always passes arguments by value, which would be a draw back, but is desirable in all of these implementations.  Also not being able to declare lambdas is a buzz-kill.

Public Sub Map(ByVal delegate As String, ByRef sequence As Variant)

    Dim i As Long
    For i = LBound(sequence) To UBound(sequence)

        sequence(i) = Application.Run(delegate, sequence(i))

    Next i

End Sub
Public Function Mapped(ByVal delegate As String, ByVal sequence As Variant) As Variant

    Mapped = sequence
    Map delegate, Mapped

End Function
Public Function Fold(ByVal delegate As String, ByVal sequence As Variant, _
                     ByVal initial_value As Variant) As Variant

    Fold = initial_value

    Dim el As Variant
    For Each el In sequence

        Fold = Application.Run(delegate, Fold, el)

    Next el

End Function
Public Function Reduce(ByVal delegate As String, ByVal sequence As Variant) As Variant

    ' Head and Tail return the first element and the rest, respectively.
    Reduce = Fold(delegate, Tail(sequence), Head(sequence))

End Function
Public Function Compose(ByVal delegates As Variant, ByVal initial_value As Variant) As Variant

    Compose = initial_value

    Dim delegate As Variant
    For Each delegate In delegates

        Compose = Application.Run(delegate, Compose)

    Next delegate

End Function

Things I like

This idea.
You've explicitly declared ByVal and ByRef.
All methods are short, clear, and concise.

Things to watch out for

All of these functions take variants in as parameters, by necessity. Client code runs the risk of passing in a sequence that isn't enumerable. These really need a comment and maybe a VB_Description attribute describing what kind of arguments the functions expect.
You also may want to consider handling runtime error 438 "Object doesn't support Method" for the functions that use For Each loops. There's not much you can do other than return a more meaningful error message though.
If you change the loop in Map to a For Each and manually increment i, all of the functions will be compatible with collections as well as arrays.

Nitpicks

Underscores have a special meaning. They indicate event procedures. So I would change initial_value to initialValue.
While I quickly understood that el stood for element, there's no reason not to just spell it out.


Words counter with a hash My problem is the following: I wrote a method, which counts words and returns a hash. There is a Ruby idiom which was found by me in some of the forums discussions, which I don't understand.

Here is the whole code:

text = "bla bla blahh blahh"
def count_words(string)
  inp = string.downcase.scan(/\b[\'a-z]+\b/i).inject(Hash.new 0){|c,w| c[w]+=1;c }
  return inp
end
puts count_words(text)

Here is the idiom: inject(Hash.new 0){|c,w| c[w]+=1;c }

My concrete questions are the following:

1) How it understands when we should add 1 to the particular key?

2) As far as I understand "inject" method, "c" is some sort of counter. So how does it happen that we write c[w]?

It is about inject initial value when you give Hash.new(0)
and call c[w] += 1 in block it is expanded to c[w] = c[w] + 1 as we said
to Hash initializer if it has no key then key will set to 0 when
first appeared.
c here is an accumulator and instance of inject initial value.
accumulator value will be set to return value of the block after
every turn. we are returning c because of this. it can be type of
any object. (1..10).inject(0) {|a,x| a + x } for example it is
Fixnum here.


Get closest Entity from the Player on a 2D battlefield I am building a little game in using Unity, and recently made an algorithm to get the closest Entity to the player on the battlefield. I made a working sample in RexTester (it is not the real code, but the algorithm is exactly the same).

I am interested in the GetCloserEntityFromPlayer function. Basically, it loops through all the entities and find the closest one. The function is defined as below:

public static Entity GetCloserEntityFromPlayer(){
    List<Entity> temp = new List<Entity>();
    temp.AddRange(allies);
    temp.AddRange(enemies);

    //assume the first is the closest, and compare to the others
    Entity aux = temp[0];
    int distance = Math.Abs((aux.X + aux.Y) - (player.X + player.Y));

    for(int i = 1; i < temp.Count; i++)
    {
        //in my game, the distance is the absolute difference
        //between the sum of both Entities x and y values
        int temp_distance = Math.Abs((temp[i].X + temp[i].Y) - (player.X + player.Y));
        if(temp_distance < distance)
        {
            aux = temp[i];
        }
    }
    return aux;
}

When the Entity number starts growing, the game seems to get a little slow, with FPS dropping. I was thinking of maintaining the Entities list ordered, so I would help the branch predictor.

First of all, this function would be more idiomatic using a foreach loop:

Entity closest = null;
int distance = int.MaxValue;

for (var other in entities)
{
    int d = Math.Abs((other.X + other.Y) - (player.X + player.Y));
    if(d < distance)
    {
        closest = other;
    }
}
return closest;

  When the Entity number starts growing, the game seems to get a little slow, with FPS drop. I was thinking of maintaining the Entities list ordered, so I would help the branch predictor, I don't know...

If you can keep entities sorted by their e.X + e.Y value,
then you could significantly speed up the search using binary search:
you will be able to find the position of the player in the list,
and check only its two neighbors to find the closest.

However, keeping the entities sorted might be costly as well.
I don't know if that's the case here,
it depends on the rest of your application that you did not include in the post.
My guess is that it's going to be so costly that you will not gain any benefit at all.

On the other hand,
if you need to find the closest entity for not only one player,
but multiple entities,
and if you perform that step for all entities at once,
then you will benefit from performing the sort right before you look for the closest neighbors.
This is different from keeping the entities sorted at all times.
You would only sort right before it's needed.


LeetCode: Spiral Matrix II C# https://leetcode.com/problems/spiral-matrix-ii/
Please review for performance, how can this code run faster

  Given a positive integer n, generate a square matrix filled with
  elements from 1 to n^2 in spiral order.

  Example:

Input: 3
Output:
[
 [ 1, 2, 3 ],
 [ 8, 9, 4 ],
 [ 7, 6, 5 ]
]

using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace ArrayQuestions
{

    /// <summary>
    /// https://leetcode.com/problems/spiral-matrix-ii/
    /// </summary>
    [TestClass]
    public class SpiralMatrix2Test
    {
        [TestMethod]
        public void Example3x3Test()
        {
            int size = 3;
            int[][] expected =
            {
                new[]{1,2,3},
                new [] {8,9,4},
                new []{7,6,5}
            };
            int[][] res= SpiralMatrix2.GenerateMatrix(size);
            for (int i = 0; i < size; i++)
            {
                CollectionAssert.AreEqual(expected[i],res[i]);
            }
        }
}

    public class SpiralMatrix2
    {
        public static int[][] GenerateMatrix(int n)
        {

            int startCol = 0;
            int endCol = n - 1;
            int startRow = 0;
            int endRow = n - 1;
            int num = 1;
            int[][] res = new int[n][];
            for (int i = 0; i < n; i++)
            {
                res[i] = new int[n];
            }

            if (n == 0)
            {
                return res;
            }

            while (startCol <= endCol && startRow <= endRow)
            {
                for (int i = startCol; i <= endCol; i++)
                {
                    res[startRow][i] = num++;
                }
                startRow++;// already did this row skip to the next one

                for (int i = startRow; i <= endRow; i++)
                {
                    res[i][endCol] = num++;
                }

                endCol--;// already did the last col move back one col

                for (int i = endCol; i >= startCol; i--)
                {
                    //keep in mind this is a spiral we can be in a line we are not suppose to touch the values
                    //only in the upper half of the matrix we need
                    if (startRow <= endRow)
                    {
                        res[endRow][i] = num++;
                    }
                }
                endRow--;

                for (int i = endRow; i >= startRow; i--)
                {
                    //we need to print the numbers only in the left half of the matrix
                    if (startCol <= endCol)
                    {
                        res[i][startCol] = num++;
                    }
                }

                startCol++;
            }
            return res;

        }
    }
}

If n is zero then the loop

while (startCol <= endCol && startRow <= endRow)

does nothing, which means that the test

if (n == 0)

is not necessary.

Here

for (int i = endCol; i >= startCol; i--)
{
    //keep in mind this is a spiral we can be in a line we are not suppose to touch the values
    //only in the upper half of the matrix we need
    if (startRow <= endRow)
    {
        res[endRow][i] = num++;
    }
}

the condition of the if-statement is a loop invariant, so that it can be done outside of the loop:

if (startRow <= endRow)
{
    for (int i = endCol; i >= startCol; i--)
    {
        res[endRow][i] = num++;
    }
}

Even if the C# compiler is smart enough to recognize the loop invariant and reorders the statements, the latter variant would be clearer to the reader of your code. The same applies to the next loop in your function.

Then note that startRow/startCol and endRow/endCol have the same value at the start of the loop body, and the tests

if (startRow <= endRow) ...
if (startCol <= endCol) ...

can fail only in the last iteration. Therefore one can move that case out of the main loop, use only two variables for the first and last row/column, and write the inner loops in a symmetric fashion:

int start = 0;
int end = n - 1;

while (start < end)
{
    for (int i = start; i < end; i++)
    {
        res[start][i] = num++;
    }
    for (int i = start; i < end; i++)
    {
        res[i][end] = num++;
    }
    for (int i = end; i > start; i--)
    {
        res[end][i] = num++;
    }
    for (int i = end; i > start; i--)
    {
        res[i][start] = num++;
    }

    start++;
    end--;
}

if (start <= end)
{
    res[start][start] = num;
}