Creating a searchable collection of rooms I'm trying to learn C++ again, trying to do all the "modern" stuff (e.g., avoid pointers in favor of references) and I found that I have some trouble understanding how references and lifetimes work. I'm using clang with -std=c++11.

I want to have a RAII-class, RoomList that basically wraps a map<int, Room>. On construction of RoomList, it will create all the Rooms (imagine loading them from a data file or so) and the map, and on destruction it should free the map and all Rooms.

This code works, but I don't fully understand all of it (I've done a lot of "randomly trying stuff" to get it to work). (GIST is here)

The Room class:

class Room {
public:
  explicit Room(int id, const std::string& name) : m_id(id), m_name(name) {}
  const std::string& GetName() const { return m_name; }
  const int& GetEnterCount() const { return m_enterCount; };
  void OnEnter() { ++m_enterCount; }
private:
  std::string m_name; // should this be const?
  int m_id; // should this be const?
  int m_enterCount = 0;
};

The RoomList class:

class RoomList {
public:
  RoomList();
  ~RoomList() = default; // Is this needed? Rule of three?
  RoomList(const RoomList&) = delete; // Don't want copies
  RoomList& operator=(RoomList const&) = delete;
  const Room& GetRoom(int index);
private:
  // Should this be <int, Room> or <int, Room&>?
  std::map<int, Room> m_rooms;
};

RoomList::RoomList() {
  // Read all the rooms, e.g., from a data file
  // What is the lifetime of the Room I'm creating?
  // Room doesn't have copy or move ctors
  // Also, I'm not initializing m_rooms - what is its lifetime?
  m_rooms.emplace(0, Room(0, "Start Room"));
}

// Returning a reference to const Room - no one should be able to modify Room, except by calling room.OnEnter()
// room.OnEnter() should modify the Room inside the map (Individual Rooms are basically Singletons)
const Room& RoomList::GetRoom(int index) {
  auto result = m_rooms.find(index);
  if(result != m_rooms.end())
  {
    // result is a local var - is result->second valid after return?
    return result->second;
  }

  // Considering Optional<Room>
  // https://github.com/akrzemi1/Optional/
  throw "Invalid Index";
}

main.cpp:

int main() {
  RoomList rooms;
  auto room = rooms.GetRoom(0);
  std::cout << "Name: " << room.GetName() << std::endl;
  return 0;
}

I left some comments, but basically the things I'm confused about:

I'm not initializing the m_rooms map - will it be default initialized to live as long as the rooms variable in main.cpp is in scope?
I have a map of Rooms, but I want to return Room references. Will returning a Room have any negative side effects? (e.g., is there some automatic creation of a wrapper object or even a copy of Room going on?)
Is result->second even still valid after a return? result is a local variable that should die when the function ends, but I'm returning what second points to, so that should be good?
Would I need to implement ~RoomList to make sure all the Rooms are freed? Or does std::map::emplace own the constructed Room?
If I wanted to return something by value, how would I need to allocate it? For example, imagine I've added this to Room, wouldn't I return a stack-allocated variable?

This code works, but I think it shouldn't:

class Room {
public:
  // ...
  std::string GetRoomDescription();
}

std::string Room::GetRoomDescription()
{
  // What's the lifetime of "result"?
  // In C, this would be stack-allocated and dead
  // when the function returns.
  std::stringstream result;
  result << "ID: " << m_id << " - Name: " << m_name;
  return result.str();
}

// main.cpp
std::cout << "Room: " << room.GetRoomDescription() << std::endl;

There are some issues here:

  auto room = rooms.GetRoom(0);

Here you do copy the Room because auto does not imitate the 'reference part'. To bind a reference to the internal map entry, you need to both return by reference and assign to a reference type. Based on your gist, I assume you would think that if you then call room.OnEnter, the Room is also updated in the RoomList. This is not the case, because it is a copy. Therefore you would want to use:

  auto& room = rooms.GetRoom(0);

instead. But for that to work, RoomList::GetRoom should return a non-const reference, so that you can actually modify it. In turn there should also be a const RoomList::GetRoom that also returns a const reference.

However, there should also be a non-const RoomList::GetRoom that returns a non-const reference, so that you can actually call non-const methods on the returned rooms.

To avoid running into that issue - you might want to delete the copy constructors and assignment operators of Room. If you further wanted to enforce that all Room objects are owned only by RoomList you could even make the Room constructor private and friend RoomList.

  I'm not initializing the m_rooms map - will it be default initialized to live as long as the rooms variable in main.cpp is in scope?

Yes

  I have a map of Rooms, but I want to return Room references. Will returning a Room have any negative side effects? (e.g., is there some automatic creation of a wrapper object or even a copy of Room going on?)

Not for std::map. You can delete the copy constructor and assignment operator or Room, that will ensure you make no copies somewhere else in the code also.

See https://stackoverflow.com/a/6442829/620382 for a listing of iterator/reference invalidations in standard containers.

  Is result->second even still valid after a return? result is a local variable that should die when the function ends, but I'm returning what second points to, so that should be good?

Yes. result points to the internal value_type of the map.

  Would I need to implement ~RoomList to make sure all the Rooms are freed? Or does std::map::emplace own the constructed Room?

RoomList::m_rooms own the constructed Rooms. More technically speaking:

m_rooms.emplace(0, Room(0, "Start Room"));

Here a temporary Room object is created, which is then moved into the std::pair<int, Room> that is construed inside of the map. In C++14 you can use std::piecewise_construct to even skip the temporary Room object. But as long as you have a move constructor, which you have by default, you are fine.

  If I wanted to return something by value, how would I need to allocate it? For example, imagine I've added this to Room, wouldn't I return a stack-allocated variable?

If you do not declare the return value to be a reference (which you do not in your example), then it is returned by copy. But the C++ rules and compilers are good so that you will either get a move (cheaper than actual copy) or even omitting it alltogether when possible.

  // Considering Optional

If you document it properly, it would be actually fine to return a raw pointer here, with nullptr meaning that it is not available. The raw pointer (similar to the raw reference) does not imply ownership. You just need to make sure the caller does never reference it without checking. optional (also available in boost and soon in the standard).

The funny thing about your code is, that you have randomly written proper and safe code. IMHO this is a nice property especially about modern C++. There is actually no explicit resource allocation (like new) in your code - when you allocate resources implicitly, through member variables, C++ will behave nicely by default and also implicitly release the resources.

  std::string m_name; // should this be const?
  int m_id; // should this be const?

Yes, it would be a good idea to make those const.

const int& GetEnterCount() const { return m_enterCount; };

I would consider it a uncommon to return an int by const reference here (as opposed to a simple copy). I don't have a strong argument for or against, but lets just take note that std::vector::size doesn't return an const int& either.

  RoomList& operator=(RoomList const&) = delete;

You should put the const before RoomList for consistency.
You also might want to explicitly default or delete the move constructor and assignment operators. I would say neither would hurt.

In Room, you list m_id and m_name in a different order in the initializer list. This can lead to confusion because the initialization order follows the order from member declaration (m_name then m_id). Therefore this is a compiler warning.

So the tip is, compile with maximal warnings, e.g. -Wall -pedantic for gcc. Also it can be helpful to run your code with valgrind to check for any leaks.


First JavaScript game: Tic Tac Toe I'm pretty new to programming in general.  I work with MySQL at my job but my knowledge of scripting language is rudimentary at best.  I've been learning JavaScript via Codecademy but I got bored and set out on my own.  I made this Tic Tac Toe game, which is purely text-based and predictably boring.  I was just wondering if anyone has suggestions for improving the code, especially in terms of following programming best practices.  Also any advice for turning this into a visual game would be greatly appreciated, especially with HTML5.

var board = [0, 0, 0, 0, 0, 0, 0, 0, 0];
var gameOver = true;
var check = true;
var i = Math.floor(Math.random()*2);

var transInput = function(p){
    switch(p) {
        case "A3": return 0;
        case "B3": return 1;
        case "C3": return 2;
        case "A2": return 3;
        case "B2": return 4;
        case "C2": return 5;
        case "A1": return 6;
        case "B1": return 7;
        case "C1": return 8;
        default  : console.log("Entry Unknown!");
    }
};

var transOutput = function(p){
    switch(p) {
        case  1: return "x";
        case 10: return "o";
        default: return " ";
    }
};

var drawBoard = function(){
    console.log("3 " + transOutput(board[0]) + "|" + transOutput(board[1]) + "|" + transOutput(board[2]));
    console.log(" -----");
    console.log("2 " + transOutput(board[3]) + "|" + transOutput(board[4]) + "|" + transOutput(board[5]));
    console.log(" -----");
    console.log("1 " + transOutput(board[6]) + "|" + transOutput(board[7]) + "|" + transOutput(board[8]));
    console.log(" A B C");
};

drawBoard();

if(i === 0){
    console.log("You go first!");
} else {console.log("Computer goes first!");}

while (gameOver){
    if(i === 0){
        var playerMove = transInput(prompt("Your move!"));
        var check = true;
        while(check){
            if(board[playerMove] === 0){
                board[playerMove] = 1;
                drawBoard();
                i = 1;
                check = false;
            } else {
                playerMove = transInput(prompt("Try again."));
            }
        }
    } else if(i === 1){
        console.log("Computer's turn...");
        var check = true;
        while(check){
            var compMove = Math.floor(Math.random()*9);
            if(board[compMove] === 0){
                board[compMove] = 10;
                drawBoard();
                i = 0;
                check = false;
            }
        }
    }
    var scores = [
        board[0] + board[1] + board[2],
        board[3] + board[4] + board[5],
        board[6] + board[7] + board[8],
        board[0] + board[3] + board[6],
        board[1] + board[4] + board[7],
        board[2] + board[5] + board[8],
        board[0] + board[4] + board[8],
        board[2] + board[4] + board[6]
    ];

    var scoreSum = 0;

    for(var k = 0; k < board.length; k++){
        scoreSum += board[k];
    }

    for(var j = 0; j < scores.length; j++){
        if(scores[j] === 3){
            console.log("You Win!");
            gameOver = false;
            break;
        } else if(scores[j] === 30){
            console.log("You Lose!");
            gameOver = false;
            break;
        }
    }

    if(
        gameOver === true && (
            scoreSum === 54 ||
            scoreSum === 45
        )
    ){
        console.log("Draw!");
        gameOver = false;
    }
}

Your code's pretty good actually. I don't see any of the typical beginner's syntactical pitfalls. Kudos! (Though I would prefer gameOver, given its name, to be false while the game's running).

Structurally, there are many ways to build it. I'd probably split things into objects or constructors (aka classes).

For instance, I'd make the board an object with its own methods for marking a square, checking for wins, resetting, etc.. Similarly, the players and the turn-taking could be modelled. It may be overkill for a simple Tic-Tac-Toe game, but the resulting code will be simpler to read (I think). It's basically the same strategy as creating views in MySQL to encapsulate complex/reusable queries.

