__constructor mysqli variable options I like the fact that I can create a class and have the connection details in one file if I need to update them and then pass them to the method I create in the class that defines my specific use of mysqli. I was thinking also that I may give an option to pass the connection details if I need to use a different db in one page. Is this a good idea. Any adverse problems?

Basic Example (Note the values defined in __constructor)

public function __construct($host = 'localhost', $user = 'myuser', $pass = 'mypass', $db = 'mydb') {
    // Connect to Database
    $this->mydb = new mysqli($host, $user, $pass, $db);
}

Note: this post gets extremely side tracked, but the two ideas are pretty closely related, so hopefully my rambling ends up being helpful.

new in a constructor is an anti-pattern. It immediately creates very high coupling, and it murders any form of coding to interface.

Rather than passing what you need to construct the object, you should pass the actual object. This idea (or at least, an idea very closely related to it) is called dependency injection.

This is all a bit abstract, so I'll give an example. Imagine that you have a simple database-backed logger. I'm more familiar with PDO than MySQLi, so I'll use it.

class DbLogger
{
    private $_db;

    const WARNING = 1;
    const ERROR = 2;

    public function __construct()
    {
        $this->_db = new PDO("sqlite:log.sqlite", null, null, array(
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
        ));
    }

    public function log($message, $severity)
    {
        $stmt = $this->_db->prepare("INSERT INTO log (message, serverity) VALUES (?, ?)");
        $stmt->execute(array($message, $severity));
    }

}

Seems simple enough, right? And quite easy to use. No matter where you are in code, you can just do $log = new DbLogger(); $log->log('...'); and you're done.

There's a major problem though. What if you want to use a different database? What if you want a users_log.sqlite and a financials_log.sqlite? Well, simple... You could just pass the name of the log to the constructor.

And yes, that would work, but how manageable is it? Do you really want to find all the places that the name is used every time you want to change it?

Consider if instead of creating the PDO instance, the constructor accepted it. This allows an important difference to happen: DbLogger no longer has to care about how to create a PDO object; it only has to care about using it. Now if you want to use two different databases, you just construct two different loggers based on two different PDO objects. "But," you might say, "don't I then have to pass around the details of my log database?"

Nope. Instead of passing around the details, you just pass around the object.

Any object that needs a logger, you just construct it with a logger. Or, if only a certain method needs logging, you only have to pass that method the logger. It's a bit hard to get used to this pattern, but once you've done it a bit, the way the object graph fits together becomes a lot more clear.

Basically, the idea is to never create your own dependencies. That creates extreme coupling and makes providing alternate implementations impossible. To steal an example from a Google code talk, imagine that you have a bit of code that's responsible for charging a credit card. Now, imagine that you want to test that code. So, you go, you add three items, and you go to run the transaction. Oops, you just actually charged yourself. So... how do you fake it? Do you comment out the actual request and just echo out the variables? Sure, that works once, but how do you make sure you don't accidentally break the code later? You need the test to be rerunnable. You need it to be able to go through the normal process, but instead of actually running the credit card, you need it to just pretend. This is a perfect example of why dependency injection is necessary. If you instead pass in a credit card charging object, a real implementation can be used during production, but for tests, you can just use a mock that just verifies what's passed to it.

Anyway, I'm rambling a bit, so here's a real example:

interface Logger
{
    const INFO = 1;
    const WARNING = 2;
    const ERROR = 3;
    public function log($message, $severity);
}

class NullLogger implements Logger
{
    public function log($message, $severity)
    { }
}

class DbLogger implements Logger
{
    private $_db;
    public function __construct(PDO $db)
    {
        $this->_db = $db;
    }

    public function log($message, $severity)
    {
        $this->_db->prepare("...");
        $this->_db->execute(...);
    }

}

class FileLogger implements Logger
{
    private $_fh;

    public function __construct($file)
    {
        if (!is_resource($file) && is_string($file)) {
            $file = fopen($file, 'a');
        }
        if (!is_resource($file)) {
            throw new InvalidArgumentException("A valid file handle or path must be provided: " . $file);
        }
        $this->_fh = $file;
    }

    public function log($message, $severity = self::WARNING)
    {
        fputs($this->_fh, "...");
    }

}

Now, imagine you have some class that needs a logger. It no longer is tied specifically to a specified DbLogger. In fact, it's no longer tied to a DbLogger at all. All the object has to care about is that it has an object on which it can call log. It's not tied to a specific logger. If we need to change the logger, we just pass it a different logger on construction. No magic to it. No need to edit a bunch of files. Just one config.php (or config.ini/config.json/etc) has to be changed. All we need is a very top layer that knows how to kick off the chain of object creation ("a User needs a DBAL which needs a DatabaseConnection which needs a PDO" for example) and this can all be done with fairly little effort (and lots of different approaches/tools exist to do that).

Hopefully this answer has been at least semi-useful. It's super rambling, and it's not exactly what you were asking about, but I'm guessing that dependency injection and coding to interface are not two things that you're familiar with. It's a bit odd in a situation with database connections since they're so low level (relatively). There's not exactly a RDBMS interface that MySQLi and other connection classes extend. This makes the interface thing a pretty unrelated topic. When the question is extended to a higher level concept though, hopefully it makes sense.

Anyway, please let me know if you have any questions as I fear that this explanation is pretty bad.


Date only limited function class Per the title date only class (or struct?).

.NET does not offer a date only data type.

I get there is time zone dynamic but then you have daylight-savings that can still kill you. I do document management and we just have a document with a date.

// minimal limited test
DateOnly date1 = new DateOnly(1901, 2, 1);
DateOnly date2 = new DateOnly(1902, 3, 2);
DateOnly date3 = new DateOnly(1902, 3, 2);
Debug.WriteLine(date1.GetDayDiff(date2));  // 365 + 29 = 394
Debug.WriteLine(date2.GetDayDiff(date1));
Debug.WriteLine(date2.CompareTo(date1));
Debug.WriteLine(date1.CompareTo(date2));
Debug.WriteLine(date2.CompareTo(date3));
// end test

public class DateOnly: Object, IComparable
{
    private Dictionary<int, int> MonthDay = new Dictionary<int, int>() { { 1, 31 }, { 2, 29 }, { 3, 31 }, { 4, 30 },  { 5, 31 },  { 6, 30 }
                                                                       , { 7, 31 }, { 8, 31 }, { 9, 30 }, { 10, 31 }, { 11, 30 }, { 12, 31 } };
    public override int GetHashCode() { return (Int32)Days; }
    public override bool Equals(Object obj)
    {
        if (obj == null || GetType() != obj.GetType())
            return false;

        DateOnly other = (DateOnly)obj;
        return (this.Days == other.Days);
    }
    public int CompareTo(object obj)
    {
        if (obj == null)
        {
            throw new ArgumentNullException();
        }
        if (obj is DateOnly)
        {
            DateOnly other = (DateOnly)obj;
            if (other == null)
            {
                throw new ArgumentNullException();
            }
            return (this.Days).CompareTo(other.Days);
        }
        else
        {
            throw new ArgumentNullException();
        }
    }
    public override string ToString()
    {
        return string.Format("{0}/{1}/{2}", Year, Month, Day);
    }
    public Int64 GetDayDiff(DateOnly Date)
    {
        Int64 getDayDiff = (Int64)this.Days - (Int64)Date.Days;
        return (getDayDiff);
    }
    public UInt32 Days    { get; private set; }
    public int Year       { get; private set; }
    public int Month      { get; private set; }
    public int Day        { get; private set; }
    public UInt32 MaxDays { get; private set; }
    public DateOnly(int year, int month, int day)
    {
        if (!ValidateDate(year, month, day))
            throw new ArgumentOutOfRangeException("invalid date");
        MaxDays = GetDays(2078, 12, 31) - 1;
        Year = year;
        Month = month;
        Day = day;
        Days = GetDays(year, month, day);
    }
    private bool ValidateDate(int year, int month, int day)
    {
        bool valid = true;
        if (year < 1900 || year >= 2079)
            valid = false;
        else if (month < 1 || month > 12)
            valid = false;
        else if (day < 1 || day > 31)
            valid = false;
        else if (day > DaysPerMonth(year, month))
            valid = false;
        return valid;
    }
    private UInt32 GetDays(int year, int month, int day)
    {
        UInt32 days = 0;
        int yearSince1900 = year - 1900;
        UInt32 temp;
        for (int i = 1; i <= yearSince1900; i++)
        {
            temp = DaysPerYear(i + 1900 - 1);
            days += temp;
        }
        for (int i = 2; i <= month; i++)
        {
            temp = DaysPerMonth(year, i - 1);
            days += temp;
        }
        days += (UInt32)day;
        return days;
    }
    private bool IsLeap(int year)
    {
        return ((year % 4 == 0 && year % 100 != 0) || year % 400 == 0);
    }
    private UInt32 DaysPerYear(int year)
    {
        if (IsLeap(year))
            return 366;
        else
            return 365;
    }
    private UInt32 DaysPerMonth(int year, int month)
    {
        UInt32 days = (UInt32)MonthDay[month];
        if (month == 2 && !IsLeap(year))
            days = 28;
        return days;
    }
}

Properly overriding GetHashCode()

I remember when first learning how to properly override and use GetHashCode(), that there was a rule that stated The integer returned by GetHashCode() should never change. You're violating this rule because Days is not a readonly property.

Bad practices and unnecessary code

Take a look at this snippet:

public int CompareTo(object obj)
{
    if (obj == null)
    {
        throw new ArgumentNullException();
    }
    if (obj is DateOnly)
    {
        DateOnly other = (DateOnly) obj;
        if (other == null)
        {
            throw new ArgumentNullException();
        }
        return (this.Days).CompareTo(other.Days);
    }
    else
    {
        throw new ArgumentNullException();
    }
}

There are 2 problems in this method.

First - you're using the is operator combined with a direct cast, instead of using the as operator.

Second - you have a redundant if statement that will never be triggered - if (other == null), if the cast (DateOnly other = (DateOnly) obj;) fails it will crash your program, it wont return null.

And lastly you don't really need an else statement there as there is flow branching involved if you trigger the if statement you will eventually reach the return statement which will break out of the method, thus not even continuing down your code, making the else redundant.

This is how I would write this method:

public int CompareTo(object obj)
{
    if (obj == null)
    {
        throw new ArgumentNullException();
    }
    var other = obj as DateOnly;
    if (other != null)
    {
        return (this.Days).CompareTo(other.Days);
    }
    throw new ArgumentNullException();
}

In GetDayDiff(DateOnly Date) you perform 2 casts when only 1 is necessary to operate with the proper type:

long getDayDiff = (long) this.Days - (long) Date.Days;

You can just do:

long getDayDiff = this.Days - (long) Date.Days;

I'm not sure why you inherit from object, this is unnecessary, as every type in C# inherits from that class whether it will be explicitly stated changes nothing, it's just few extra letters.

Shortening the code

You can use interpolated strings instead of String.Format():

  return string.Format("{0}/{1}/{2}", Year, Month, Day);

As interpolated string:

return $"{Year}/{Month}/{Day}";

Ternary operator:

You can make use of the ternary operator here:

if (IsLeap(year))
    return 366;
else
    return 365;

Like this:

return (uint) (IsLeap(year) ? 366 : 365);

Overall design

Few concerns here:

I would just use DateTime as a backing field and utilise some of the already written methods there (I'm sure this was a design pattern but I cant think of the name right now, maybe Decorator?).
You lack range checks and overall validation of your public methods.
You're inconsistent with where you use uint and int, if you've decided that it would be good to use uint because x, y, z, why are you not using that everywhere? This will also partially solve point 2.


Re-use of read/write object , for multiple objects I have read a lot about serialization and I cannot figure out how I am able to serialize then deserialize multiple objects. I am very much a newb to Java, so please bear with me.

My code currently works, but my logic is flawed. I have an object called "club", which contains several fields to be serialized e.g teamname. I have used case statements with a switch, so that I can serialize more than one object, but I ended up re-trying my try/catch block and streams to get it to work.

Both objects can be independently written to a file, but I am unclear of the syntax to use, to ensure I can implement the following pseudocode.

if teamName = club1
and player would like to save the game
serialize club1
then de-serialize club1, if player selected load game.

Finally, could someone point me in the right direction of writing a game "state" to a file, rather than just pre-defined fields that I have declare as constants.

Snippets of my code are as follows :

switch (option) {

    case 1:
    chooseTeam(clubName);
    exit = true;
    break;

    case 2:
    Serialize saveclub1 = new Serialize();
    saveclub1.Serialize();
    exit = true;
    break;

    case 3:
        DeSerialize load = new DeSerialize();
        load.DeSerialize(clubName);
        exit = true;
    break;

    case 4:
    credits();
    exit = true;
    break;

    case 5:
    exit = true;
    System.out.println("Goodbye!");
    break;

    case 6:

    Serialize saveclub2 = new Serialize();
    saveclub2.Serialize();
    ClubInfo club2 = new ClubInfo();
      club2.teamName = "Aston Villa";
      club2.stadium = "Villa Park";
      club2.division = "Premier League";
      club2.SSN = 11122333;
      club2.stadiumCapacity = 40000;
    try
        {
         FileOutputStream fileOut =
         new FileOutputStream("/home/cg/root/club.ser");
         ObjectOutputStream out = new ObjectOutputStream(fileOut);
     out.writeObject(club2);
     out.close();
         fileOut.close();

      //System.out.printf("Serialized data is saved in C:/tmp/club.ser");
      }catch(IOException i)
      {
          i.printStackTrace();
      }
     exit = true;
        break;

public void DeSerialize(String[] clubName)
   {
      ClubInfo club = null;

      try
      {
         FileInputStream fileIn = new FileInputStream("/home/cg/root/club.ser");
         ObjectInputStream in = new ObjectInputStream(fileIn);
         club = (ClubInfo) in.readObject();

         in.close();
         fileIn.close();

      }catch(IOException i)
      {
         i.printStackTrace();
         return;
      }catch(ClassNotFoundException c)
      {
         System.out.println("Club class not found");
         c.printStackTrace();
         return;
      }

public void Serialize()
   {

      ClubInfo club = new ClubInfo();
      club.teamName = "Arsenal";
      club.stadium = "Emirates";
      club.division = "Premier League";
      club.SSN = 11122333;
      club.stadiumCapacity = 60000;

      try
      {
         FileOutputStream fileOut =
         new FileOutputStream("/home/cg/root/club.ser");
         ObjectOutputStream out = new ObjectOutputStream(fileOut);
     out.writeObject(club);

         out.close();
         fileOut.close();

      System.out.printf("Serialized data is saved in C:/tmp/club.ser");
      }catch(IOException i)
      {
          i.printStackTrace();
      }

   }
}

public class ClubInfo implements java.io.Serializable{

   public String teamName;
   public String stadium;
   public String division;
   public transient int SSN;
   public int stadiumCapacity;

} // end ClubInfo class

Based on the comments, I think you're confused about actually saving/loading distinct ClubInfo objects. You can call writeObject and readObject multiple times, but you need to store those ClubInfo objects in separate files if you want to persist them.

What you could do is add a ClubInfo parameter to your Serialize method, then name the file you save based on the clubName. By doing this, you don't tie down your serialize method to one instance of ClubInfo...you can pass in any ClubInfo object and it will save it to a different output file:

public void Serialize(ClubInfo club) {

  String fileName = club.teamName + ".ser";

  try {
    FileOutputStream fileOut = new FileOutputStream("/home/cg/root/" + fileName);
    ObjectOutputStream out = new ObjectOutputStream(fileOut);

    out.writeObject(club);
    out.close();
    fileOut.close();

    System.out.println("Serialized data is saved in C:/tmp/club.ser");

    } catch(IOException i) {
        i.printStackTrace();
    }
  }
}

This is close to your original code. A few things I would change:

Assuming you're using Java 7 or newer, use the try-with-resources feature so you don't explicitly have to call close() on your streams.
I would have the thread throw the IOException that's required instead of handling it inside the method itself. It makes the method cleaner and you can handle the exception in different ways, depending on where and how you call it.

Here's how it looks afterwards:

public void Serialize(ClubInfo club) throws IOException {

  String fileName = "/home/cg/root/" + club.teamName + ".ser";

  try (FileOutputStream fileOut = new FileOutputStream(fileName);
       ObjectOutputStream out = ObjectOutputStream out = new ObjectOutputStream(fileOut)) {
    out.writeObject(club);
    System.out.println("Serialized data is saved in C:/tmp/club.ser");
    }
  }
}

And to call it:

case 2:
    Serialize saveclub1 = new Serialize();
    try {
      saveclub1.Serialize(club1);
    } catch(IOException e) {
      System.out.println("Error saving club!");
    }

    exit = true;
    break;

You can do the same-ish thing with Deserialize. I would pass in the teamName I want to search for, get a list of files from the directory I am persisting the objects to and look for the file name that contains the team name. Then call readObject and return it from the Deserialize method. The method signature would look like this:

public ClubInfo Deserialize(String teamName)

Then your method isn't tied to the single club file.

This should also lead you in the direction you should take in saving the game state. I'll leave that to you to figure the guts out, but that should get you in the right direction!


Recursive higher-order function I have implemented a through function for myself in a project.
Since I am still quite new in python and want to learn it correctly, I would be happy about any feedback including style, variable naming...

A first question is how to name the function. Because of Mathematica's documentation I called it through. Is there a more intuitive name?

def through(list_of_functions, value, recurse_level=np.infty):
    """Calls each function in a list

    This function is passing the value as argument to each function
    in a list.
    For a one dimensional list consisting of
    only functions it is equivalent to::

    [f(value) for f in list]

    If an element is not callable and not iterable, it is not called.
    An iterable is either directly appended or will be recursed through
    depending on recurse_level.

    Args:
        list_of_functions (list):
        value (any type):
        recurse_level (int):

    Returns:
        list:
    """
    new_list = []
    for possible_function in list_of_functions:
        try:
            new_list.append(possible_function(value))
        except TypeError: # item not callable; could be iterable
            # If it is iterable and recurse_level is not negative
            # recurse through elements
            if recurse_level >= 0:
                try:
                    new_list.append(through(possible_function,
                                            value,
                                            recurse_level))
                    recurse_level = recurse_level - 1
                except TypeError: # not an iterable; append without calling
                    new_list.append(possible_function)
            else:
                new_list.append(possible_function)
    return new_list

An example:

In: test = [[1, 2], [3, (lambda x : x ** 2)], (lambda x : x ** 3)]
In: through(test, 4, recurse_level=1)
Out: [[1, 2], [3, 16], 64]
In: through(test, 4, recurse_level=0)
Out: [[1, 2], [3, <function __main__.<lambda>>], 64]

Naming

In Clojure, there's a similar function named juxt.  Not saying that's a better name by any means, just mentioning what it happens to be called in one other place.  Upon further reflection, what you are doing is really just a recursive form of function application.  So a suitable name might reflect that.  Perhaps apply_rec?
Use the shortest name that conveys sufficient meaning.  For example, list_of_functions could be shortened to functions (or even just fs).  possible_function could be shortened to f.  (Single-character names are not good names in general, but for generic entities that have a well-known mathematical notation, they are a good fit.)

Use Better Checks for Callable / Iterable

You check if something's callable by trying to call it and catching a TypeError exception.  However, if you call a function, and that function happens to through a TypeError, then your code will incorrectly assume it's not a function.  You can use the callable function instead, to determine whether or not something is callable.

A similar situation exists in the way you check for iterables.  In this case, you can use the iter function as a fail-fast way to check if something's iterable.

Consider Using a List Comprehension

Reading your code, I thought it was a bit ironic that you used a list comprehension in the docstring to describe the functions behavior, but not in the code itself.  Granted, due to the recursive nature, it's not quite as straight-forward.  But with just a little refactoring, it should be possible.  Here's how I would do it:

def apply_rec(f, x):
  """
  If f is a function, applies f to x and returns the result.
  Otherwise, if f is iterable, applies every element in f to x,
  returning a list of the results.
  If f is neither callable nor iterable, returns f.
  """

  if callable(f):
    return f(x)
  else:
    try:
      fs = iter(f)
    except:
      # Not iterable => return f
      return f

    return [apply_rec(f, x) for f in fs]

I've omitted the recursion limit, but that should be trivial to add.