For instance something like this (not a complete implementation)

// Constructor
function Board() {
  this.squares = new Array(9);
}

// Prototype functions (i.e. methods)
Board.prototype = {
  // get number of marked squares
  occupied: function () {
    var i, l, count = 0;
    for( i = 0, l = this.squares.length ; i < l ; i++ ) {
      count += this.squares[i] ? 1 : 0;
    }
    return count;
  },

  // get a row
  row: function (index) {
    var i = index * 3;
    return this.squares.slice(i, 3);
  },

  // get a column
  column: function (index) {
    var i, l, column = [];
    for( i = index, l = this.squares.length ; i < l ; i += 3 ) {
      column.push(this.squares[i]);
    }
    return column;
  },

  // get diagonal squares
  // index 0: from top-left
  // index 1: from top-right
  diagonal: function (index) {
    var step = index ? 2 : 4,
        squares = [];
    index = index ? 2 : 0;
    while( squares.length < 3 ) {
      squares.push(this.squares[index]);
      index += step;
    }
    return squares;
  }
};

Edit: The above code has some issues. One was a straight-up typo on my part (fixed now). The other is a little more subtle: slice() will ignore undefined elements in an array. So (new Array(3)).slice(0) → [] instead of [undefined, undefined, undefined]. This could obviously cause trouble, as the array will be shorter than one might assume. So the array should either be seeded with values (zeros for instance), or the slice-call should be replaced with something similar to what column() is doing in the above: Looping and pushing values, even if they're undefined.

Then you can call new Board() to get a, well, a new board. That board, in turn, provides functions for determining the state of play. (Note that most of these functions can be simplified using array functions like forEach and win-conditions can be checked with reduce or a homemade array-sum function. However, that's an exercise for the reader).

With regard to visual output, you could start by simply "printing" to <pre> element on the page instead of the console. A minor step, yes, but just to get the hang of DOM manipulation/interaction. Actually, using your code, I've done that here. Of course, the real next step is using elements for each square, have them react to clicks, etc..

For that, you could consider simply letting the DOM itself handle the state. I.e. rather than a separate array of values, simply use the elements themselves and their attributes to determine if/how a square is marked.

For now, though, I'll stop here. See if you get something useful out of a more object oriented approach, before moving on the HTML GUI. The reason I'm suggesting this, is that the DOM is object oriented already, so getting the game logic on that footing will likely make creating the GUI easier/simpler. At least you're bound to learn something :)


How to write save block code like this more elegant in Ruby? I have a class Creator which will execute a block code for a number of times. I'm not sure how to write this in a more elegant way in Ruby.

class Creator
    attr_accessor :block
    def self.create(&block)
        @block = block
        return self
    end

    def self.for(number)
        0.upto(number) {
            block.call
        }
    end
end

And this is how I call it which I want to stay like this.

Creator.create{
  SomeModel.create!(@attr)
}.for(30)

You want instance methods, not class methods.  With the current code, you would get:

hello = Creator.create { puts "Hello" }
goodbye = Creator.create { puts "Goodbye" }
hello.for(1)   # Prints "Goodbye"

Assuming you don't want block to be changed afterwards, I would change attr_accessor to attr_reader.  I don't see much reason to even expose a reader, though — it seems that doing so would only lead to mischief.

class Creator
  # For compatibility with the old API
  def self.create(&block)
    return self.new(&block)
  end

  def initialize(&block)
    @block = block
  end

  def for(number)
    number.times { @block.call }
  end
end

This is just a syntactic sugar wrapper for blocks.  Perhaps a more generic name than Creator might be appropriate.  It might also be simpler to call number.times { block.call } directly.

Standard indentation for Ruby is two spaces.


Small method that runs various processes I have a project which runs from a db table - it's purpose is to run processes written in various ways.

Currently it deals with python scripts, vbs scripts and console exe files.

It works but I'm wondering if there any obvious refactors?

private Process aProcess;

public void runProcess(string aPath,string aName,string aFiletype)
{

  aProcess = new Process();
  string stInfoFileName = null;
  string stInfoArgs = null;
  bool blUseShell = false;

  //if it is to call a python script
  if(aFiletype == "py")
  {
    stInfoFileName = @"python.exe";
    stInfoArgs = string.Format("\"{0}{1}{2}\"",aPath,Path.DirectorySeparatorChar,aName);
  }

  //if it is to call console app exe
  if(aFiletype == "exe")
  {
    stInfoFileName = aName;
    stInfoArgs = string.Format("\"{0}{1}{2}\"",aPath,Path.DirectorySeparatorChar,aName);
    blUseShell = true;
  }

  //if it is to run vbs script
  if(aFiletype == "vbs")
  {
    stInfoFileName = "cscript";
    stInfoArgs = string.Format("/B \"{0}{1}\"",@aPath,@aName);
  }

  aProcess.StartInfo.FileName = stInfoFileName;
  aProcess.StartInfo.Arguments = stInfoArgs;

  aProcess.StartInfo.WorkingDirectory = aPath;           //<< does not seem to need the extra quotes

  aProcess.StartInfo.UseShellExecute = blUseShell;
  aProcess.StartInfo.RedirectStandardOutput = false;

  aProcess.Start();

  this.aProcess.WaitForExit();
  this.aProcess.Dispose();

}

I'd first start with naming conventions. Prefixes are usually avoided and method name is PascalCase:

public void RunProcess(string path, string fileName, string fileExtension)

Honestly I'd accept just one parameter with full path and I'd split it inside this method but you might not be able to change this then I won't comment about it.

First I'd extract a method to create the StartupInfo leavinig in this method only invocation and parameters validation:

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

// More validations...

using (var process = Process.Start(CreateStartupInfo(path, fileName, fileExtension)))
{
    process.WaitForExit();
}

Then, moving to CreateStartupInfo() implementation, I'd first drop those if in favor of a Dictionary or a plain list because it's much easier to extend and it might come from a configuration file (no need to change code to support more file types.)

For the moment keep the if in-place but consider:

Do not perform string comparison with ==. It's a current culture aware case sensitive comparison, it's not obvious and it's not probably what you want. Use String.Equals() specifying the right StringComparer/StringComparison value.
Do not manually build paths, there is Path.Combine() for that. Let's see an example where for simplicity I have as input the full file path.

Given this holder:

sealed class KnownFileType
{
    public KnownFileType(string type,
        bool useShellExecute,
        Func<string, string> interpreter,
        Func<string, string> interpreterCommandLine)
    {
        // ...
    }

    public string Type { get; }
    public Func<string, string> Interpreter { get; }
    public Func<string, string> InterpreterCommandLine { get; }
    public bool UseShellExecute { get;  }
}

We can declare a list of known file types like this (note that his can be outside the method and static):

var fileTypes = new KnownFileType[]
{
    new KnownFileType(".py", false, x => "python.exe", x => $"\"{x}\""),
    new KnownFileType(".exe", true, x => Path.GetFileName(x), x => $"\"{x}\""),
    new KnownFileType(".vbs", false, x => "cscript.exe", x => $"/B \"{x}\""),
};

Do you need to add new known file type? Add one line here. If those values come from a configuration file then you might want to use String.Format() instead (be careful if configuration can be tampered.) Because I dislike escape sequences I'd introduce an helper method:

private static string Quote(string text) => '"' + text + '"';

Using it I may simplify above code to:

new KnownFileType(".py", false, x => "python.exe", x => Quote(x))

To create the ProcessStartupInfo you just need to:

string extension = Path.GetExtension(fullPath);
var fileType = fileTypes.First(x => String.Equals(x.Type, extension, StringComparison.InvariantCultureIgnoreCase));

return new ProcessStartInfo
{
    UseShellExecute = fileType.UseShellExecute,
    FileName = fileType.Interpreter(fullPath),
    Arguments = fileType.InterpreterCommandLine(fullPath),
    WorkingDirectory = Path.GetDirectoryName(fullPath)
};

Note that here I used a plain array but if you opt for a Dictionary<string, KnownFileType> (with the proper case insensitive comparer for keys) then code may be slightly simplified to:

var fileType = fileTypes[Path.GetExtension(fullPath)];
// ...


Calculator for scoring dives in a competition I was looking for a light weight coding challenge and found this:

  In the sport of diving, seven judges award a score between 0 and 10, where each score may be a floating-point value. The highest and lowest scores are thrown out and the remaining scores are added together. The sum is then multiplied by the degree of difficulty for that dive. The degree of difficulty ranges from 1.2 to 3.8 points. The total is then multiplied by 0.6 to determine the diver’s score.
  Write a computer program that inputs a degree of difficulty and seven judges’ scores, and outputs the overall score for that dive. The program should ensure that all inputs are within the allowable data ranges.

So, I tried to write some modern c++, and I am looking for suggestions to modernize it or is it so basic that this isn't a concept.

#include "pch.h"
#include <iostream>
#include <sstream>
#include <string>

constexpr auto JUDGES = 7;
constexpr auto FUDGE_FACTOR = 0.6f;
constexpr auto MIN_DIFFICULTY = 1.2f;
constexpr auto MAX_DIFFICULTY = 3.8f;
constexpr auto MIN_SCORE = 0.f;
constexpr auto MAX_SCORE = 10.f;

using std::cout;
using std::endl;
using std::cin;
using std::string;
using std::stringstream;

float getInput(float low, float high)
{
    string line;
    float ret;
    while (std::getline(cin, line))
    {
        stringstream ss(line);
        if (ss >> ret && ss.eof()) {
            if (ret>=low && ret<=high) return ret;
        }
        cout << "invalid input, try again: ";
    }
    return -1.0;
}

int main()
{
    float difficulty;
    float total{ 0. };
    float min{ MAX_SCORE };
    float max{ MIN_SCORE };

    cout << "please enter degree of difficulty for the dive: ";
    difficulty = getInput(MIN_DIFFICULTY, MAX_DIFFICULTY);
    for (int i = 0; i < JUDGES; ++i) {
        cout << "please enter the judge #"<<i+1<<"'s score: ";
        auto score = getInput(MIN_SCORE, MAX_SCORE);
        if (score < min) min = score;
        if (score > max) max = score;
        total += score;
    }
    total -= (min + max);
    total *= difficulty * FUDGE_FACTOR;
    cout << "The dive scored: " << total << endl;
}

Any thoughts/suggestions will be welcome.

Variables should be initialized with proper values when declared (if not, they should be declared as close to the point of usage as practical). This way there's no need to initialize them to some invalid value, or risk leaving them uninitialized. (difficulty and ret are the main offenders here, but total, min and max should be declared just before the loop using them).
I'm not sure there's much point in the using std::foo;. It's probably as much work to keep track of what you're using and update the declarations than just typing std:: where necessary. But it's better than using namespace std;, so that's good. :)
The prompt message to the user asking for input could be passed as a parameter to getInput and printed as part of the function.
The min and max values could also be shown when requesting an input.

Consider splitting the main function into two phases: getting input, calculating the result. This may make the program a little easier to understand.
Certain things could be split off into separate functions. e.g. String to float conversion and calculating the overall score.

These last two are really just suggestions though; the original code is very good.

Edited code, with some alternative ways of doing things:

#include <iostream>
#include <sstream>
#include <string>
#include <vector>
#include <algorithm>
#include <cassert>

constexpr auto JUDGES = 7;
constexpr auto FUDGE_FACTOR = 0.6f;
constexpr auto MIN_DIFFICULTY = 1.2f;
constexpr auto MAX_DIFFICULTY = 3.8f;
constexpr auto MIN_SCORE = 0.f;
constexpr auto MAX_SCORE = 10.f;

bool toFloat(std::string const& string, float& value)
{
    std::stringstream ss(string);
    return (ss >> value && ss.eof());
}

bool isInRange(float value, float low, float high)
{
    return (value >= low && value <= high);
}

float getFloatInput(float low, float high, std::string const& prompt)
{
    while (true)
    {
        std::cout << prompt << " [" << low << ", " << high << "]: ";

        std::string line;
        std::getline(std::cin, line);

        auto value = 0.f;
        if (toFloat(line, value) && isInRange(value, low, high))
            return value;

        std::cout << "invalid input!\n";
    }

    assert(false);
    return -1.0f;
}

std::vector<float> getJudgesScores()
{
    auto scores = std::vector<float>();

    for (auto i = 0; i != JUDGES; ++i)
        scores.push_back(getFloatInput(MIN_SCORE, MAX_SCORE, "score #" + std::to_string(i + 1)));

    return scores;
}

float calculateOverallScore(std::vector<float> const& scores, float difficulty)
{
    auto min = MAX_SCORE;
    auto max = MIN_SCORE;
    auto sum = 0.f;

    for (auto score : scores)
    {
        min = std::min(min, score);
        max = std::max(max, score);
        sum += score;
    }

    sum -= (min + max);

    return sum * difficulty * FUDGE_FACTOR;
}

int main()
{
    auto difficulty = getFloatInput(MIN_DIFFICULTY, MAX_DIFFICULTY, "difficulty");
    auto judgesScores = getJudgesScores();
    auto overallScore = calculateOverallScore(judgesScores, difficulty);

    std::cout << "The dive scored: " << overallScore << std::endl;
}


Updating database based on selected check boxes I posted the original question here, but I think it would be nice to have someone else's eye on this code for improving it.

public override void Update(UserProfileDto item)
{
    base.Update(item);

    var memberIntersestIds = _memberInterestService.GetAllPlusDeleted();

    var dbIds = memberIntersestIds.Select(p => p.InterestSubCategoryId);
    var union = dbIds.ToList().Union(item.InterestIds);

    if (item.InterestIds != null)
    {
        var guids = union as Guid[] ?? union.ToArray();
        foreach (var id in guids)
            if (_memberInterestService.IsSubInterestExist(id))
                _memberInterestService.Delete(id, item.Id, true);

        foreach (var id in guids)
        {
            if (!_memberInterestService.IsSubInterestExist(id))
                _memberInterestService.Add(new MemberInterestDto
                {
                    UserProfileId = item.Id,
                    InterestSubCategoryId = id
                });
            else
                _memberInterestService.Delete(id, item.Id, false);
        }
    }
    else
        foreach (var memberInterest in memberIntersestIds)
            _memberInterestService.Delete(memberInterest.InterestSubCategoryId, item.Id, true);
}

MemberIntersetService

public class MemberInterestService : IMemberInterestService
{
    public IQueryable<MemberInterest> Items { get { return _dbSet.Where(x => x.IsDeleted == false); } }

    public MemberInterestService(AppDbContext dbContext) : base(dbContext)
    {
        _dbSet = dbContext.MemberInterests;
    }

    public bool IsSubInterestExist(Guid subInterestId)
    {
        return _dbSet.Any(p => p.InterestSubCategoryId == subInterestId);
    }

    public bool IsSubInterestExistInItem(Guid subInterestId)
    {
        return Items.Any(p => p.InterestSubCategoryId == subInterestId);
    }

    public void Delete(Guid interestId, Guid userProfileId, bool delete)
    {
        var item = _dbSet.Single(x => x.InterestSubCategoryId == interestId && x.UserProfileId == userProfileId);

        //soft delete
        item.IsDeleted = delete;

        _dbSet.Update(item);
    }

    public IList<MemberInterestDto> GetAllPlusDeleted()
    {
        return _dbSet.Select(x => Mapper.Map<MemberInterest, MemberInterestDto>(x))
            .ToList();
    }
}

OMITTING CURLY BRACES ?

Don't do that a single line statements like this are acceptable even tho they are still not recommended you should always have {} :

if (Foo is Bar)
    return "Foo is Bar";
else
    return "Foo is not Bar";

But this is a huge red flag for bad code style :

foreach (var id in guids)
{
    if (!_memberInterestService.IsSubInterestExist(id))
        _memberInterestService.Add(new MemberInterestDto
        {
            UserProfileId = item.Id,
            InterestSubCategoryId = id
        });
    else
        _memberInterestService.Delete(id, item.Id, false);
}
foreach (var id in guids)
    if (_memberInterestService.IsSubInterestExist(id))
        _memberInterestService.Delete(id, item.Id, true);

If your statement needs to be written on few lines You should have {} e.g in your foreach as it's body contains more than a single line, same goes for your if's it's kinda hard to read it that way and follow the flow of your program.

Aside from that I would move dbIds && union inside your if (item.InterestIds != null) as you are performing potentially unnecessary operations, they will be used only if the if is triggered anyway :

if (item.InterestIds != null)
{
    IEnumerable<Guid> dbIds = memberIntersestIds.Select(p => p.InterestSubCategoryId);
    IEnumerable<Guid> union = dbIds.ToList().Union(item.InterestIds);
    Guid[] guids = union as Guid[] ?? union.ToArray();
    //..
}

This line I assume you have because you are avoiding multiple enumeration

Guid[] guids = union as Guid[] ?? union.ToArray();

But why not call .ToArray() on union instead, there is no need to create new variable here, you are always working with guids and never using union itself as IEnumerable<> :

IEnumerable<Guid> dbIds = memberIntersestIds.Select(p => p.InterestSubCategoryId);
IEnumerable<Guid> guids = dbIds.ToList().Union(item.InterestIds).ToArray();

This part of your method :

foreach (var id in guids)
{
    if (_memberInterestService.IsSubInterestExist(id))
    {
        _memberInterestService.Delete(id, item.Id, true);
    }
}

foreach (var id in guids)
{
    if (!_memberInterestService.IsSubInterestExist(id))
    {
        _memberInterestService.Add(new MemberInterestDto
        {
            UserProfileId = item.Id,
            InterestSubCategoryId = id
        });
    }
    else
    {
        _memberInterestService.Delete(id, item.Id, false);
    }
}

It's weird, and so is the .Delete() method. First you have a foreach that remove duplicates from the database

foreach (var id in guids)
{
    if (_memberInterestService.IsSubInterestExist(id))
    {
        _memberInterestService.Delete(id, item.Id, true);
    }
}

But apparently you don't trust your methods and you keep searching if there is any item that exist in your db with a matching ID from the set you've previously iterated and already removed. What's the point ?

But why you deleting the items from the db only the first time ? What's different ? It's the same ID collection.

Since there is no obvious reason why are you doing this, I would suggest to combine those in a single foreach :

foreach (var id in guids)
{
    if (_memberInterestService.IsSubInterestExist(id))
    {
        _memberInterestService.Delete(id, item.Id, true);
    }
    else
    {
        _memberInterestService.Add(new MemberInterestDto
        {
            UserProfileId = item.Id,
            InterestSubCategoryId = id
        });
    }
}

With all of those changes your code should  look like this :

public void Update(UserProfileDto item)
{
    MemberInterestService _memberInterestService = new MemberInterestService();
    IList<MemberInterestDto> memberIntersestIds = _memberInterestService.GetAllPlusDeleted();

    if (item.InterestIds != null)
    {
        IEnumerable<Guid> dbIds = memberIntersestIds.Select(p => p.InterestSubCategoryId);
        IEnumerable<Guid> guids = dbIds.ToList().Union(item.InterestIds).ToArray();
        foreach (var id in guids)
        {
            if (_memberInterestService.IsSubInterestExist(id))
            {
                _memberInterestService.Delete(id, item.Id, true);
            }
            else
            {
                _memberInterestService.Add(new MemberInterestDto
                {
                    UserProfileId = item.Id,
                    InterestSubCategoryId = id
                });
            }
        }
    }
    else
    {
        foreach (var memberInterest in memberIntersestIds)
        {
            _memberInterestService.Delete(memberInterest.InterestSubCategoryId, item.Id, true);
        }
    }
}

Your Delete method might be better of with a .FirstOrDefault() instead  of .Single() unless you really want to verify there is exactly one item, .FirstOrDefault() should be faster in most cases as it will return whenever the first item is found, where .Single() will always be O(n) operation because it needs to verify if there is exactly one occurrence.

Probably more can be improved, but again more context is required.


begin() vs rbegin(): how can I avoid duplication? Can the display function be re-written to avoid duplication of the std::for_each loop?

#include <iostream>
#include <set>
#include <algorithm>

struct Widget
{
    int id;
    Widget(int i) : id(i){}
};

namespace std
{
    template<>
    struct less<Widget *> {
    public:
        bool operator ()(Widget *const& a, Widget *const& b) const {
            return  (a->id < b->id);
        }
    };
}

void display(const std::set<Widget *>& wList, bool order)
{
    if (order) {
        std::for_each(wList.begin(), wList.end(), [](const Widget* x) {
            std::cout << x->id << std::endl;
        });
    }
    else {
        std::for_each(wList.rbegin(), wList.rend(), [](const Widget* x) {
            std::cout << x->id << std::endl;
        });
    }
}

int main()
{

    std::set<Widget *> s;

    s.insert(new Widget(1));
    s.insert(new Widget(2));
    s.insert(new Widget(5));
    s.insert(new Widget(3));
    s.insert(new Widget(9));
    s.insert(new Widget(7));

    display(s, false);
}

I would re-write the display to take iterators.

template<typename I>
void display(I begin, I end)
{
    std::for_each(begin, end, [](const Widget* x) {
         std::cout << x->id << std::endl;
    });
}

Then you can pass any container that contains Widget pointers to the method. Also solves the problem about ordering. You pass the appropriate iterators for the task.