Algorithm to follow a point This code basically makes the dot follow a certain point on the screen. It consumes a significant amount of computing time in my game, so I'd like it to be best optimized. I wrote this code after quickly reading some pages in a high school mathematics book, so it's unlikely to be a computationally efficient solution.

The mj_ prefix is just what I use instead of a namespace in my C code.

Suggest some good possible optimizations.

struct mj_dot {
    double x;
    double y;
    double th;
};

void mj_dot_rotate(struct mj_dot *, double, double);
void mj_dot_move(struct mj_dot *, double, double, double, double);

static double mj_distance(double x, double y, double x2, double y2) {
    return sqrt(pow(x - x2, 2) + pow(y - y2, 2));
}

void mj_dot_rotate(struct mj_dot *this, double x, double y) {
    double dx = x - this->x;
    double dy = y - this->y;
    this->th = atan(dy / dx);
    if (isnan(this->th)) {
        this->th = 0;
        return;
    }
    if (dx < 0) {
        this->th += MJ_MATH_PI;
    }
}

void mj_dot_move(struct mj_dot *this, double x, double y, double speed, double seconds) {
    double d = speed * seconds;
    if (d >= mj_distance(this->x, this->y, x, y)) {
        this->x = x;
        this->y = y;
    } else {
        mj_dot_rotate(this, x, y);
        this->x += d * cos(this->th);
        this->y += d * sin(this->th);
    }
}

pow(x,2) is less efficient than simply multiplying with itself:

static double mj_distance(double x, double y, double x2, double y2) {
    double dx = x - x2;
    double dy = y - y2;
    return sqrt(dx*dx + dy*dy);
}

However because you just compare it against a distance you can do away with the square root and compare it against the square of the distance:

double d = speed * seconds;
if (d*d >= mj_distance_squared(this->x, this->y, x, y)) {

there is a atan2 function which takes a delta y and delta x to return the correct angle.

void mj_dot_rotate(struct mj_dot *this, double x, double y) {
    double dx = x - this->x;
    double dy = y - this->y;
    this->th = atan2(dy, dx);
}


Implementation of binary min-heap data structure What do you think is wrong with this code, and how can it be improved?  What corner case have I overlooked, if any?

Note: I do not want to use any STL features here, but I'm okay with anything else from C++11.

class min_heap{
    int pvt,P;
    int arr[10000];
public:

    min_heap(){
        P=1;
    }
    void add(int x){
        pvt=P;
        arr[P]=x;

        while(pvt>1){
            if(arr[pvt]<arr[pvt/2]){
                int temp=arr[pvt];
                arr[pvt]=arr[pvt/2];
                arr[pvt/2]=temp;
                pvt=pvt/2;

            }
            else
                break;
        }
        P++;
    }

    int extract(){
        int ans=arr[1];
        arr[1]=arr[P-1];
        int elm=arr[1];
        int x=1;
        while(x*2+1<=P-1){
            int exc=1;
            int temp=arr[x];

            if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
                arr[x]=arr[x*2+1];
                arr[x*2+1]=temp;
                x=x*2+1;
                continue;
            }
            if(arr[x*2]<=arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
                arr[x]=arr[x*2];
                arr[x*2]=temp;
                x=x*2;
                continue;
            }
            break;

        }
        if(P-1>=x*2&&arr[x*2]<arr[x]){
            int temp=arr[x];
            arr[x]=arr[x*2];
            arr[x*2]=temp;
        }
        P--;
        return ans;
    }
};

int main()
{
    min_heap min1;
    for(int i=1;i<=50;i++)
    {
        int x1;
        std::cin>>x1;
        min1.add(x1);
    }
    std::cout<<std::endl;
    for(int i=1;i<=50;i++){
        int x1=min1.extract();
        std::cout<<x1<<std::endl;
    }

}

You've mentioned that you don't want to utilize the STL, so I'll honor that.  Just be aware that neglecting to use it may lessen the quality of your code, especially if you start to experience bugs.

I'll mainly address best-practices as I'm not so familiar with the algorithm myself.

Your lack of whitespace just makes this painful to read.  If a line appears to be too long, it either needs to be shortened (preferred) or wrapped onto separate lines.

Either way, you should especially have some whitespace between operators and operands so that it won't take others unneeded time to read and understand it.  This will also benefit you whenever you're maintaining the code (fixing bugs, adding things, and changing things around).

For instance, this line:

if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){

should look like this:

if (arr[x*2] > arr[x*2+1] && (elm > arr[x*2] || elm > arr[x*2+1])) {

Isn't that easier to read?  There's no need to cram everything together; just let the code breathe.  You could still space out the operators within the [], but it's not quite as common.
Your variable names are very generic.  Many of them are only single letters, and your array data member is just named arr.  Without good naming, you'll just leave others guessing at what each variable does.  Plus, if you look at this code years from now, you may have no idea what it all means, and so you may be tempted to throw it away and start all over.
Although it's entirely optional, you can add the private keyword anyway, even though classes are private by default.  It may add a little more readability, especially if there is a lot of code.
The constructor:

min_heap(){
    P=1;
}

can be an initializer list:

min_heap() : P(1) {}

If it ever gets lengthy, you can list the members on separate lines:

min_heap()
    : P(1)
    , // additional ones
    , // separated by commas
{}

If pvt and arr need to be initialized, then it should be done here as well.  I'm not sure why you just initialize one data member.
You don't need std::endl here.  In most cases, a flush isn't necessary along with a newline, which is what's being done by that.

Instead, just output "\n" for a newline:

std::cout << "\n";

This is also mentioned here.

Overall, I'd highly recommend making this more readable first, so that it's easier to understand and improve for yourself and others.  Implementing all the algorithms yourself will especially require the code to be clean and self-documenting, otherwise you're really just writing C code instead.


Levenshtein Distance of transitively similar words The code's purpose is this: Two words are friends if they have a Levenshtein distance of 1. That is, you can add, remove, or substitute exactly one letter in word X to create word Y. A word’s social network consists of all of its friends, plus all of their friends, and all of their friends’ friends, and so on. Count the friends in the social network of a certain word.

My code is implemented using the Trie written by Steve Hanov. His code is here: http://stevehanov.ca/blog/index.php?id=114.

What I did was this:

social_links = set_up_dictionary_from_text('dictionary.txt')
tree = Trie()
for i in social_links:
    tree.insert(i)
def find(keyword):
    neighbors = [keyword]
    already_in_set = set()
    while len(neighbors) > 0:
        if neighbors[-1] not in already_in_set:
            temp = neighbors[-1]
            already_in_set.add(neighbors.pop())
            current_neighbors = search(tree, temp)
            neighbors.extend(current_neighbors)
        else:
            already_in_set.add(neighbors.pop())
    return(len(already_in_set))

This code works, but runs over 8 minutes for files that are over 100,000 words. Is there something I'm doing wrong? Or should I not use Python for this?

First of all, this is not a Python issue. Rather this is an issue of the implementation itself.

I agree with @Gareth Rees . You should always provide a minimally working example of the code. This is true for StackOverflow and especially true for CodeReview. In that respect all we can review is the little snipped you provide under the assumption that the functions you don't provide do certain things.

The first thing that you can cut is the else: block. It enters iff the last element of neighbors is in already_in_set and what it does is it adds the last element of neighbors to already_in_set; in other words: nothing. As a side effect you do pop the last element and since you do it in both cases it is better allocated above the if.

It looks like search(tree, temp) will return an iterable thing containing all rank 1 neighbors of temp. If you don't do any caching, search is incredibly slow! Loosely speaking that's O(len(dictionary.txt) * max([len(word) for word in dictionary.txt])^2) for the naive implementation and O(max([len(word) for word in dictionary.txt]) * depth(tree)) for the one given in the blog post you mention.

To make matters worse, you do this for every friend of a word exactly one (since you prune duplicates). So what your running is O(len(dictionary.txt)*max([friends(word) for word in dictionary.txt])*O(search)) which in the very crudest worst case can be O(len(dictionary.txt)^4) (!); although this case is only relevant for theoretical considerations.

Here is a list of things you can do:

Cache the Levenshtein distance of two words; also you don't need the actual value, rather the result of the expression distance <= 1 so there is room for more optimization. Also this is symmetric: distance(a,b) = distance(b,a) so you can cache two values for every computation
Cache the result of search(tree, temp). Again this is symmetric: if b in search(tree,a) then a in search(tree,b) so you can cache this result for every element of search(tree,a) without ever computing them [note that this is reflexive too: a in search(tree,a)]
Cache the result of find(keyword). find defines a group relation on dictionary.txt; hence if b in find(a) and c in find(a) then also: a in find(b), a in find(c), c in find(b), b in find(c). You can simply cache this number for every element in the network of a.

Doing all these things will reduce worst case performance to
O(O(find)+O(search)+O(distance)) = O(len(dictionary.txt)^2) and should make it substantially faster. You could think of ways to reduce the number of computations needed for search and distance potentially reducing the overall complexity, but I didn't think into this further.


What's the right granularity for test-driven development? I'm not very experienced in test-driven development (TDD), but I'm trying to get into the groove.  Here I am making a console app that creates records in a database. I bootstrapped by creating a console app and then let MSTest make a unit test for Main(string [] args).

I know I'm doing at least something right, because the tests didn't pass until I did the development -- they "drove" me to "develop".

But I feel like maybe I'm still missing the point, as I didn't really test my code - I tested the database the code acted on.  How would an experienced TDDer approach this?

[TestClass]
public class ProgramTest
{
    private MyEntities _model;

    [TestInitialize()]
    public void MyTestInitialize()
    {
        _model = new MyEntities();
    }

    [TestCleanup()]
    public void MyTestCleanup()
    {
        _model.Dispose();
    }

    [TestMethod()]
    public void MainTest_01_Create_PayTerms()
    {
        string[] args = new[]  { "/Operation", "Insert", "/RecordType", "PayTerms" };
        DirectAgentsService.Program.Main(args);

        _model.PayTerms.SelectValue<string>("it.Name").ShouldContain("Biweekly", "Retainer", "Net 7", "Net 15", "Net 30");
    }
}
static public class Extensions
{
    static public void ShouldContain(this string[] source, params string[] targets)
    {
        foreach (var target in targets)
        {
            Assert.IsTrue(targets.Any(c => c == target), "{0} is missing from collection", target);
        }
    }

    static public void ShouldContain<T>(this T source, params string[] targets) where T : ObjectQuery<string>
    {
        source.ToArray().ShouldContain(targets);
    }
}

Assuming you are speaking of Unit test driven development there are two things you need to get your head around first (or should I say, things I had to get my head around).
Both of them are actually about writing unit testable code (as compared to actually writing test first, etc)

Dependency injection: Avoid constucting objects inside other objects; instead, pass already constructed ones in through the constructor, or properties if appropriate. Look out for the new keyword and avoid it (exceptions apply, such as for classes that are pure data). Of course at some point objects need to be constructed. This should be done at high level classes (say somewhere near your console app's main()) at such a point that is simple enough to need no (unit) testing (since you won't be able to unit test it).
Dependency injection will allow you to inject mock or fake versions of the other classes you are dependent on.
Mocking/Stubbing/Faking/Test Doubles: Your unit tests should test only one thing, not all the things that one this is dependent on (they have their own tests). This allows you to localise (as in know the location of) any errors that show up. You can write your own stubs, but there are great libraries out there, and using them introduces a bit more consistency about how your tests work/look. NSubstitute is a good choice becuase it uses extension methods to be really easy to read.

As to the granularity for Unit Tests, it is as granular as you can go. That way you can localise errors (also the less granular/more integration test like, the harder is it to write tests, they become long). There is some argument about whether private methods should be tested or not. I would say it comes down to whether the private method is just a part of some public method (no) or it has its own well defined purpose (yes).

However, Unit tests driven development isn't the only kind: BDD (behavour driven development) focuses on Functional tests.


Displaying all substrings of some given string I solved exercise 8 from Core Java for Impatient, chapter 1. This exercise is to implement a displayAllCombinations method to display all substrings of the given string.

I would like to prove that following algorithm is valid in Java. I wonder if there is an easier solution?

package pl.hubot.exercises.corejava.r01.ex08;

import java.util.Scanner;

/**
 * This program contains mine solutions of exercises
 * from book Core Java for the Impatient, chapter 1.
 *
 * The content of following exercise:
 * "Write a program that reads a string and displays all contained
 * in it non-empty strings."
 *
 * @version 1.00 23-07-2016
 * @author Hubot
 */
public class Program
{
    public static void main(String[] args)
    {
        System.out.print("Enter any string: ");
        Scanner in = new Scanner(System.in);
        String text = in.nextLine();
        displayAllCombinations(text);
    }

    private static void displayAllCombinations(String text)
    {
        int offset = 0;
        int current = 0;
        int ending = text.length();
        while (offset != ending)
        {
            String result = text.substring(current, ending - offset);
            if (!result.equals(""))
                System.out.println(result);
            if (current != (ending - offset)) current++;
            else
            {
                current = 0;
                offset++;
            }
        }
    }
}

Sample run:

Enter any string: Amy
Amy
my
y
Am
m
A

The variable names offset, current, and ending could be better.  offset seems to be distance from the end, and current acts as a starting index, so the names should reflect that.

The loop logic is cryptic.  There is a current = 0; offset++; that "resets" the loop, so it would be more clearly written with nested for loops instead.

If you do it right, you shouldn't need to check !result.equals("").  If you did need to check, then it would be better written as !result.isEmpty().

private static void displayAllCombinations(String text) {
    for (int end = text.length(); end > 0; end--) {
        for (int start = 0; start < end; start++) {
            System.out.println(text.substring(start, end));
        }
    }
}


Get data from JSON values I have been working on where I want to improve my knowledge to do a safe JSON scraping. By that is that I could pretty much do a ugly workaround and wrap it into a try-except but that is not the case. I would like to know where it goes wrong and how I can improve the code I have been working with:
import json
from json import JSONDecodeError

import requests

payload = {
    "name": None,
    "price": None,
    "sizes": None
}

headers = {'origin': 'https://www.aplace.com', 'Content-Type': 'application/json'}
with requests.get("http://api.aplace.com/sv/dam/aaltoili-iso-mehu-gren-red-sand", headers=headers) as response:
    if not response.ok:
        print(f"Requests NOT ok -> {payload}")

    try:
        # They added extra comma to fail the JSON. Fixed it by removing duplicate commas
        doc = json.loads(response.text.replace(",,", ",")).get("productData", {})
    except (JSONDecodeError, KeyError, ValueError) as err:
        print(f"Not able to parse json -> {err} -> Payload: {payload}")
        raise

if doc:

    product_name = doc.get('name', {})
    product_brand = doc.get('brandName', {})
    product_price = doc.get('markets', {}).get('6', {}).get('pricesByPricelist', {}).get('3', {}).get('price', {})

    # Added exception incase the [0] is not able to get any value from 'media'
    try:
        product_image = doc.get('media', {})[0].get('sources', {}).get('full', {}).get('url', {})
    except KeyError:
        product_image = None

    product_size = doc.get('items', {})

    try:

        if product_name and product_brand:
            payload['name'] = f"{product_brand} {product_name}"

        if product_price:
            payload['price'] = product_price.replace('\xa0', '')

        if product_image:
            payload['image'] = product_image

        if product_size:

            count = 0
            sizes = []
            sizesWithStock = []
            for value in product_size.values():
                if value.get('stockByMarket', {}).get('6', {}):
                    count += value['stockByMarket']['6']
                    sizes.append(value['name'])
                    sizesWithStock.append(f"{value['name']} - ({value['stockByMarket']['6']})")

            payload['sizes'] = sizes
            payload['sizesWithStock'] = sizesWithStock
            payload['stock'] = count

    except Exception as err:
        print("Create notification if we hit here later on")
        raise

print(f"Finished payload -> {payload}")

My goal here is to cover all the values and if we do not find it inside the json then we use the if statements to see if we have caught a value or not.
Things that annoys me:

Long nested dicts but im not sure if there is any other way, is there a better way?
try-except for product_image, because its a list and I just want to grab the first in the list, is it possible to skip the try-except?
If unexpected error happens, print to the exception but am I doing it correctly?

Hopefully I can get new knowledge from here :)

You need functions.
Functions help to split up your code and make understanding how everything fits together easier.
You should have a function for:

Getting the page.

Parsing the JSON with your helpful error message.

Getting the payload.

A main function for calling all the other functions.
We should only call the main function if the script is the entry point.
We can do so with the following code:
if __name__ == "__main__":
    main()

You should add a function to walk paths.

product_price = doc.get('markets', {}).get('6', {}).get('pricesByPricelist', {}).get('3', {}).get('price', {})

# Added exception incase the [0] is not able to get any value from 'media'
try:
    product_image = doc.get('media', {})[0].get('sources', {}).get('full', {}).get('url', {})
except KeyError:
    product_image = None

Things would be so much simpler if we could just do:
price = get(data, "markets", "6", "pricesByPricelist", "3", "price")
image = get(data, "media", 0, "sources", "full", "url")

To build get is simple.

We take an object, a path and a default value.
For each segment of the path we index the object.
If the object doesn't have the value (__getitem__ raises LookupError) we return the default value.
The default value should be to raise an error.
We can ensure anything the user passes to the function is returned.
By making a new instance of an object we can then check if the default is the instance.
For convenience we can build the get function which defaults to None.

_SENTINEL = object()

def walk(obj, path, default=_SENTINEL):
    try:
        for segment in path:
            obj = obj[segment]
        return obj
    except LookupError:
        if default is _SENTINEL:
            raise LookupError(f"couldn't walk path; {path}") from None
        else:
            return default

def get(obj, *path):
    return walk(obj, path, default=None)

I'd highly recommend using exceptions for control flow in Python.
We can subclass BaseException to make an exception that prints to the user and then exits the system.
By wrapping the main() call in a try we can check if our custom error is raised print and then exit with a status of 1.
try:
    main()
except PrettyExitException as e:
    print(e)
    raise SystemExit(1) from None

import json
from json import JSONDecodeError

import requests

_SENTINEL = object()

class PrettyExitException(BaseException):
    pass

def get_page():
    headers = {'origin': 'https://www.aplace.com', 'Content-Type': 'application/json'}
    with requests.get("http://api.aplace.com/sv/dam/aaltoili-iso-mehu-gren-red-sand", headers=headers) as response:
        if not response.ok:
            raise PrettyExitException(f"Requests NOT ok -> {payload}")
        return response.text

def parse_json(raw_json):
    try:
        return json.loads(raw_json)
    except (JSONDecodeError, KeyError, ValueError) as err:
        raise PrettyExitException(f"Not able to parse JSON -> {err} -> Payload: {payload}") from None

def walk(obj, path, default=_SENTINEL):
    try:
        for segment in path:
            obj = obj[segment]
        return obj
    except LookupError:
        if default is _SENTINEL:
            raise LookupError(f"couldn't walk path; {path}") from None
        else:
            return default

def get(obj, *path):
    return walk(obj, path, default=None)

def build_payload(data):
    name = get(data, "name")
    brand = get(data, "brandName")
    price = get(data, "markets", "6", "pricesByPricelist", "3", "price")
    image = get(data, "media", 0, "sources", "full", "url")
    size = get(data, "items")
    count = 0
    sizes = []
    sizes_with_stock = []

    if name is not None and brand is not None:
        name = f"{brand} {name}"
    else:
        name = None

    if price is not None:
        price = price.replace("\xa0", "")

    if size is not None:
        for value in size.values():
            stock = get(value, "stockByMarket", "6")
            if stock is not None:
                _name = get(value, "name")
                count += stock
                sizes.append(_name)
                sizes_with_stock.append(f"{_name} - ({stock})")

    return {
        "name": name,
        "price": price,
        "image": image,
        "sizes": sizes,
        "sizesWithStock": sizes_with_stock,
        "stock": count,
    }

def main():
    raw_data = get_page()
    data = parse_json(raw_data.replace(",,", ","))
    payload = build_payload(data.get("productData", {}))
    print(f"Finished payload -> {payload}")

if __name__ == "__main__":
    try:
        main()
    except PrettyExitException as e:
        print(e)
        raise SystemExit(1) from None


Singly-linked queue in PHP 7 #2 I have improved this post to the code below.

As always, any critique is much appreciated!

<?php

class QueueNode {

    function __construct($item) {
        $this->item = $item;
        $this->next = null;
    }

    function __destruct() {
        $this->item = null;
    }

    function getItem() {
        return $this->item;
    }
}

class QueueIterator implements Iterator {

    function __construct($queue) {
        $this->queue = $queue;
        $this->current_node = $queue->head;
    }

    public function current() {
        return $this->current_node->item;
    }

    public function key(): \scalar {
        return null;
    }

    public function next(): void {
        $this->current_node = $this->current_node->next;
    }

    public function rewind(): void {
        $this->current_node = $this->queue->head;
    }

    public function valid(): bool {
        return isset($this->current_node);
    }
}

class Queue implements IteratorAggregate {

    function __construct() {
        $this->head = null;
        $this->tail = null;
        $this->size = 0;
    }

    function __destruct() {
        $this->head = null;
        $this->tail = null;
    }

    function push($item) {
        if ($this->size === 0) {
            $this->head = $this->tail = new QueueNode($item);
        } else {
            $new_node = new QueueNode($item);
            $this->tail->next = $new_node;
            $this->tail = $new_node;
        }

        $this->size++;
    }

    function pop() {
        if ($this->size === 0) {
            throw new Exception("Popping from an empty queue.");
        }

        $ret = $this->head->getItem();
        $this->head = $this->head->next;

        if (--$this->size == 0) {
            $this->head = $this->tail = null;
        }

        return $ret;
    }

    function getFirst() {
        if  ($this->size() === 0) {
            throw new Exception("getFirst() on empty Queue.");
        }

        return $this->head->getItem();
    }

    function getLast() {
        if ($this->size() === 0) {
            throw new Exception("getLast() on empty Queue.");
        }

        return $this->tail->getItem();
    }

    function size() {
        return $this->size;
    }

    function isEmpty() {
        return size() === 0;
    }

    public function getIterator(): \Traversable {
        return new QueueIterator($this);
    }
}

$queue = new Queue();

for ($i = 1; $i <= 10; $i++) {
    $queue->push($i);
}

echo "Iteration: ";

foreach ($queue as $item) {
    echo $item . " ";
}

echo "
Popping: ";

for ($i = 1; $i <= 10; $i++) {
    echo $queue->getFirst() . ", " . $queue->pop() . " ";
}

echo "
Bye!";
?>

Output

Iteration: 1 2 3 4 5 6 7 8 9 10
Popping: 1, 1 2, 2 3, 3 4, 4 5, 5 6, 6 7, 7 8, 8 9, 9 10, 10
Bye!

I see three major problems with your code:

You dont use PHP 7 type hinting / return types
You dont have any scopes on your methods
If you wont modify your classes in the future and/or you wont extend from these classes you should use final for your class declarations
you should always set declare(strict_types=1); at the beginning of every php script as long as there are no veeeery good reasons to avoid it. It will force you to write your code more typesafe or you will get fatal errors

I will give you an example for each point i mentioned

1. "You dont use PHP 7 type hinting / return types"

Before:

function push($item) {

After:

function push(int $item): void

2. You dont have any scopes on your methods

Before:

function getItem() {

After:

public function getItem(): void

3. class declaration as final

Before:

class QueueNode

After:

final class QueueNode

Sidenote:

Also you should use properties like private $item = null; instead of your constructor setting. Your IDE and also any other maintainer will have trouble finding bugs and/or understanding this code.


Perform loop through process on all sheets at once I have workbook having multiple sheets and having same data on it. Currently I am using macro that .autofilterand copy paste on respective sheets. I am using looping process like:-

   Sub SCRIPT()
    Application.ScreenUpdating = False
    Sheets("SCRIPT").Range("A3:P100").ClearContents
    Dim ws As Worksheet, lrc As Long, LR1 As Long, LR2 As Long
    Dim key1 As String
    key1 = InputBox("Type Script Name", "Title")
    For Each ws In ActiveWorkbook.Worksheets
    If (ws.Name <> "SCRIPT") And (ws.Name <> "MARKET") And (ws.Name <> "CHAIN") Then
    ws.Range("AE1:AJ500").ClearContents
    ws.Range("CB1:CH500").ClearContents
    ws.Range("A1").AutoFilter Field:=1, Criteria1:=key1
    ws.Range("A1").AutoFilter Field:=2, Criteria1:="28-Jan-16"
    lrc = ws.Range("D" & Rows.Count).End(xlUp).Row
    ws.Range("D1:D" & lrc).AutoFilter Field:=4, Criteria1:="CE"
    ws.Range("L2:L" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("AG2")
    ws.Range("C2:C" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("AE2")
    ws.Range("L2:L" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("CD2")
    ws.Range("C2:C" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("CB2")
    ws.Range("C2:C" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("AH2")
    ws.Range("I2:I" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("AJ2")
    ws.Range("I2:I" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("CH2")
    ws.Range("M2:M" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("CF2")
    AutoFilterMode = False
    ws.Range("D1:D" & lrc).AutoFilter Field:=4, Criteria1:="PE"
    ws.Range("L2:L" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("AF2")
    ws.Range("L2:L" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("CC2")
    ws.Range("I2:I" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("AI2")
    ws.Range("I2:I" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("CG2")
    ws.Range("M2:M" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("CE2")
    AutoFilterMode = False
    ws.Range("A1").AutoFilter
    ws.Columns("AE:AJ").Sort key1:=ws.Columns("AE"), Order1:=xlDescending
    ws.Columns("CB:CH").Sort key1:=ws.Columns("CB"), Order1:=xlDescending

    ws.Range("P1").AutoFilter Field:=1, Criteria1:=key1
    ws.Range("P1").AutoFilter Field:=2, Criteria1:="EQ"
    ws.Range("R2:U" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("AM2")
    ws.Range("Z2:Z" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("AL2")
    ws.Range("P2:P" & lrc).SpecialCells(xlCellTypeVisible).Copy Destination:=ws.Range("AK2")
    AutoFilterMode = False
    ws.Range("P1").AutoFilter
    End If

    If (ws.Name <> "SCRIPT") And (ws.Name <> "MARKET") And (ws.Name <> "CHAIN") Then
    LR1 = Sheets("SCRIPT").Range("A" & Rows.Count).End(xlUp).Row + 1
    LR2 = ws.Range("D" & Rows.Count).End(xlUp).Row
    ws.Range("AK2:AZ" & LR2).Copy
    Sheets("SCRIPT").Range("A" & LR1).PasteSpecial xlPasteValues
    ws.Range("AK2:AR" & LR2).Copy
    Sheets("CHAIN").Range("A" & LR1).PasteSpecial xlPasteValues
    End If

    Application.CutCopyMode = False
    Next ws
    Application.Goto (ActiveWorkbook.Sheets("SCRIPT").Range("A1"))
    Application.Goto (ActiveWorkbook.Sheets("CHAIN").Range("A1"))
    Application.ScreenUpdating = True
    End Sub

Filtering huge data on each sheet. Therefore takes lot of time to finished process while looping each sheet. Does there any other way that perform looping task on all sheets at once?

I'll take a crack at this. First things first - always qualify where you're doing something e.g. AutoFilterMode = False should be ws.AutoFilterMode = False.
Your variable names are lacking - lrc, LR1 and LR2 give me no idea as to what they are being used for or if they are different from one another. Same goes for key1 - it's for a "Script Name" according to the inputbox, so why not name is ScriptName or ScriptNameKey? And ws as Worksheet, why not say MyWorksheet or CurrentWorksheet?
You're clearing out Sheets("SCRIPT") but I don't know why, I don't see anything going back to it.
Your indentation is difficult to follow, your for loops and if blocks should be offset like this -
key1 = InputBox("Type Script Name", "Title")

    For Each ws In ActiveWorkbook.Worksheets
         If (ws.Name <> "SCRIPT") And (ws.Name <> "MARKET") And (ws.Name <> "CHAIN") Then
              ws.Range("AE1:AJ500").ClearContents
              ws.Range("CB1:CH500").ClearContents
         End If
    Next

It makes it a lot easier to read. You can actually indent several lines in the VBE by highlighting and hitting Tab.
When you're working with all those .Range elements, you can use a With block -
 key1 = InputBox("Type Script Name", "Title")

        For Each ws In ActiveWorkbook.Worksheets
            If (ws.Name <> "SCRIPT") And (ws.Name <> "MARKET") And (ws.Name <> "CHAIN") Then
                With ws
                    .Range("AE1:AJ500").ClearContents

                    .Range("P1").AutoFilter
                End With
            End If
         Next

This will make it less cluttered. You should also use some comments to explain why things are happening -
 'These ranges need to be cleared to ensure the formatting of xxx doesn't change as it reaches the destination
 .Range("AE1:AJ500").ClearContents
 .Range("CB1:CH500").ClearContents
 'We filter on these two criteria because that allows ....
 .Range("A1").AutoFilter Field:=1, Criteria1:=key1
 .Range("A1").AutoFilter Field:=2, Criteria1:="28-Jan-16"

Speaking of your autofilters why not
Dim FilterDate as Date
FilterDate = Now() 'or whatever the date needs to be

Now you don't have to go into the code to find it.
The same goes for your second criteria, why not give those variables at the top as strings and then use the variables, that way if anything changes you can just change it at the top.
In your second If block you are moving between ws and two defined sheets. Why not give those sheets a variable as well?
Personally I don't like all the .Copy Destination methods. I like to work directly with the locations like this -
.Range("AG2") = .Range("L2:L" & lrc).SpecialCells(xlCellTypeVisible)
.Range("AE2") = .Range("C2:C" & lrc).SpecialCells(xlCellTypeVisible)
.Range("CD2") = .Range("L2:L" & lrc).SpecialCells(xlCellTypeVisible)

This has the added benefit that if you want to change the worksheets within your If block, you can just slap on a sheet variable.
Also what's up with this -
Application.Goto (ActiveWorkbook.Sheets("SCRIPT").Range("A1"))
Application.Goto (ActiveWorkbook.Sheets("CHAIN").Range("A1"))

The second one negates the first one, or are you just making sure nothing is still selected on those sheets? Nothing should be .Selected anyway.
If you need some more speed try doing these along with ScreenUpdating
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Application.EnableEvents = False

Just remember to turn them back on when you're finished.
One more thing, I don't see any error handling, what happens if there's an error? It will stop and you'll need to loop through all the sheets again once it's fixed.
Oh - also use Option Explicit on the top, you may have caught some of the things I mentioned just by compiling with that engaged.


90 year data simulation in need of performance increase I developed some code for my masters project that will simulate 90 years daily data using 1000 different data sets. The code is working fine and gives the correct output that I wanted but the processing time are very high. It took about 8 hours to finish the simulation.

    tic
%% importing the csv file with selected column
files=dir('*_scen_*.csv');
for i=1:length(files);
    LHR=importcsv(files(i).name);

%% Definable variables
% Define These Value
TAW=-216;     %total available water
RAW=-129;     %readily available water
KC=1.0;       %crop coefficient
IRL=15;       %intense rain level
RC=(80/100);  %percentage of recharge
RO=(1-RC);    %percentage of runoff

% The very first row of Soil Moisture Deficit
for j=1
    SMD(j,i)=(LHR.RAIN(j)-LHR.PET(j));
 if     SMD(j,i)>0;
        SMD(j,i)=0;
 elseif SMD(j,i)<RAW;
        SMD(j,i)=(LHR.RAIN(j)-(LHR.PET(j)*((TAW-SMD(j-1))/(TAW-RAW))));
 end
end

%for the following SMD Calculation
for k=2:(length(LHR.RAIN));
    SMD(k,i)=SMD(k-1,i)+(LHR.RAIN(k)-LHR.PET(k));
    % The SMD conditions
    if SMD(k,i)>0;
       SMD(k,i)=0;
 elseif SMD(k,i)<RAW;
     SMD(k,i)=SMD(k-1,i)+(LHR.RAIN(k)-(LHR.PET(k)*((TAW-SMD(k-1,i))/(TAW-RAW))));
    end
end
%Convert negative SMD to Positive
SMD=abs(SMD);

 %%Evapotranspiration Calculation
 for l=1:(length(SMD));
    if SMD(l,i)<abs(RAW);
        AET(l,i)=LHR.PET(l);
    elseif SMD(l,i)>abs(RAW);
        AET(l,i)=KC*LHR.PET(l)*((abs(TAW)-(SMD(l,i)))/(abs(TAW)-abs(RAW)));
    end
end
for m=2:(length(SMD));
    if SMD(m,i)<abs(RAW);
        AET(m,i)=LHR.PET(m);
    elseif SMD(m,i)>abs(RAW);
       AET(m,i)=KC*LHR.PET(m)*((abs(TAW)-(SMD(m-1,i)))/(abs(TAW)-abs(RAW)));
    end
end
%% HER calculation
for n=1:length(SMD);
    if SMD(n,i)<(LHR.RAIN(n)-AET(n,i));
        HER(n,i)=(LHR.RAIN(n)-AET(n,i)-SMD(n,i));
    elseif SMD(n,i)>(LHR.RAIN(n)-AET(n,i));
        HER(n,i)=0;
    end
end
%% Calculation of recharge anf runoff
for o=1:(length(HER));
    if (HER(o,i)+(abs(TAW)-SMD(o,i)))<abs(TAW);
        RUNOFF(o,i)=0;
    elseif (HER(o,i)+(abs(TAW)-SMD(o,i)))>abs(TAW);
        if HER(o,i)>IRL;
            RUNOFF(o,i)=RO*HER(o,i);
        elseif HER(o,i)<IRL;
            RUNOFF(o,i)=0;
        end
    end
    if (HER(o,i)+(abs(TAW)-SMD(o,i)))<abs(TAW);
        RECHARGE(o,i)=0;
    elseif (HER(o,i)+(abs(TAW)-SMD(o,i)))>abs(TAW);
        if HER(o,i)>IRL;
            RECHARGE(o,i)=RC*HER(o,i);
        elseif HER(o,i)<IRL;
            RECHARGE(o,i)=HER(o,i);
        end
    end
end
%% rainfall
for p=1:length(LHR.RAIN);
RAINFALL(p,i)=LHR.RAIN(p);
PET(p,i)=LHR.PET(p);
end

end
clear i
clear j
clear k
clear l
clear m
clear n
clear o
clear p

toc

Is there any improvement scope for this code that might reduce the processing time? Sorry if the code looks unprofessional; I am in the beginner stage for MATLAB programming.

This cleanup code at the end is indicative of the problem with this program:

clear i
clear j
clear k
clear l
clear m
clear n
clear o
clear p

Nobody is going to reverse-engineer your minified / obfuscated code to understand it.  You will probably have a hard time understanding it yourself if you come back to it after a few weeks.  The program is therefore unmaintainable.

I wouldn't have much confidence in the correctness of the results, either.  The code should be broken down into functions, each of which has a single purpose, specific inputs and outputs, and is separately testable.


API calls organization in Ionic (Angular) application I'm doing a prototype of server API calls in my Ionic (Angular) application. I'm considering this use case:

@IonicPage({/* */})
@Component({/* */})
export class SomePage {
  constructor(private api: Api) { }

  ngOnInit() {
    this.api.user.getList();
  }
}

To achieve this I have something like this:

@Injectable()
export class Api {
  constructor(
    public user: UserApi,
    public event: EventApi,
    public search: SearchApi
  ) {

  }
}

@Injectable()
export class UserApi extends BaseApi {
  private static readonly ENDPOINT: string = "user/";

  constructor(public http: HttpClient) {
    super(UserApi.ENDPOINT);
  }

  getList() {
    return this.http.get(this.url);
  }
}

export class BaseApi {
  private host: string = "https://my-site.com/api/";
  protected url: string;

  constructor(endpoint: string) {
    this.url = this.host + endpoint;
  }
}

Next purposes:

Separate API Service from others Servicies
Split one API Service to little parts where each one works with single essence

And I have a few questions about my approach:

Are there any pitfalls in this approach?
Is this a "good" approach to organize API calls? What can I improve or do in another way?

Your approach is already pretty good.

What I would do to improve your code is the following:

Use a config/constants.ts file for your constants.
Generate two providers (using ionic g provider <providername>).
In the first provider put all the route related code (I allways call it server-communication.ts).
In the second provider put all the logic (I allways call it app-data.ts).
Import the second provider in all the pages where it is needed.

Here is short example:

constants.ts:

/*
 * SERVER
 */
export const AUTH = "Authorization",
  TOKEN_PREFIX = "Token ",
  SERVER_ADDRESS = "https://myserver.com/api/";
/*
 * ROUTES
 */
export const ROUTE_LOGIN = "login/",
  ROUTE_FACEBOOK = "social/facebook/",
  ROUTE_REGISTER = "register/";
// Go on with all the other constants you might need.

server-communication.ts:

import { Injectable } from "@angular/core";
import { Http, Headers, RequestOptions } from "@angular/http";
import "rxjs/add/operator/map";

import {
  SERVER_ADDRESS,
  TOKEN_PREFIX,
  ROUTE_LOGIN,
  ROUTE_FACEBOOK,
  ROUTE_REGISTER
} from "../../config/constants";

@Injectable()
export class ServerCommunicationProvider {
  constructor(private http: Http) {}

authenticate(credentials) {
    return this.http
      .post(SERVER_ADDRESS + ROUTE_LOGIN, credentials)
      .map(res => res.json());
  }

  registerUser(credentials) {
    return this.http
      .post(SERVER_ADDRESS + ROUTE_REGISTER, credentials)
      .map(res => res.json());
  }
  // Go on with your other routes.

  // If you need to specify more data about the request do it like this:
  getObject(objectID: number): Promise<any> {
    return new Promise((resolve, reject) => {
      this.http
        .get(SERVER_ADDRESS + ROUTE_OBJECT + objectId, this.getHttpHeader())
        .subscribe(
          data => {
            resolve(data.json());
          },
          err => {
            console.log(err);
            reject(err);
          }
        );
    });
  }

  //Getting the correct format for the authorization header
  getHttpHeader(): RequestOptions {
    return new RequestOptions({
      headers: new Headers({
        Authorization: TOKEN_PREFIX + this.storageProv.userToken,
        "content-Type": "application/json",
        Accept: "application/json"
      })
    });
  }

And then utilize those appdata.ts:

import { Injectable } from "@angular/core";
import { ServerCommunicationProvider } from "../server-communication/server-communication";

@Injectable()
export class AppDataProvider {
  constructor(
    private serverProv: ServerCommunicationProvider
  ) {}

login(email: string, password: string): void {
    this.loadingProv.authLoading();
    let credentials = {
      username: email,
      password: password
    };
    this.serverProv.authenticate(credentials).subscribe(
      res => {
      // Your login logic here
      },
      err => {
      // What happens when user can't log in here.
      }
    );
  }
  // More functions with logic for your routes.


Table of Tribonacci sequence using NumPy and PANDAS What I am trying to accomplish is calculate the Tribonacci numbers and the ratio. I can do this in Excel easy like this.

So I tried do the same using Python 3 now.
It works and gives such output.

But would you be so kind to review the code and see what obvious mistakes you see in it and advise on what is a better approach as I do not want to continue coding if my logic to tackle such problem is fundamentally flawed somewhere.

Priorities for me are execution time and readability I guess

What would you have done differently and why?
I know it may look like a mess.
From what I heard using exec and eval (code as string) are the bad ideas and also that function calls are expensive?

I don't like my code at all especially how next() is used so many times but hopefully you can see how I was thinking trying to replicate the Excel spreadsheet

To run the code you need numpy and pandas

import pandas as pd
import numpy as np

column_names = ["Number sequence", "Ratio"]
column_counter = len(column_names) -1
header_columns = ['']
while column_counter >= 0:
    column_counter = column_counter - 1
    header_columns.append(column_names[column_counter])

tribonacci_numbers = [0,1]
tribonacci_counter = 19 #Reach
while tribonacci_counter -1 >= 0:
    tribonacci_counter = tribonacci_counter - 1
    tribonacci_numbers.append(sum(tribonacci_numbers[-3:]))
tribonaccis_list_length = len(tribonacci_numbers)-1

index_builder = 1
rows = []
while index_builder <= tribonaccis_list_length:
    try:
        index_builder = index_builder + 1
        rows.append((["Number: "+str(index_builder),tribonacci_numbers[index_builder],tribonacci_numbers[index_builder]/tribonacci_numbers[index_builder-1]]))
    except IndexError:
        continue

def get_row():
    the_row = [x for x in rows]
    row_counter = 0
    while row_counter <= len(the_row):
        try:
            row_counter = row_counter + 1
            yield (the_row[row_counter])
        except IndexError:
            continue

a = get_row()
def therow():
    for q in a:
        yield q

datas = np.array([
header_columns,
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow()),
next(therow())])

df = pd.DataFrame(data=datas[1:,1:],index=datas[1:,0],columns=datas[0,1:])
print(df)

If you want to provide an example that does the same and produces a Pandas dataframe like this it would be very useful for me to study your code.

You’re using the wrong tool for the job. Basically, you do all the computation in Python, use numpy for intermediate storage and pandas for display.

Instead, you should compute the list of tribonacci numbers and from there on use pandas for anything else as it would be much more efficient / readable. I’d keep building the tribonacci numbers in Python as I don't know enough pandas to be able to come up with an efficient way that wouldn't involve Dataframe.append and also because it can be expressed very nicely as a generator:

def tribonacci_sequence():
    a, b, c = 0, 1, 1
    yield a
    yield b
    while True:
        yield c
        a, b, c = b, c, a + b + c

You can then select the amount of numbers you like using itertools.islice and give them to a pandas.Dataframe.

From there on, you can easily compute the ratio by shifting the dataframe and dividing element-wise:

def compute_tribonacci(upper_limit, skip_first_rows=3):
    tribonacci = list(islice(tribonacci_sequence(), upper_limit + 1))
    df = pd.DataFrame({'Number sequence': tribonacci})
    df['Ratio'] = df['Number sequence'] / df.shift(1)['Number sequence']
    df.index = df.index.map('Number: {}'.format)
    return df.iloc[skip_first_rows:]

Last thing to do is to actually ask for a certain amount of rows. Full code:

from itertools import islice
import pandas as pd

def tribonacci_sequence():
    a, b, c = 0, 1, 1
    yield a
    yield b
    while True:
        yield c
        a, b, c = b, c, a + b + c

def compute_tribonacci(upper_limit, skip_first_rows=3):
    tribonacci = list(islice(tribonacci_sequence(), upper_limit + 1))
    df = pd.DataFrame({'Number sequence': tribonacci})
    df['Ratio'] = df['Number sequence'] / df.shift(1)['Number sequence']
    df.index = df.index.map('Number: {}'.format)
    return df.iloc[skip_first_rows:]

if __name__ == '__main__':
    df = compute_tribonacci(50)
    print(df)


Weekday+day validation The string I'd like to check is something like "abcSun24def". If any valid "xxxyy" (xxx = weekday and yy = day) is found, return the position inside the string. If "xxxyy" is not found, return -1.

The code works as desired, but I think it can be optimized.

/* -------------------------------------------------------------
FUNC    : findxy (find pattern xxxyy)
          xxx = weekday (e.g. "Mon01")
          yy = day
          roster specific formatting
PARAMS  : c (char *), pointer to string
RETURNS : (int), if pattern found, pointer to found pattern in string c
          -1 if pattern not found
REMARKS :
---------------------------------------------------------------- */
int findxy(char *c) {
    const char *days[] = { "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
    int i, j;
    char bufw[4];
    char bufd[3];

    /* check if c is at least 5 chars long */
    if (strlen(c) < 5)
        return -1;

    for (i = 0; i <= (int)strlen(c)-5; i++) {
        memcpy(bufw, c+i, 3);
        bufw[3]='\0';
        /* check all 7 weekdays */
        for (j = 0; j < 7; j++) {
            /* find weekday matches */
            if (!strcmp(bufw, days[j])) {
                /* check if both chars following weekday are numerical */
                if (isdigit(c[i+3]) && isdigit(c[i+4])) {
                    memcpy(bufd, c+i+3, 2);
                    bufd[2]='\0';
                    /* check if number after weekday is a valid day */
                    if (atoi(bufd) >= 1 && atoi(bufd) <= 31) {
                        return i;
                    }
                }
            }
        }
    }
    return -1;
}

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

Use the required #includes

The code uses strlen and memcpy which means that it should #include <string.h>.  It was not difficult to infer, but it helps reviewers if the code is complete. It's also an important part of the interface.  I believe these are the required includes:

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

Use const where practical

In your findxy routine, the passed string is never altered, which is just as it should be.  You should indicate that fact by declaring it like this:

int findxy(const char *c)

Check for a null pointer

Things do not go well if the routine is passed a NULL pointer.  On my machine, I get a segmentation fault and a crash.  You can eliminate this hole by adding these lines near the top of the routine:

if (c == NULL) {
    return -1;
}

Use better naming

The days array is well named because it's easy to guess from the name what it contains.  Likewise i and j are commonly used as index variables as you have done in this code.  However, findxy is a rather cryptic name for what this does and c is a poor name for the passed string.  I'd recommend something like this instead:

int findWeekdayDate(const char *str)

Avoid copying if practical

It's not strictly necessary to make copies of portions of the passed string.  With a bit of careful planning, it can be done in place.  Here's one way to do it, although it's not very efficient:

int isValidWeekdayDate(const char *str) {
    static const char *days[] = { "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
    if (str == NULL || strlen(str) < 5) {
        return 0;
    }
    for (const char **dayname = days; *dayname; ++dayname) {
        char *pos = strstr(str, *dayname);
        if (pos && pos == str) {
            if (isdigit(pos[3]) && isdigit(pos[4])) {
                int val = (pos[3]-'0') * 10 + (pos[4]-'0');
                if (val > 0 && val <= 31) {
                    return 1;
                }
            }
        }
    }
    return 0;
}

int findWeekdayDate(const char *str) {
    for (const char *curr = str ; *curr; ++curr) {
        if (isValidWeekdayDate(curr)) {
            return curr-str;
        }
    }
    return -1;
}

Use a finite state machine

We can create a much more efficient routine by creating a finite state machine. For a particular candidate string, we note that the first character must be one of {'F', 'M', 'S', 'T', 'W'}.  If it is not one of those, then the candidate string can be immediately rejected.  Now let's say the first character is 'S'.  In that case the second character must be one of {'a', 'u'}.  We can proceed like this, one character at a time to create a finite state machine.  Here's a visualization of such a state machine:

This is how compiler tools like flex and bison and lex and yacc work.  The code is very efficient but might not be as easy to understand, so it's a tradeoff you should be aware of.

A very pedantic note

Strictly speaking, this line:

int val = (pos[3]-'0') * 10 + (pos[4]-'0');

is guaranteed to be portable.  The C standard requires that encoding of digits is contiguous, so this will work with any character encoding, including EBCDIC, Unicode and ASCII.


Issue a redirect, trying several string replacements for the domain How could I improve this code?

var to = "http://forum.";

if (!RedirectPermanent("http://www.", to))
    if (!RedirectPermanent("http://blog.", to))
        if (!RedirectPermanent("http://forum.", to))
            if (!RedirectPermanent("http://tracker.", to))
                if (!RedirectPermanent("http://wiki.", to))
                    RedirectPermanent("http://", to);

This is the method:

private bool RedirectPermanent(string from,string to)
{
    if (Request.Url.AbsoluteUri.StartsWith(from))
    {
        var domain = Request.Url.GetLeftPart(UriPartial.Authority).Replace(from, to);
        Response.RedirectPermanent(string.Concat(domain, Request.RawUrl));
        return true;
    }

    return false;
}

Went for this:

var arr = new[] { "http://www.", "http://blog.", "http://forum.", "http://tracker.", "http://wiki.", "http://" };
var to = "http://forum.";

foreach (var from in arr)
{
    if (RedirectPermanent(from, to))
        break;
}


JavaScript-based quiz application called Quizz I've been spending lot of time reading through this site and found plenty of resources that shaped up my code.

I am working on an open source project known as Quizz. It is in a primitive stage as I work only during my spare time I cannot concentrate much. For this JavaScript to work, I have a huge objects and array's of topics and questions in a variable called qbank (read question bank).

var qbank   = {};

qbank['js'] = [];
qbank['html']   = [];

qbank['js'].push({
    level:  'basic',
    questions:  [
        {
            title:  'One JS BASIC A question statement?',
            type:   'single',
            anss:   [
                'Answer 1',
                'Answer 2',
                'Answer 3'
            ],
            ans:    [1],
            comments:   'If there is a wrong answer, a justification is needed'

        },

    ]
});

I guess from the above code it is understandable that the topics may be JavaScript/HTML/PHP or Java, and each JavaScript array holds set of questions based on levels (basic, intermediate,... advanced).

The idea is to iterate over array and hold each levels and topics in a separate array variable.

Do you think this approach is good? I really feel there is something wrong with this.

var digest  = function() {
        for( var q in qbank ) {
            //  q   is js/html/php/..... topic names
            var
                topic,
                t,
                qns     = 0,
                i   = 0,
                questions,
                question,
                level
            ;

            topic   = qbank[q];

            //  Initializing instance topics
            _instance.topics[q]= [];

            for(
                var
                    l = levels.length
                ; i < l; i++ ) {

                if( typeof _instance.topics[q][i] === 'undefined' ) {
                    _instance.topics[q][i]  = {
                        'level': levels[i],
                        'questions': []
                    };
                }
            }

            for( t in topic ) {
                questions   = topic[t].questions;
                level       = topic[t].level;

                for( qns in questions ) {
                    for( var
                            inst = 0,
                            l = _instance.topics[q].length; inst < l; inst++ ) {

                        if( _instance.topics[q][inst].level === level ) {
                            _instance.topics[q][inst].questions.push( questions[qns] );
                        }
                    }
                }
            }
        }
    };

Secondly, I have this digest function where it will pull out and merge all the questions based on levels and topics. I really hate to see my code with plenty of for loops. Please help in suggesting a good practice or approach I need to take.

I can understand your frustration of nested for loops.

However, your data structure is full of nested arrays. How else can you handle them?

One way to have pretty code is to separate each loop in its own function, with clear names.

This way, the main logic remains readable. And if you want to go in detail, the only scope you have to care about is a small function.

You can refactor more than in the example given above, it's all up to you.

Example:

var digest  = function() {
        for( var q in qbank ) {
            //  q   is js/html/php/..... topic names
            var
                topic,
                t,
                qns     = 0,
                i   = 0,
                questions,
                question,
                level
            ;

            topic   = qbank[q];

            initInstance( _instance, q );

            for( t in topic ) {
                initTopics( _instance, t, q );

            }
        }

        //  Initializing instance topics
        function initInstance( _instance, q ) {
            _instance.topics[q]= [];

            for(
                var
                    l = levels.length
                ; i < l; i++ ) {

                if( typeof _instance.topics[q][i] === 'undefined' ) {
                    _instance.topics[q][i]  = {
                        'level': levels[i],
                        'questions': []
                    };
                }
            }
        }

        function initTopics( _instance, t, q ) {
            var
                questions   = topic[t].questions,
                level       = topic[t].level
            ;

            for( qns in questions ) {
                for( var
                        inst = 0,
                        l = _instance.topics[q].length; inst < l; inst++ ) {

                    if( _instance.topics[q][inst].level === level ) {
                        _instance.topics[q][inst].questions.push( questions[qns] );
                    }
                }
            }
        }
};


Calculator of combinations without repetition I am interested to find an absolute value (not an approximation) of "combination without repetition" for given \$n\$ and \$k\$, or \$\binom{n}{k}\$.

The brute force solution would look like this

private static ulong Factorial(int x)
{
    ulong res = 1;
    while (x > 1)
    {
        res *= (ulong)x--;
    }
    return res;
}

public static int Combination0(int k, int n)
{
    k = Math.Min(k, n - k);
    if (n < 2 || k < 1) return 1;
    if (k == 1) return n;
    return (int)(Factorial(n) / (Factorial(k) * Factorial(n - k)));
}

We can slightly optimize this solution, by finding \$\prod_{n\geq i>k}{i}\$
instead of \$\frac{n!}{(n-k)!}\$.

private static ulong Factorial(int x, int until = 0)
{
    ulong res = 1;
    while (x > until)
    {
        res *= (ulong)x--;
    }
    return res;
}

public static int Combination1(int k, int n)
{
    k = Math.Min(k, n - k);
    if (n < 2 || k < 1) return 1;
    if (k == 1) return n;
    return (int)(Factorial(n, n - k) / Factorial(k));
}

But these two solutions have one significant problem - we are limited by ulong.MaxValue, which is more than \$20!\$, but less than \$21!\$.

Another way to find the number of combinations, which doesn't have the previously described problem, is the Pascal's triangle.

public static int Combination2(int k, int n)
{
    k = Math.Min(k, n - k);
    if (n < 2 || k < 1) return 1;
    if (k == 1) return n;
    int[] triangle = new int[k + 1];
    triangle[0] = 1;

    // expanding
    int i = 0;
    for (; i < k; i++)
    {
        for (int j = i + 1; j > 0; j--)
        {
            triangle[j] += triangle[j - 1];
        }
    }

    // progressing
    for (; i < n - k; i++)
    {
        for (int j = k; j > 0; j--)
        {
            triangle[j] += triangle[j - 1];
        }
    }

    // collapsing
    for (; i < n; i++)
    {
        int until = k - (n - i);
        for (int j = k; j > until; j--)
        {
            triangle[j] += triangle[j - 1];
        }
    }
    return triangle[k];
}

But the problem is that Combination2 is significantly slow.

I would appreciate any comments and suggestions for an improvement.

Update

@quasar and @henrik-hansen suggested the way to prevent overflow by calculating \$\prod_{0 \leq i < k}{\frac{n-i}{i+1}}\$.

One trick is to keep the partial products in small, \${n}\choose{k}\$ so they don't overflow.

I iteratively multiply \$n/(n-k)\$ by \$(n-1)/(n-k-1)\$, cache the result in an accumulator, multiply that by \$(n-2)/(n-k-2)\$ and so forth.

#include <iostream>

template <class T>
T choose(T n, T k)
{
    T accum = 1;
    T m = n;
    for (T i = 1; i <= m - k; i++)
    {
        accum = accum * n / (n - k);
        n--;
    }
    return accum;
}

int main()
{
    std::cout << std::fixed;
    long double n = 50, k = 25;
    std::cout << "\nLDBL_MAX" << LDBL_MAX;
    long double result = choose(n, k);
    std::cout << "\nC(" << n << "," << k << ") = " << result;
    std::cin.clear();
    std::cin.get();
    return 0;
}


Entab.c - Program that replaces string of blanks by the min # of spaces & tabs to achieve identical output This is Exercise 1-21 of K&R (2nd edition).

I've seen someone else's post about it but it doesn't solve the problem the way my code does, and I wasn't sure whether I was allowed to post my code as an answer and ask for feedback (it just seemed better to create a new thread and ask for advice here, didn't wanna pollute the other user's topic but if that's what I am supposed to do please let me know and I'll reply over there).

In any way, here's the approach I took to solve the problem:

  Approach:

  We'll keep track of number of spaces encountered (between nontab/nonspace characters)
  We'll keep track of characters (that aren't tabs/blanks/newlines) per input line
  We'll evaluate the "gaps" generated by spaces by:

  Evaluating whether the number of spaces in between those characters.
  A gap will be "big enough" when the number of spaces is >= TABSIZE
  Then, for all the left over spaces in our "buffer", we'll print them out individually

  Finally, we print out the character that was read in (which was not a tab/blank)

  As well as updating space count and character count if necessary.

Here's the code:

#include <stdio.h>

//Entab: replaces string of blanks by the min # of spaces & tabs
//Same tab spots as for detab (8)
//Given performance to tabs.

#define TABSIZE 8

//Approach:
//We'll keep track of number of spaces encountered (between nontab/nonspace characters)
//We'll keep track of characters (that aren't tabs/blanks/newlines) per input line
//We'll evaluate the "gaps" generated by spaces by:
//  evaluating whether the number of spaces in between those characters.
//  A gap will be "big enough" when the number of spaces is >= TABSIZE
//  Then, for all the left over spaces in our "buffer", we'll print them out individually
//Finally, we print out the character that was read in (which was not a tab/blank)
//Update space count and character count if necessary.

int main()
{
    int c, nc, ns;

    nc = ns = 0;
    while((c = getchar()) != EOF)
    {
        //if this character was a space
        if(c == ' ') {
            //increase the space count
            ++ns;
        }
        //else if the character read in is not a space
        else {

            //while one or more spaces are still "in buffer"
            while(ns > 0)
            {
                //decide whether gap in between chars is long enough for a space
                if(ns >= TABSIZE - nc)
                {
                    //if it is, then place a tab, and remove that amount of spaces
                    putchar('\t');
                    ns -= TABSIZE;
                }
                //else, if the gap is not big enough for spaces, just print them out individually
                else
                {
                    putchar(' ');
                    --ns;
                }
            }

            //if read a newline, reset char count
            //if not, increase it by one for the char we just read in
            //as well as update the total space count to 0
            if(c == '\n')
                nc = ns = 0;
            else if(c != '\t')
                ++nc;

            //finally, put this "nonspace/nontab" character just read
            //after the gap we just covered
            putchar(c);
        }
    }
}

Also, keep in mind that I am just revisiting C's K&R and I am aware that some of it may be outdated, but I'm doing my best to use what's still "useful" of the book and discard what may not be.

I'm using these notes to accompany my travels through C.

A good implementation of this problem.  You've avoided some of the hurdles (such as remembering to store the result of getchar() in an int if you want to compare against EOF).
Most of what I say will be about style and readability.
Be consistent with your braces
We have a few different styles here:
int main()
{

Brace on its own line.  I like that for function definitions (though others differ).
    while((c = getchar()) != EOF)
    {

Another brace getting its own line.
        if(c == ' ') {
            //increase the space count
            ++ns;
        }
        //else if the character read in is not a space
        else {

Here, the brace shares the same line as the if.  And the else shares a line with only one of the braces.
            if(c == '\n')
                nc = ns = 0;
            else if(c != '\t')
                ++nc;

Here, we have no braces at all.  While that's legal, it's a good idea to always include braces (and I was reminded so recently, when I posted my own code for review!).
I would write the above like this:
int main()
{

    while((c = getchar()) != EOF) {

        if(c == ' ') {
            ++ns;
        } else {
            // not a space

            if(c == '\n') {
                nc = ns = 0;
            } else if(c != '\t') {
                ++nc;
            }

Alternatively, as:
int main()
{

    while((c = getchar()) != EOF)
    {

        if(c == ' ')
        {
            ++ns;
        }
        else
        {
            // not a space

            if(c == '\n')
            {
                nc = ns = 0;
            }
            else if(c != '\t')
            {
                ++nc;
            }

Naming
It's quite conventional to use c for a variable holding the current character.  But nc and ns are less obvious.  You could change the names; alternatively, add comments where they are declared:
    int nc = 0;                 /* number of characters read in this line */
    int ns = 0;                 /* number of consecutive spaces seen */

Comments
Generally the comments are pretty good.  I think you've over-commented a few things that are obvious to an experienced C programmer, but that's natural as you are learning - with experience, you'll recognise parts that are not idiomatic and concentrate your comments there, where they are most needed.
My personal taste is to avoid duplicating the code, so I wouldn't write "else" in the comment when it immediately follows an else in the code - I hope the samples below demonstrate what I mean.
Simplify loops
This loop is quite complex:
        //while one or more spaces are still "in buffer"
        while (ns > 0) {
            //decide whether gap in between chars is long enough for a space
            if (ns >= TABSIZE - nc) {
                //if it is, then place a tab, and remove that amount of spaces
                putchar('\t');
                ns -= TABSIZE;
            } else {
            // the gap is not big enough for spaces, just print them out individually
                putchar(' ');
                --ns;
            }
        }

We can replace with two simpler loops:
            //while one or more spaces are still "in buffer"
            while (ns >= TABSIZE - nc) {
                // place a tab, and remove as many spaces
                putchar('\t');
                ns -= TABSIZE;
            }
            while (ns > 0) {
                // use spaces
                putchar(' ');
                --ns;
            }

Finally
I'm not sure whether you've actually got the logic correct.  Remember that a tab jumps to the next tab stop - it may be equivalent to less than 8 spaces.  Have you tested with a variety of inputs (including tabs in input, and e.g. a run of 4 or 5 spaces after some non-space characters)?
The logic might be simpler if we keep track of current read column and current output column - at the end of a run of spaces we then just need to do the correct action to make the output column match the input column (using tabs where possible).
As a hint, if the current (0-based) column number is n, then a tab will advance the column number by TABSIZE - n % TABSIZE positions.