But my main problem. Is why are you storing pointers in a container?
Who owns the pointers? If it is the container then you are doing it wrong as the destructor will not clean up (nor will replacing elements do the correct thing). For this you should use boost::ptr_set (in <boost/ptr_container/ptr_set_adapter.hpp>) or std::set<std::unique_ptr<Widget>> if the container is not taking ownership then it should hold references to the object std::set<std::ref<Widget>> there are other alternatives but it depends intirely on the semantics you intend to achieve.

But pointers in containers is a no-no (in general). Better have a darn good reason.


Callback/Promises implementation for a boolean check Currently I have the following callback system:

var saveTask = function(err, result) {
    if (err) return callback(err, result);

    var newid = mongoose.Types.ObjectId();
    var task = new Task({
        _id: newid,
        taskname: req.body.name,
        teamid: req.body.team,
        content: req.body.content,
        creator: req.user.userId
    });

    task.save(function (err) {
        if (!err) {
            log.info("New task created with id: %s", task._id);
            return callback(null, task);
        } else {
            if(err.name === 'ValidationError') {
                return callback('400', 'Validation error');
            } else {
                return callback('500', 'Server error');
            }
            log.error('Internal error(%d): %s', res.statusCode, err.message);
        }
    });
};

if (req.body.team) {
    valTeam.isMember(req.body.team, req.user._id, function (err, done) {
        if (err) {
            saveTask('403', 'Not the owner or member of this team');
        } else {
            saveTask(null, true);
        }
    });
} else {
    saveTask(null, true);
}

valTeam.isMember

exports.isMember = function(teamid, userid, callback) {
    Team.find({'_id':teamid, $or:[{'creator': userid }, {'userlist': { $in : [userid]}}]}, function(err, result) {
            if (err) return err;
            console.log(result);
            if (!result.length)
                return callback('404', false);
            else
                return callback(null, true);
    });
}

In short, if team is sent by POST, I'm checking if the user is member of that ID in valTeam.isMember. Am I using the correct syntax and best method to call back my saveTask function to save the task if the user is part of the team?

This code currently works, but I feel like there should be an easier way to do it? How could I use a promise to achieve the same thing?

Thanks in advance.

Assuming you're using Node.JS v6.x, or lower but with babel (this is ES6 syntax):

function saveTask(req) {
  // Always return a promise.
  return new Promise((resolve, reject) => {
    // Build a task.
    const _id = mongoos.Types.ObjectId();
    const task = new Task({
      _id, // Not a mistake. it's valid ES6 code.
      taskname: req.body.name,
      teamid: req.body.team,
      content: req.body.content,
      createror: req.user.userId
    });

    // Save it.
    task.save((err) => {
      if (!err) {
        log.info('New task created with id: %s', _id);
        // Resolve promise with task.
        // If you don't actually need task, you could
        // resolve(req); // to pass req along.
        resolve(task);
      } else {
        log.error('Internal error(%d): %s', err.code, err.message);
        // Reject promise with err.
        reject(err);
      }
    });
  });
}

// Utility function to run a specific code sequence.
const postTask = (req) => valTeam.isMember(req).then(saveTask);
// If `valTeam` is not a module library, then this might be better:
// const postTask = (req, valTeam) => valTeam.isMember(req).then(saveTask);
// and call with `postTask(req, valTeam)`

And here's the isMember() function

/**
 * Utility function to replace:
 *
 * if (req.body.team) { ... }
 *
 * with a promise.
 */
const hasTeamId = (req) => new Promise((resolve, reject) => {
  if (req.body.team) {
    // Has team id. Pass `req`
    resolve(req);
  } else {
    reject(new Error('No team id provided'));
  }
});

// Function isMember() always returns a promise
module.exports.isMember = function (req) {
  const teamid = req.body.team;
  const userid = req.user._id;

  // Internal utility function to check if a given team it exists.
  const checkTeamExists = (req) => {
    // Execute query
    Team.findOne({'_id': teamid, $or:[{'creator': userid }, {'userlist': { $in : [userid]}}]}, function(err, result) {
      // On callback, solve the promise.
      if (!err && result) {
        // No error, and at least 1 document in results.
        // Resolve promise.
        resolve(req);
      } else {
        // 1) Either error, or
        // 2) No error, but empty results => Create a new error.
        // Reject promise.
        reject(err || new Error('404 - Not found'));
      }
    });
  };

  return hasTeamId(req).then(checkTeamExists);
};

Also, if you have Mongoose 4.x+ which supports Promises, better use this:

// ALTERNATIVE FOR MONGOOSE v4.x+
// Function isMember() always returns a promise
module.exports.isMember = function (req) {
  const teamid = req.body.team;
  const userid = req.user._id;

  // Internal utility function to check if a given team it exists.
  const checkTeamExists = (req) => {
    // Execute query ( In mongoose 4.x+, queries return Promises )
    return Team.findOne({'_id': teamid, $or:[{'creator': userid }, {'userlist': { $in : [userid]}}]})
      .then((result) => {
        // On callback, solve the promise.
        if (result) {
          //at least 1 document in results.
          return req;
        } else {
          // not found.
          throw new Error('404 - Not found');
        }
      });
  };

  return hasTeamId(req).then(checkTeamExists);
}

Finally, use the above like so:

postTask(req)
  .then((task) => {
    // Successfully saved task.
    // Handle task, if needed, otherwise just remove the `then()` branch.
  })
  .catch((err) => {
    // Some error occured, either in `isMember` or in `saveTask`
    // Handle error, if needed, otherwise just remove the `catch()` branch.
  });


Compare files, returning first differing line Exercise 7-6 from the K&R book.
Compares 2 files, printing the line number and content with the first dissimilarity.
What am I doing that could be improved?

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

#define MAXLINE 100

/* compare: compare files, printing first differing line */
int main(int argc, char *argv[])
{
    FILE *fp1, *fp2;
    char line1[MAXLINE], line2[MAXLINE];
    int i;
    char *prog = argv[0];

    /*require 2 files to comp*/
    if (argc != 3)
    {
        fprintf(stderr, "Incorrrect usage: got %d arguments, expected 3", argc);
        exit(1);
    }
    /*attempt to open files*/
    if (!( (fp1 = fopen(*++argv, "r")) && (fp2 = fopen(*++argv, "r")) ))
    {
        fprintf(stderr, "%s: can't open %s\n", prog, argv[1] || argv[2]);
        exit(2);
    }
    /*read line from each file*/
    for (i = 0; fgets(line1, MAXLINE, fp1) && fgets(line2, MAXLINE, fp2); i++)
    {
        if (strcmp(line1, line2))
        {
            /* print line # and line that differed */
            fprintf(stdout, "Line %d\n%s: %s\n%s: %s", i, argv[1], line1, argv[2], line2);
            break;
        }
    }
}

Perfect code does not exist, it's all a matter of opinion, but I would write the handling of the program arguments differently.
The first argument is the name of the program so you dont expect 3 arguments, but just 2 from the user's perspective.
Also, I would change the *++argv x2 into argv[1] and argv[2] to make more clear what you mean with the program arguments.

And I would explicitly check the retrun value of strcmp to make it more clear that you are checking for inequality.

So not if (strcmp()) but if (strcmp() != 0)

    if (argc != 3)
    {
        fprintf(stderr, "Incorrrect usage: got %d arguments, expected TWO", argc);
        exit(1);
    }
    /*attempt to open files*/
    if (!( (fp1 = fopen(argv[1], "r")) && (fp2 = fopen(argv[2], "r")) ))
    {
        fprintf(stderr, "%s: can't open %s\n", prog, argv[1] || argv[2]);
        exit(2);
    }
    /*read line from each file*/
    for (i = 0; fgets(line1, MAXLINE, fp1) && fgets(line2, MAXLINE, fp2); i++)
    {
        if (strcmp(line1, line2) != 0)
        {
            /* print line # and line that differed */
            fprintf(stdout, "Line %d\n%s: %s\n%s: %s", i, argv[1], line1, argv[2], line2);
            break;
        }
    }


Flattening array in Javascript I have implemented a solution to flatten an array of multiple levels in Javascript. I am just a newbie so forgive any mistakes. Could someone give me any tips to improve the code?

function streamrollArray(arr) {
    var copyArr = arr;
    var to_return = [];

    while (true) {
        if (copyArr.length <= 0) { return to_return; }
        var head = copyArr[0];
        to_return = to_return.concat(getValues(head));
        //Chop head off since we have got all the values from it.
        copyArr = copyArr.splice(1);
    }
}

function getValues(arr) {
    var copyArr = arr;
    var to_return = [];

    if (!Array.isArray(arr)) {
        return arr;
    }

    while (true) {
        //A head can contain multiples arrays inside it.
        if (copyArr.length <= 0) { return to_return; }
        var head = copyArr[0];
        to_return = to_return.concat(getHead(head));
        copyArr = copyArr.slice(1);
    }
}

function getHead(head) {
    //If its an array we break into it getting the head and keep checking.
    while (Array.isArray(head)) {
        head = head[0];
    }
    return head;
}

I would think that this might be a problem best solved via recursion.

For example:

function flattenArray(arr) {
    var result = [];

    arr.forEach(function($val) {
        if(Array.isArray($val)) {
            result = result.concat(flattenArray($val));
        } else {
            result.push($val);
        }
    });

    return result;
}

Also consider a more meaningful name for your function like flattenArray vs. steamrollArray.  This better conveys what the function does.  Look at your question title for example. You used Flattening array to describe your code not Steamrolling array.


Form for choosing settings I've created a form to choose the settings: These settings are automatically saved in a file from the game client in this form:

["Setting"] = {
    ["track"] = "Spell Reflect",
    ["duration"] = {
        ["minimum"] = {
            ["enabled"] = 1,
            ["value"] = 3,
        },
        ["maximum"] = {
        },
    },
    ["stack"] = {
        ["minimum"] = {
        },
        ["maximum"] = {
            ["enabled"] = 1,
            ["value"] = 4,
        },
    },
}

To save the settings in this form I used this function:

function UnitScan_onAdvancedOptionClose()
    if not (db["duration"]) then
        db["duration"] = {}
        db["duration"]["minimum"] = {}
        db["duration"]["maximum"] = {}
    end
    db["duration"]["minimum"].enabled = minduration.cbutton:GetChecked()
    db["duration"]["minimum"].value = tonumber(minduration.ebox:GetText())
    db["duration"]["maximum"].enabled = maxduration.cbutton:GetChecked()
    db["duration"]["maximum"].value = tonumber(maxduration.ebox:GetText())

    if not (db["stack"]) then
        db["stack"] = {}
        db["stack"]["minimum"] = {}
        db["stack"]["maximum"] = {}
    end
    db["stack"]["minimum"].enabled = minstack.cbutton:GetChecked()
    db["stack"]["minimum"].value = tonumber(minstack.ebox:GetText())
    db["stack"]["maximum"].enabled = maxstack.cbutton:GetChecked()
    db["stack"]["maximum"].value = tonumber(maxstack.ebox:GetText())
end

Where

db = The "Setting" table
GetChecked() = Returns whether the check button is checked
GetText() = Returns the edit box's text contents

Could you help me to make the code less repetitive and more elegant?

One big thing we need to do is to sort everything into arrays

I should point out that

db.stack

is the same as

db["stack"]

Firstly, shift GetChecked() and GetText() (sidenote functions names usually start with lowercase) into an array

local get = { maximum = GetChecked, minimum = GetText }

Stuff that needs to be called during min should be given the "minimum" key. Same with "maximum".

Sort everything else out...

local func = { minimum = "cbutton", maximum = "ebox"}
local keys = {"enabled", "value"}
local minmax = {"minimum", "maximum"}
local db = {}
local stuff = {"stack", "duration"}
local minormax = { minimum = "min", maximum = "max" }

Lets take a break for a second, and create the important array

for _, each in ipairs(stuff) do
  db[each] = {}
  for _, it in ipairs(minmax) do
    db[each][it] = {}
  end
end

This creates

db["stack"]["minimum"]
db["stack"]["maximum"]
db["duration"]["minimum"]
db["duration"]["maximum"]

Now first, we iterate over the stack and duration keys

for _, param in ipairs(stuff) do

end

Then we iterate over the min max values

for _, param in ipairs(stuff) do
  for _, min_max in ipairs(minmax) do

  end
end

Then we iterate over enabled and value

for _, param in ipairs(stuff) do
  for _, min_max in ipairs(minmax) do
    for _, key in ipairs(keys) do

    end
  end
end

To recap, for both the stack and duration, we are checking min and max, and for both min and max, we are checking enabled and value. So your function looks something like this:

db[param][min_max][key] = _G[minormax[min_max]..param][func[min_max]]:get[min_max]()

Finally, we iterate over all the min entries to tonumber them

for _, each in ipairs(db) do
  for _, it in ipairs(db[each]["minimum"]) do
    db[each]["minimum"] = tonumber[each]["minimum"]
  end
end

Let's consolidate everything:

local func = { minimum = "cbutton", maximum = "ebox"}
local get = { maximum = GetChecked, minimum = GetText }
local keys = {"enabled", "value"}
local minmax = {"minimum", "maximum"}
local db = {}
local stuff = {"stack", "duration"}
local minormax = { minimum = "min", maximum = "max" }

for _, each in ipairs(stuff) do --Create the multidimensional array
  db[each] = {}
  for _, it in ipairs(minmax) do
    db[each][it] = {}
  end
end

for _, param in ipairs(stuff) do --The important function
  for _, min_max in ipairs(minmax) do
    for _, key in ipairs(keys) do
      db[param][min_max][key] = _G[minormax[min_max]..param][func[min_max]]:get[min_max]()
    end
  end
end

for _, each in ipairs(db) do --Tonumber all the min entries
  for _, it in ipairs(db[each]["minimum"]) do
    db[each]["minimum"] = tonumber([each]["minimum"])
  end
end

Sorry for the long answer. Hopefully it works, because with such a complicated program my poor brain had to work it out slowly and methodically. The different steps should help you understand my train of thought and help you flush out any bugs (if there are any)

I cannot guarantee this solution will work (I may have messed up somewhere), and it looks really repetitive in itself but here you go.

EDIT: I noticed some of the code is missing. I'll fill in the rest when I have time

EDIT 2: I changed some of the code after reviewing it again.

EDIT 3: Thank you Etan Reisner for pointing out the mistakes I made. I've edited the code.

EDIT 4: You can actually combine the array creation loop and the main body loop if you want:

for _, param in ipairs(stuff) do --The important function
  db[param] = {}
  for _, min_max in ipairs(minmax) do
    db[param][min_max] = {}
    for _, key in ipairs(keys) do
      db[param][min_max][key] = _G[minormax[min_max]..param][func[min_max]]:get[min_max]()
    end
  end
end


Reverse String : Recursion We have to reverse char[] using recursion.

Input: ["H","a","n","n","a","h"]
Output: ["h","a","n","n","a","H"]

package codeMonk;

public class RevereseString {

    public  char[] reverseStr(char[] reversedString, int startIndex , int lastIndex, int midIndex) {

        // Base Condition
        if(startIndex > midIndex || midIndex >= lastIndex){
            return reversedString;
        } else {
            char storeChar = reversedString[startIndex];
            reversedString[startIndex] = reversedString[lastIndex -1];
            reversedString[lastIndex -1 ] = storeChar;
            return reverseStr(reversedString, ++startIndex , --lastIndex, midIndex);
        }
    }

    public static void main(String[] args) {
        char[] oldArray = {'A',' ','m','a','n',',',' ','a',' ','p','l','a','n',',',' ','a',' ','c','a','n','a','l',':',' ','P','a','n','a','m','a'};
        int midIndex = 0 + (oldArray.length - 0)/2 ;
        RevereseString reverseStr = new RevereseString();
        char [] newArray = reverseStr.reverseStr(oldArray, 0 , oldArray.length, midIndex);
        for(char ch : newArray) {
            System.out.print(ch +" ");
        }
    }

}

This code works fine, but how can I improve it and reduce the lines of code?

You are not using any stored data in the RevereseString [sic] object, so you do not actually need to create the object.  Changing reverseStr() to a static method, (optionally private as well) and you can remove RevereseString reverseStr = new RevereseString(); to save one line of code.

There is no need for a midIndex.  You just need to reverse characters until the startIndex and lastIndex meet or cross.  Thus you can remove int midIndex = 0 + (oldArray.length - 0)/2 ; and save another line of code.

Finally, you are not returning a new array; you are returning the original oldArray and assigning that object to newArray.  In other words, as main() finishes, oldArray == newArray is true.  You have reversed the oldArray in place; there is no need for a return value from reverseStr(), so you can eliminate at least another two lines of code.


Making multiple http request Angular2 I need to get some ids from contacts and contactgroups to be able to collect the data. My example works but i dont know if this is the way to go.

getIdArray() {
let contactIds = [];
let groupIds = [];
let contacts = this.http.get('Contacts/').map(res => res.json());
let contactGroup = this.http.get('ContactGroups/').map(res =>res.json());
Observable.forkJoin([contacts, contactGroup]).subscribe(res => {

  for (let b of res[0].contacts) {
    console.log(b.contactId);
    contactIds.push(b.contactId)
  }

  for (let b of res[1].contactgroups) {
    console.log(b.contactgroupId)
    groupIds.push(b.contactgroupId)
  }
  this.loadContacts(contactIds);
  this.loadGroups(groupIds);

 });
}

loadContacts(contactId) {
console.log("ids" + JSON.stringify(contactId));
Observable.forkJoin(
  contactId.map(
    i => this.http.get('Contacts/' + i)
      .map(res => res.json())
  )
 ).subscribe(data => {
  for (let b of data) {
    console.log(b);
  }
});
}

loadGroups(groupIds){
console.log("ids" + JSON.stringify(groupIds));
Observable.forkJoin(
  groupIds.map(
    i => this.http.get('ContactGroups/' + i)
      .map(res => res.json())
  )
).subscribe(data => {
  for (let b of data) {
    console.log(b);
  }
});
}

Instead Of Preface

My answer is split into three main parts.
The first two explain how to make the small-ish code changes to improve readability and decrease fragility.
The third part covers more of a design aspect which you may find important.

A. Tactical Improvements

1. TypeScript is about Types

Use explicit types wherever possible. This will:

Enable compile time errors (very helpful for nasty typo detection);
Help a lot during the code refactor (especially when it comes to signature changes). However, don't get too confident -- certain things are not being caught by the compiler.

In the code provided below, notice that all the signatures declare parameter types and return type.

1.1. Little Type Hint Trick

Often, frameworks or libraries return objects of type any. When this happens, the compiler loses the narrow type information making the code less reliable. As an example, we can see that response.json() in our Observables' map part is doing exactly that. Since we know, what object the remote service API is supposed to return us back, we can immediately cast that expression to the specific type and make the compiler our helper again: <ContactGroup>response.json().

It's important to understand though, that in runtime there is no guarantee that the return object is actually of the same type as the type we cast to. In other words, this cast is practically helpful, but is sometimes misleading and should be used carefully.

2. Master JavaScript and TypeScript idioms

TypeScript is a superset of JavaScript, i.e. whatever is syntactically correct in JS is also correct in TS.

Therefore, instead of looping through the items and writing them to console one by one, you can use the spread operator console.log('Contact id list', ...response.contacts), where response.contacts is an array.
Another thing is the var/let/const. As a rule, I start with const and switch to let only when feel it necessary or pragmatic. This approach forces me to think in a way more functional programming way which I find less error prone compared to imperative style. A thing (variable) does not have to be a constant in physical, mathematical, or business-logic sense of this word like Pi, Epsilon, Gravitational constant, etc. to be declared as const. It's just an instruction to the compiler the you don't want this thing to be reassigned down the road. This practice prevents accidental assignments (source of bugs) and pushes towards cleaner code.

I am very new to JavaScript myself, therefore it is hard for me to notice any other non-idiomatic pieces of code.

B. Slightly Different Code

Please notice that this is not a refactoring since I don't know what your models exactly look like.

    private contacts: Contact[] = [];
    private contactGroups: ContactGroup[] = [];

    loadContactsAndContactGroupsTogether(): void {
      Observable
        .forkJoin([
          this.http.get('Contacts/').map(response => response.json()),
          this.http.get('ContactGroups/').map(response => response.json())
        ])
        .subscribe(
          results => {
            const contactsListResponse = <ContactsListResponse>results[0];
            const contactGroupsListResponse = <ContactGroupsListResponse>results[1];

            this.loadContacts(contactsListResponse.contacts);
            this.loadGroups(contactGroupsListResponse.contactgroups);
          },
          error => console.error(error));
    }

    loadContacts(contactIdList: string[]): void {
      Observable
        .forkJoin(
          contactIdList.map(
            contactId => this.http.get(`Contacts/${contactId}`).map(response => <Contact>response.json())
          )
        ).subscribe(
          data => this.contacts = data,
          error => console.error(error)
        );
    }

    loadGroups(groupIdList: string[]): void {
      Observable
        .forkJoin(
          groupIdList.map(
            groupId => this.http.get(`ContactGroups/${groupId}`).map(response => <ContactGroup>response.json())
          )
        ).subscribe(
          data => this.contactGroups = data,
          error => console.error(error)
        );
    }

  }

Here's the assumed structure of your models

class ContactsListResponse {
  contacts: string[];
}
class ContactGroupsListResponse {
  contactgroups: string[];
}
class Contact { }
class ContactGroup { }

C. Strategic Improvements

One question you may want to ask yourself whenever assessing a system or its part is: Does it scale [well]?

There's an issue with the way we load data. If Contacts/, ContactGroups/, Contacts/{id}, or ContactGroups/{id} API or their combination return(s) enormous amount of data, our single page application (SPA) is very likely going to screw up:

The application user experience cay suffer if we load too many records (scrolling through more than a few dozens of records is rarely enjoyable).
The application responsiveness cay be at risk if we load even more data. The numbers vary a lot, but I've seen SPAs that show noticeable lags while displaying "just 300 rows of data". That was the result of many things including heavy client-side calculation, sub-optimal code, and other...
The application can become unstable (may freeze or even crash the browser). If the contact/contactGroups API(s) does not limit the response at all, and returns many megabytes of data, SPA will attempt to consume whatever memory is needed to accommodate all the records.

All of these scenarios are failures from the business perspective which only vary in scale. Here are a few techniques you may want to consider as countermeasures (you can search for more).

Client-side filtering or Infinite scrolling be helpful for keeping the UX enjoyable. The infinite scrolling may be not very easy to implement very efficiently. Also, both techniques are unlikely to help with the responsiveness and stability issues in worst cases.

Server-side filtering ("search") and Pagination are a couple of effective ways to guarantee that the amount of data given to the client has a controllable limit. These things require either the API changes or client-side changes or both.

Disclaimer

I apologize in advance if my answer is too broad -- your question does not include a concrete set of aspects you'd like to get covered in review and I tried to point out some small and some bigger things.


Object.extends in JS I built an easy inheritance implementation for JavScript that works similar to other OO languages and wanted some feedback on it. This works perfectly how I'd like it to, but I've read a lot of varying opinions about using __proto__ and was wondering if there's a better way to do this.

jsFiddle

/**
 *
 * Extensible Prototype
 *
 */
Object.prototype.extends = function(obj){
        this.__proto__ = new obj;

        return this;
};

/**
 *
 * Parent Object
 *
 */
var Person = function(name){

        this.name = name;

        this.sayName = function(){
                alert(this.name);
        };

        return this;
};

/**
 *
 * Child Object
 *
 */
var Programmer = function(name){
        this.extends(Person);

        this.name = name;
        this.title = "Software Engineer";

        this.sayTitle = function(){
                alert(this.title);
        };
};

var eric = new Programmer('eric');
eric.sayName();
eric.sayTitle();

Not really that bad, unless you consider the fact that __proto__ is non-standard (but is used in most browsers). While some developers argue that __proto__ isn't going away soon, I'd rather go for a more "standard-ish" approach.

The usual way is to just use prototypes like they were designed:

// Person class
var Person = function(){
  this.foo = 'hello';
  this.bar = 'world';
};
Person.prototype = Object.create(Object.prototype);
// Since we just clobbered prototype, we return the constructor
Person.prototype.constructor = Person;
// Add in methods
Person.prototype.read = function(){/*...*/};

// Programmer class
var Programmer = function(){
  // This is to run this object through the parent constructor so that it gets the properties.
  Person.apply(this,arguments);

  this.right = 'bob';
  this.left = 'alice';
};

// Then "inherit" the parent methods
Programmer.prototype = Object.create(Person.prototype);
Programmer.prototype.constructor = Programmer;
Programmer.prototype.readCode = function(){/*...*/};

console.log(new Person());
console.log(new Programmer());

Now it's pretty ceremonious at this point, all that prototype stuff. You can skip all that if you create functions that automate that. Some libraries do the same thing, with sugar:

var ChildClass = BaseClass.extend();

One problem with this style of encapsulation is that your methods are defined per instance rather than shared. This will eat up memory:

function Class(){
  this.foo = function(){};
}

var instance1 = new Class();
var instance2 = new Class();
console.log(instance1.foo === instance2.foo); // false

Sometimes "forcing" private properties are just more hassle than originally intended. Besides, using the dev tools, your code is open to modification anyways. What most libraries do is use "pseudo-private", by prefixing the property with _. Technically, these properties are public, but it's a common programming convention telling the developer it's private.


Switch function in Python I wrote this switch function for Python, as there isn't a built-in one and most of the Googlable ones doesn't execute code, but only return something (I admit I haven't looked much further than a single Stack Oveflow page to see what people use, but a lot of the solutions are unnecessarily complex):

def switch(variable, **cases):
    for case in cases.keys():
       if variable is case:
           exec(cases[case])

I use **cases, due to this syntax:

def switch(variable, **cases):
    for case in cases.keys():
       if variable is case:
           exec(cases[case])

some_string = "a"
switch(some_string,
       b="print('It is b!')",
       a="print('It is a!')")

some_string = "b"
switch(some_string,
       b="print('It is b!')",
       a="print('It is a!')")

some_string = "c"
switch(some_string,
       b="print('It is b!')",
       a="print('It is a!')")

Can this be optimized/made more pythonic?

The most Pythonic way to do is to define a (reusable) dictionary. Most often this dictionary contains callables, but not necessarily.

The least efficient way to do it, though, is to loop over each possibility as you do. It computes in \$O(n)\$ rather than \$O(1)\$ as a dictionnary can offer. And avoid the capability of short-circuit a succession of if...elif...elif...else can offer.

So, at the very least, use something like

def switch(value, **cases):
    try:
        statement = cases[value]
    except KeyError:
        pass
    else:
        exec(statement)

However, I find that defining the various statements at each call is innefficient. As I said, it's better to be able to define a reusable dictionary. I would also advise against the use of exec and make the function more generic; letting the user decide what to do with the retrived value:

def switch(value, cases, default=None):
    try:
        return cases[value]
    except KeyError:
        return default

Usage being:

PRINT_STUFF = {
    'a': lambda: print("It's a!"),
    'b': lambda: print("It's b!"),
}

switch('a', PRINT_STUFF, lambda: None)()
switch('b', PRINT_STUFF, lambda: None)()
switch('c', PRINT_STUFF, lambda: None)()

But way... This try..except reminds me of something... Oh the dict.get method. So there is really no need for switch:

PRINT_STUFF.get('a', lambda: None)()
PRINT_STUFF.get('b', lambda: None)()
PRINT_STUFF.get('c', lambda: None)()

Or, if you trully insist on using strings rather than real callable objects:

PRINT_STUFF = {
    'a': 'print("It\'s a!")',
    'b': 'print("It\'s b!")',
}

exec(PRINT_STUFF.get('a', 'pass'))
exec(PRINT_STUFF.get('b', 'pass'))
exec(PRINT_STUFF.get('c', 'pass'))


Heaviest Stone algorithm time complexity I implemented the heaviest stone algorithm, but I am not sure about its time complexity. I'm doing sort which is O(nlogn), but it' inside a while loop.
Problem:
We have a collection of stones, each stone has a positive integer weight.
Each turn, we choose the two heaviest stones and smash them together.  Suppose the stones have weights x and y with x <= y.  The result of this smash is:
If x == y, both stones are totally destroyed;
If x != y, the stone of weight x is totally destroyed, and the stone of weight y has new weight y-x.
At the end, there is at most 1 stone left.  Return the weight of this stone (or 0 if there are no stones left.)
Example:
Input: [2,7,4,1,8,1]
Output: 1
Explanation:
We combine 7 and 8 to get 1 so the array converts to [2,4,1,1,1] then,
we combine 2 and 4 to get 2 so the array converts to [2,1,1,1] then,
we combine 2 and 1 to get 1 so the array converts to [1,1,1] then,
we combine 1 and 1 to get 0 so the array converts to [1] then that's the value of last stone.

My solution:
var lastStoneWeight = function(stones) {
    if(!stones || stones.length === 0)  return 0;
    if(stones.length === 1) return stones[0];

    stones.sort((a,b) => b-a);

    while(stones.length > 1) {
        const x = stones[0];
        const y = stones[1];
        stones.splice(0, 2);
        if(x !== y) {
            stones.push(x-y);
            if(stones.length === 1) return stones[0];
            stones.sort((a,b) => b-a);
        }
    }

    return stones.length > 0 ? stones[0] : 0;
};

By the way, is there a way to have a better performance? Maybe not sorting?
Thanks

Issue(s)

You sort descending, but then take and assign the heaviest stone to x and the second heaviest stone to y whereas in your problem you state "...stones have weights x and y with x <= y".
Code isn't as DRY (Don't Repeat Yourself) as it could be.

Suggestions

Provide stones a default/initial empty array value.
Sort descending and assign const y = stones[0]; and const x = stones[1]; and push new value stones.push(y - x); to match problem. (Or normal sort ascending and take from array end, more on this later)
Use array destructuring to assign to x and y since array::splice returns an array of spliced out values.
Sort once at the beginning of each iteration.
Remove extraneous array length checks, let the while-loop condition handle it.

Code
const lastStoneWeight = (stones = []) => {
  if (!stones || !stones.length) return 0;

  while (stones.length > 1) {
    stones.sort((a, b) => a - b);
    const [x, y] = stones.splice(-2);
    if (x !== y) stones.push(y - x);
  }
  return stones[0] || 0;
};

If you can use Optional Chaining and Nullish Coalescing
const lastStoneWeight = (stones = []) => {
  if (!stones?.length) return 0;

  while (stones.length > 1) {
    stones.sort((a, b) => a - b);
    const [x, y] = stones.splice(-2);
    if (x !== y) stones.push(y - x);
  }
  return stones[0] ?? 0;
};

Here I use the default sort (ascending) and splice off the last two elements, removes need to shift entire forward 2 indices.
Using a Heap Data Structure
Since you ask about improved performance and the going suggestion is to use a heap/priority queue. This is a very similar implementation.
const lastStoneWeightHeap = (stones = []) => {
  if (!stones?.length) return 0;

  const heap = new PriorityQueue(); // <-- uses equivalent comparator as array::sort
  stones.forEach((stone) => heap.enq(stone)); // <-- populate heap

  while (heap.size() > 1) {
    const y = heap.deq();
    const x = heap.deq();
    if (x !== y) heap.enq(y - x);
  }
  return heap.size() ? heap.deq() : 0;
};

t1 is regular algorithm
t2 is version using heap/priority queue data structure
10 iterations x 10000 runs
    # Elements  t0 avg      t1 avg
1   8           0.00363     0.00106
2   16          0.01036     0.00157
3   32          0.01781     0.00224
4   64          0.09148     0.00432
5   128         0.22560     0.00944
6   256         0.56833     0.01618
7   512         2.37584     0.06091
8   1024        8.78741     0.12614
9   2048        34.29092    0.29697
10  4096        130.50169   0.63872

Notes:

Time is in milliseconds

https://www.npmjs.com/package/priorityqueuejs
Conclusion
Using a heap data structure is orders of magnitude an improvement. With the naive implementation it is clearly at least an \$O(n^2)\$ complexity as each time the dataset size doubles (2x) the runtime roughly quadruples (~4x) whereas the implementation using a heap roughly only doubles (~2x) the runtime with each doubling of the dataset.
Performance Benchmarking
performanceBenchmark.js
const measurePerf = (fn, data, runs = 1e3) =>
  [...Array(runs).keys()]
    .map(() => {
      const start = performance.now();
      fn([...data]);
      const end = performance.now();
      return end - start;
    })
    .reduce((total, current) => total + current) / runs;

const toFixed = (val, fixed) =>
  Number.isFinite(val) ? Number(val).toFixed(fixed) : val;

export const benchmark = async ({
  functions = [],
  createRunData,
  iterations = 5,
  runs = 1e3,
  logIntermediateResults
}) => {
  logIntermediateResults && console.log(`${iterations} x ${runs}`);

  const results = [];

  logIntermediateResults &&
    console.log(
      `\t# Elements\t${functions.map((_, i) => `t${i} avg`).join("\t")}`
    );

  for (let i = 0; i < iterations; i++) {
    const data = createRunData(i);

    const res = await Promise.all(
      functions.map((fn) => measurePerf(fn, data, runs))
    );

    results.push(res);

    logIntermediateResults &&
      console.log(
        `${i + 1}\t${data.length}\t${res
          .map((t) => `${toFixed(t, 5)}`)
          .join("\t")}`
      );
  }

  return results;
};

Setup & benchmark
const ITERATIONS = 10;
const RUNS = 1e4;
const SEED = 8;

const functions = [
  lastStoneWeight,
  lastStoneWeightHeap,
];

const createRunData = (i) => {
  const dataLength = SEED << i;
  const stones = [...Array(dataLength).keys()].map(() =>
    Math.floor(Math.random() * dataLength)
  );
  return stones;
};

benchmark({
  functions,
  createRunData,
  iterations: ITERATIONS,
  runs: RUNS,
  logIntermediateResults: true
});

Extended heap implementation benchmark
15 x 10000
    # Elements  t0 avg
1   8           0.00100
2   16          0.00171
3   32          0.00242
4   64          0.00434
5   128         0.00933
6   256         0.01825
7   512         0.05681
8   1024        0.13715
9   2048        0.27621
10  4096        0.59631
11  8192        1.24577
12  16384       4.75092
13  32768       6.09799
14  65536       13.07677
15  131072      28.88058


Raspberry PI switching from Soft Access Point to WIFI client I am relatively new to C language and Linux systems I would like some feedback/review. The main function of the code is to read the dhcpcd file looking for the 3 lines and then either adding/removing to the new file. It works and the Pi can switch from AP to client without a reboot, but I feel like it is more of a hack than a proper solution.

#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct word {
    int linenumber;
    char *value;
};

struct words {
    int wordcount;
    struct word wordlist[3];
};

int main(void) {
    FILE *fp;
    FILE *temp;
    char filename[] = "/etc/dhcpcd.conf";
    char tempname[] = "/etc/temp.conf";
    char *line = NULL;
    size_t len = 0;
    ssize_t read;
    int linecount = 0;
    int index = 0;
    int shouldSkip = 0;
    struct words searchlist;
    struct word wordA;
    struct word wordB;
    struct word wordC;

    wordA.value = "interface wlan0";
    wordA.linenumber = -1;
    wordB.value = "static ip_address=192.168.4.1/24";
    wordA.linenumber = -1;
    wordC.value = "nohook wpa_supplicant";
    wordC.linenumber = -1;

    searchlist.wordcount = 3;
    searchlist.wordlist[0] = wordA;
    searchlist.wordlist[1] = wordB;
    searchlist.wordlist[2] = wordC;

    fp = fopen(filename, "r");
    temp = fopen(tempname, "w");

    if (fp == NULL || temp == NULL) {
        printf("Unable to open file!\n");
        exit(-1);
    }

    while ((read = getline(&line, &len, fp)) != -1) {
        struct word *xword = &searchlist.wordlist[index];
        if(strstr(line, xword->value) != NULL) {
            xword->linenumber = linecount;
            if(index > 0 && index != searchlist.wordcount - 1) {
                if ((linecount - searchlist.wordlist[index - 1].linenumber) != 1) {
                    for (int i = 0; i < searchlist.wordcount; i++) {
                        struct word *wd = &searchlist.wordlist[i];
                        wd->linenumber = -1;
                        index = 0;
                    }
                }
            }
            index = (index == (searchlist.wordcount - 1)) ? index : index + 1;
        }
        linecount++;
    }
    linecount = 0;
    //AP is on so toggle off
    if (fseek(fp, 0L, SEEK_SET) == 0) {
      while ((read = getline(&line, &len, fp)) != -1) {
        for(int i = 0; i < searchlist.wordcount; i++) {
          if(linecount == searchlist.wordlist[i].linenumber && strstr(line, searchlist.wordlist[i].value) != NULL) {
            shouldSkip = 1;
          }
        }

            if(!shouldSkip)
          fputs(line, temp);
        shouldSkip = 0;
            linecount++;
      }
    }

    if (index != searchlist.wordcount - 1) {
       for(int i = 0; i < searchlist.wordcount; i++) {
         fputs(searchlist.wordlist[i].value, temp);
         fputs("\n", temp);
       }
    }

    fclose(fp);
    fclose(temp);
    remove(filename);
    rename(tempname, filename);
    if(line)
     free(line);

    if (index == searchlist.wordcount - 1) {
      printf("Turning WIFI on\n");
    } else {
      printf("Turning Access Point on\n");
    }
    system("sudo service hostapd stop");
    system("sudo service dhcpcd restart");
    system("sudo service hostapd start");

    exit(0);
}

There are a few compiler errors that may not have show up for you, the first major compiler problem is that read is a basic function in C input and output so the variable declaration ssize_t read; should be changed to something like size_t readsize. It might be good if you compiled with the -w flags to add warnings to the compiler error messages.

The second problem I see is that you might have compiled this with a C++ compiler rather than a C compiler, a strict C compiler will not recognize the function readline() in a strict C compiler fgets() should be used instead. You will have to make the variable line an array of characters.

char line[BUFSIZ];

The symbolic constant BUFSIZ is defined in the include file stdio.h.

    fgets(line, BUFSIZ, fp);

Overall it would be better if main() was a short function that called other functions, it is currently too complex (does too much).


Optimizing code that needs to run through a lot of different numbers for Project Euler I have been working on Project Euler problem number 12. I have code that will get me the right answer, however it takes too long. I know the code works because I have tested it multiple times with numbers that are smaller and take much less time to iterate through. I first wrote essentially the same thing in python, but I know C is faster so I gave it a shot in C, but my solution is just not efficient. Here is the code (in C):

#include <stdio.h>

int main(void) {
  int num = 0;
  int i;
  int j;
  int divisors = 0;
  for (i=1; i<10000; i++){
    num += i;

    for (j=1; j<=num; j++) {
      if (num % j == 0) {
        divisors += 1;
      }
    }

    if (divisors > 500) {
      printf("Num is %d\n", num);
      printf("Divs are %d\n", divisors);
    }

    divisors = 0;

  }
}

Project Euler problems are supposed to be solved in under a minute of run-time anyways, so this isn't a great solution even if I let it run until I got an answer.

Any suggestions on ways to do this faster would be much appreciated!

Iterating [1...num] is too slow
Only [1...sqrt(num)] needed

Instead of

for (j=1; j<=num; j++) {
  if (num % j == 0) {
    divisors += 1;
  }
}

Go to the root of num (when j*j <= num) and get 2 or 1 divisors.

for (j=1; j*j <= num; j++) {
  if (num % j == 0) {
    divisors += 1;
    if (j*j < num) divisors += 1;
  }
}

Re-formulate to avoid j*j overflow

for (j=1; j < num/j; j++) {
  if (num % j == 0) {
    divisors += 2;
  }
}
if (j == num/j) {
  divisors += 1;
}

Note: Many compilers with recognize the nearby num/j and num % j and perform the operation for the time cost one of one.

Example

Consider num == 36.  Original code tries 36 numbers with the starred ones having an exact division.

1*,2*,3*,4*,5,6*,7,8,9*,10,11,12*,13,14,15,16,17,18*,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36*.

Notice that when num%j == 0, both j and num/j are divisors.  Except when j == num/j, that discovers two unique divisors at once.  Code only needs to  iterate to 6.

j=1, divisors: 1 & 36
j=2, divisors: 2 & 18
j=3, divisors: 3 & 12
j=4, divisors: 4 & 9
j=5, divisors: none
j=6, divisor: 6

6 iterations faster than 36.

With a num = 1,000,000, new code will iterate to 1000 rather than 1,000,000 - a thousand times faster.

Detail on overflow possibility.

Consider num is at or near INT_MAX:

With for (j=1; j<=INT_MAX; j++) {, j will attempt to increment beyond INT_MAX resulting in undefined behavior.

With for (j=1; j*j < near_INT_MAX; j++) {, Unless near_INT_MAX is a perfect square, j*j will overflow.  Again undefined behavior.

With for (j=1; j < num/j; j++) {, j never overflow for all num.

====

There are many more advance techniques for faster code.


A piece of Python code to compute a frequency table of a list of values I have written a Python 2.5.4 function which accepts as input a list of numbers alongwith the desired number of classes and yields a frequency distribution (i.e. a histogram) of the same data.

While setting class boundaries, I've attempted to increase the accuracy (i.e. number of digits after the decimal point).

Here's my code:

def num_after_point(x):
    s = str(x)
    if '.' not in s:
        return 0
    return len(s) - s.index('.') - 1

def histogram(list, numBuckets):
    count = [0] * len(list)
    for i in range(len(list)):
        count[i] = num_after_point(list[i])
    lowest = min(list) - 10**(-max(count)-1)
    highest = max(list) + 10**(-max(count) -1)
    print "Lowest data value:", lowest
    print "Highest data value:", highest
    dataRange = highest - lowest
    bucketSize = dataRange / numBuckets
    print "Bucket Size:", bucketSize
    bucketCountList = [0] * numBuckets
    low = lowest
    high = lowest + bucketSize
    index = 0
    for i in range(len(list)):
        if low < list[i] < high:
            bucketCountList[index] += 1
    print "From", low, "to", high, ":", bucketCountList[index]
    while high < highest:
        low = high
        high = low + bucketSize
        index = index + 1
        for i in range(len(list)):
            if low < list[i] < high:
                bucketCountList[index] += 1
        print "From", low, "to", high, ":", bucketCountList[index]

histogram( [ 0.1, 0.22, 0.333, 0.4444, 0.55555, 0.666666,
             0.7777777, 0.88888888, 0.999999999, 1.0], 3)

Is this code error-free enough?

If not, where and how does it break down?

How can I improve its functionality and efficiency?

You may want to try 2.5.6, a security fix to 2.5.4.
Alternately either 2.7 or, preferably, 3 would be a better upgrade.
They may be unfeasible, but you should upgrade, you're 7 years behind!

First things first, don't do hacky stuff that give incorrect bounds.

  lowest = min(list) - 10**(-max(count)-1)

The correct bounds are not the ones you are using!
They are:

[0.1, 0.4)
[0.4, 0.7)
[0.7, 1.0]

All bar the last 1 inclusive is easy, just remove that hack you are using.
If we put the bound creator in a function then we can create the bounds and re-use them in a future function.
To handle the 1 inclusive you can change it to a 2, (1 + 1).

I would also use comprehensions, they are quite simple, but extremely useful.
For example creating count would be:

count = [num_after_point(list[i]) for i in range(len(list))]

This can be improved by not indexing list and not using len and range.

count = [num_after_point(i) for i in list]

As most of your function is creating the bounds I'll go through it first.
You want to keep most of your code the same. But:

Remove num_after_point, count and all that mess.
Remove all the bucketCountList stuff.
Change the while loop to a for loop for _ in range(numBuckets - 1):.
Yes you want the -1.
After changing low and high, yield them. (yield low, high).
After the for loop yield high, highest + 1, this is the only edge case we will be using.

This should get you:

def bounds(list_, numBuckets):
    lowest = min(list_)
    highest = max(list_)
    bucket_size = (highest - lowest) / numBuckets
    upper_bound = lowest
    for _ in range(numBuckets - 1):
        lower_bound, upper_bound = upper_bound, upper_bound + bucket_size
        yield lower_bound, upper_bound
    yield upper_bound, highest + 1

Now to use it is very easy:

You want to go through all the lower and upper bounds of the bounds function.
simply for l, u in bounds(...):.
Generate the sum in the bounds.
Display everything.

To get:

def histogram(list_, num_buckets):
    for lower_bound, upper_bound in bounds(list_, num_buckets):
        data = sum(lower_bound <= i < upper_bound for i in list_)
        print "From", lower_bound, "to", upper_bound, ":", data

The only down-side is the one hack we used to include the upper bound.
This has a small impact on the display, but ultimately it's a much nicer solution.

The bounds are easier to read.
You can use every number in the range. You couldn't use 0.0999999999 for example.

Finally, not knowing the numbers in the range is a bit... odd.
You should probably allow that, which is using a comprehensions if.

def histogram(list_, num_buckets):
    for lower_bound, upper_bound in bounds(list_, num_buckets):
        data = [i for i in list_ if lower_bound <= i < upper_bound]
        print "From", lower_bound, "to", upper_bound, ":", len(data)
        print data


Avoiding sub-queries in named scopes I use named scopes all over the place.

When I have a named scope that deals with two different tables, I tend to query the second table in a sub-query. That way, if I mix and match with other tables I don't need to worry about reusing a table alias or having ambiguous column names that only appear in bizarre scenarios.

For example, suppose I have a posts table that is one-to-many related to a tags table. I might add a with_tags scope on my Post model like this:

named_scope :with_tags, lambda { |tags|
  tags = [tags] if tags.kind_of? String
  {:conditions => ["id IN (SELECT post_id FROM tags WHERE name IN (?))", tags]}
}

However, that doesn't seem ideal. Many databases can use a join more efficiently than they can use a sub-query. A query that looks like this might perform better:

SELECT DISTINCT posts.* FROM posts JOIN tags ON posts.id = tags.post_id WHERE tags.name IN (?)

How do other people do this? Do you use the :include/:join parameters cleverly to know what the aliases ActiveRecord will use?

You want to use what Rails calls eager loading. This is done with the :include parameter in your AR call. Add it to your lambda block.


Is it a bad programming practice to declare variable in a loop? Today, I came across a code example like the following:

Option Explicit

Sub MySub()
    Dim iCount As Integer

    For iCount = 0 To 100
        Debug.Print "Arbitrary Code Here"
        If iCount Mod 2 = 0 Then

            Dim myString As String 'This is the line I'm curious about

            myString = iCount
            Debug.Print myString
        End If
    Next iCount
End Sub

Once a variable goes in scope in VB6 and/or VBA, it stays in scope throughout the procedure.

So, is it a bad programming practice to declare a variable in a loop?  Is it better to declare myString either at the top of the procedure or, at the very least, above the FOR LOOP?

There are a few things to consider here. First we have to consider if the position will matter to the compiler, which depends on the language. VB6 will scope that variable to the function (or sub in this case), so it doesn't matter too much there. In VB.NET, the variable will be scoped to the containing block (in this case the loop), so declaring inside of the loop would let us reuse that same variable name in some other block.

In terms of maintainability, there are two good reasons to put the declaration as close as possible to the first use of the variable. One, it improves readability. Two, it simplifies future refactoring -- imagine that you wanted to move that loop to its own function, that's easier if the variable is right there with the loop.

short answer: In VB6 I think it's best to declare just before the loop. This is mostly so that nobody thinks that the variable will be re-initialized every time the loop runs.


Configuring a date picker with support for disabled dates I'd like to see if I can somehow make this loop more effective (smaller?). I'm using MultiDatesPicker to make a reservation plugin. And I've set everything right, and everything works, but I'd like to see if my jquery code can be optimized somehow.

HTML looks like this (minus the actual calendar):

    Reservation Date:
    <input type="hidden" id="disabled_dates" value=""27.11.2015, 28.11.2015"">
    <input type="text" name="reservation_date" id="reservation_dates_input" value="14.11.2015, 15.11.2015, 25.11.2015">

I have inputs where I have dates that have been reserved #disabled_dates, and in my #reservation_date is the date of the current reservation. Now this input, if you're creating a new reservation, is empty, which means if I don't make the check if it's empty, I'll get an error in my date picker plugin (if the option for it is included). There are double quotes in the #disabled_dates because that's how I get my output from php but the code below deals with that, so that's not an issue at all.

I'm just wondering if this big if - else if - else chunk can be reduced somehow.

jQuery(document).ready(function($) {
    "use strict";

    var disabled_dates = $('#disabled_dates').val(),
        reservation_dates_input = $('#reservation_dates_input').val(),
        disabled_dates_array,
        reserved_dates_array,
        options;

    if (disabled_dates.length) {
        var clean_disabled_dates = disabled_dates.replace(/\"/g, '');
            disabled_dates_array = clean_disabled_dates.split(", ");
    }

    if (reservation_dates_input.length) {
        var clean_reserved_dates = reservation_dates_input.replace(/\"/g, '');
            reserved_dates_array = clean_reserved_dates.split(", ");
    }

    if (disabled_dates.length>2 && reservation_dates_input.length>2) {
        options = {
            firstDay: 1,
            dateFormat: "dd.mm.yy",
            altField: '#reservation_dates_input',
            addDisabledDates: disabled_dates_array,
            addDates: reserved_dates_array
        };
    } else if(disabled_dates.length>2 && reservation_dates_input.length<=2) {
        options = {
            firstDay: 1,
            dateFormat: "dd.mm.yy",
            altField: '#reservation_dates_input',
            addDisabledDates: disabled_dates_array,
        };
    } else if(reservation_dates_input.length>2 && disabled_dates.length<=2) {
        options = {
            firstDay: 1,
            dateFormat: "dd.mm.yy",
            altField: '#reservation_dates_input',
            addDates: reserved_dates_array
        };
    } else {
        options = {
            firstDay: 1,
            dateFormat: "dd.mm.yy",
            altField: '#reservation_dates_input',
        };
    }

    $('#reservation').multiDatesPicker(options);

});

I tried putting empty arrays in the disabled_dates_array and reserved_dates_array, but MultiDatesPicker doesn't allow empty arrays, or arrays with no date in it.

You don't need to build the options object in one go instead create what all cases need and then optionally add:

options = {
    firstDay: 1,
    dateFormat: "dd.mm.yy",
    altField: '#reservation_dates_input',
};

if (disabled_dates.length>2) {
    options.addDisabledDates = disabled_dates_array;
}
if (reservation_dates_input.length>2) {
    options.addDates = reserved_dates_array;
}

you also have some code duplication when creating the arrays, you can put that in a function and call it twice:

function createArrayFromInput(dates_input){
    if (dates_input.length) {
        var clean_dates = dates_input.replace(/\"/g, '');
        return clean_dates.split(", ");
    }
    return [];
}


BadCafe as a git object short hash as I was reading the Git Internals in the Pro Git book, I decided to see what objects I need to create to have the short hash say something funny. To do this, I have written the following code:

messages = ['badcafe','deaddad','badbeef','feedbac']
possible_letters = string.ascii_lowercase + string.ascii_uppercase + ' '

def getHash(data):
    return hashlib.sha1(str("blob " + str(len(data)) + "\0" + data).encode('utf-8')).hexdigest()

def populate(n):
    for data in product(possible_letters, repeat=n):
        s = getHash(''.join(data))[0:7]
        if s in messages:
            print(s,data)

Unfortunately, this is too slow for n > 4. For smaller n, it only finds badcafe. Is there any way to speed this up?

That's a brute force search of hashes for a particular prefix, meaning it's complexity is \$O(2^n)\$, where \$n\$ is the number of bits in the prefix. In your case that's \$2^{28}\$.

You can't make it faster in terms of complexity or the hash would be broken, but you can increase the constant factor.

Using cProfile I see that out of 30 seconds of runtime the worst offenders are:

8042221   10.763    0.000   24.243    0.000 tmp.py:10(getHash)
      5    5.138    1.028   30.701    6.140 tmp.py:13(populate)
8042222    3.805    0.000    3.805    0.000 {_hashlib.openssl_sha1}
8042221    5.143    0.000    5.143    0.000 {method 'encode' of 'str' objects}
8042221    3.876    0.000    3.876    0.000 {method 'hexdigest' of '_hashlib.HASH' objects}
8042221    1.320    0.000    1.320    0.000 {method 'join' of 'str' objects}

The actual hashing is done in hexdigest and OpenSSL. Those pretty much cannot be reduced, so at best it could be made ~3 times as fast.

In python 2 you can remove the encode invocation. That alone saves a few seconds. A set is faster for in testing, so that's also an easy fix. Take advantage of hashlib.sha1's copy and update by noticing the length stays constant and you are at 2x the speed:

def getHash(data, h):
    h = h.copy() # "fork" the hash
    h.update(data)
    return h.hexdigest()

def populate(n):
    h = hashlib.sha1("blob " + str(n) + "\0") # shared part of the hash
    for data in product(possible_letters, repeat=n):
        s = getHash(''.join(data), h)[0:7]
        if s in messages:
            print(s,data)

On Python 3 you can encode the letters into bytes and avoid encode inside the loop, for similar performance.

You might still make it about 1.5x as fast if you replaced the product -> join algorithm with something faster (bytearray(n) and depth first search?), but that's about it.