Proper way to work with stored procedures in Entity Framework Core I need to work with stored procedures, with the help of Entity Framework Core. This is my function:

public async Task<bool> CheckIfUserRegistered(string phoneNumber, DateTime dateOfBirth)
    {
        if (string.IsNullOrWhiteSpace(phoneNumber))
        {
            return false;
        }
        using (var cmd = _dbContext.Database.GetDbConnection().CreateCommand())
        {
            cmd.CommandText = "dbo.CheckIfUserRegistered";
            cmd.CommandType = CommandType.StoredProcedure;

            cmd.Parameters.Add(new SqlParameter("@phoneNumber", SqlDbType.NVarChar) { Value = phoneNumber });
            cmd.Parameters.Add(new SqlParameter("@dateOfBirth", SqlDbType.Date) { Value = dateOfBirth });

            cmd.Parameters.Add(new SqlParameter("@registered", SqlDbType.Bit) { Direction = ParameterDirection.Output });

            if (cmd.Connection.State != ConnectionState.Open)
            {
                cmd.Connection.Open();
            }

            await cmd.ExecuteNonQueryAsync();

            return (bool)cmd.Parameters["@registered"].Value;
        }
    }

I'm not sure, with the correctness of this function, is this the right way to work with stored procedures in EF Core? Don't I have problems with forgotten connections, memory leakings, etc?

The biggest thing I would change is to return the connection to the state you found it.

bool isOpen = cmd.Connection.State == ConnectionState.Open;
if (!isOpen)
{
    cmd.Connection.Open();
}

await cmd.ExecuteNonQueryAsync();

if (!isOpen)
{
    cmd.Connection.Close();
}

Then, from there, I would create a helper method to build the SqlParameter objets for you:

private SqlParameter BuildParamter(string name, SqlDbType type, object value, ParameterDirection? direction)
{
    var parameter = new SqlParameter(name, type);

    if (value != null)
    {
        parameter.Value = value;
    }

    if (direction.HasValue)
    {
        parameter.Direction = direction.Value;
    }

    return parameter;
}

Then your parameters go from:

cmd.Parameters.Add(new SqlParameter("@phoneNumber", SqlDbType.NVarChar) { Value = phoneNumber });
cmd.Parameters.Add(new SqlParameter("@dateOfBirth", SqlDbType.Date) { Value = dateOfBirth });

cmd.Parameters.Add(new SqlParameter("@registered", SqlDbType.Bit) { Direction = ParameterDirection.Output });

To:

cmd.Parameters.Add(BuildParameter("@phoneNumber", SqlDbType.NVarChar, phoneNumber));
cmd.Parameters.Add(BuildParameter("@dateOfBirth", SqlDbType.Date, dateOfBirth));

cmd.Parameters.Add(BuildParameter("@registered", SqlDbType.Bit, null, ParameterDirection.Output));

It's less verbose, but if you build a lot of SqlParameter objects it might be handy to have one method you can use for all of them. (You can optionally add a int? size parameter to the method as well.)

Then, extract a SetStoredProcedure method which would take the input SqlCommand and string, set the type and command text to "dbo." + name, then you add the parameters, then ExecuteStoredProcedure would do the connection checking and then you can return your cmd.Parameters["@registered"].Value as you are. In the end something like:

SetStoredProcedure("CheckIfUserRegistered", cmd):

cmd.Parameters.Add(BuildParameter("@phoneNumber", SqlDbType.NVarChar, phoneNumber));
cmd.Parameters.Add(BuildParameter("@dateOfBirth", SqlDbType.Date, dateOfBirth));

cmd.Parameters.Add(BuildParameter("@registered", SqlDbType.Bit, null, ParameterDirection.Output));

ExecuteStoredProcedure(cmd);

return (bool)cmd.Parameters["@registered"].Value;

Optionally with your async/await pattern.

This way, if you create more stored procedures it's less work to make them operate.

That's way too heavy to call a stored procedure. Here is my case.
I have a SP called "DeleteBusiness" which takes 1 parameter.
Entity Framework 6 will generate a method in DataModel.context.cs as follows,

public virtual int DeleteBusiness(Nullable<int> p_COMPID)
{
    var p_COMPIDParameter = p_COMPID.HasValue ?
        new ObjectParameter("p_COMPID", p_COMPID) :
        new ObjectParameter("p_COMPID", typeof(int));

    return ((IObjectContextAdapter)this).ObjectContext.ExecuteFunction("DeleteBusiness", p_COMPIDParameter);
}

Then in my code I just call the method in DbContext like this,

DbContext.DeleteBusiness(compId);


Finding indices of numbers that sum to a target I've taken this algorithm question, which is to find the indexes of any two numbers in an array that sum to a given number. It is quite a simple problem to solve. But it is the execution time of my code algorithm that bugs me.

#include <iostream>
#include <vector>
using namespace std;

class TwoSum
{
public:
  static std::pair<int, int> findTwoSum(const std::vector<int>& list, int TargetSum)
  {
    for (int increment = 0; increment < list.size(); increment++)
    {
        for (int i = 0; i < list.size(); i++)
        {
            if (list.at(increment) + list.at(i) == TargetSum)
            {
                return make_pair(increment, i);
            }
        }
    }
    return make_pair(-1, -1);
  }
};

int main(int argc, const char* argv[])
{
   vector<int> list = {1, 3, 5, 7, 9 };
   int targetSum = 12;
   pair<int, int> indices = TwoSum::findTwoSum(list, targetSum);

   indices.first == -1 && indices.second == -1 ?
   cout << "No vector elements can be added to achieve the target sum" << endl :
   cout << "The target sum can be achieved by adding the vector element " << indices.first << " and " << indices.second;
   cout << endl;

   return 0;
}

It is guaranteed correct as I tested it with the website's compiler (or whatever you call it), they can test your code just by pasting and running it on their code editor. I tested this on my IDE as well, and it worked fine.

The problem is, my code failed the performance test when dealing with large number of vector elements as it took longer than the website's expectation. For me, the function named findTwoSum()is already straightforward. And I don't have any idea how to optimize this function. Note that the test's focus here is the function itself and how fast it executes.

Any modifications in function's return type and parameter is not allowed. As the website stated that you cannot modify the main function's contents, implying that you cannot also change how the function is called.

But I slightly modified the main function for you to know what the program is trying to achieve.

Not quite right

First off, you're checking every cross pair of indices against each other. But that means that findTwoSum({1, 3, 4}, 6) will succeed, even though there is no pair of elements that sum to 6 - because you count the 3 twice. You need: "any two distinct elements".

Iterating over vectors

list is a confusing name for a vector. increment is a poor variable name for this - since it's not incrementing anything, it's just another index, so you really should use i and j instead. Also you don't need at() - that member function does range checking. But your indices will all definitely be in range, so you could simply write:

for (size_t i = 0; i < v.size(); ++i) {
    for (size_t j = i+1; j < v.size(); ++j) { // correcting initial bug
        if (v[i] + v[j] == target) {
            return std::make_pair(i, j);
        }
    }
}
return std::make_pair(-1, -1);

Failure case

Rather than using sentinel values, I prefer to use something like Boost.Optional to indicate failure, so that:

boost::optional<std::pair<int, int>> findTwoSum(const std::vector<int>&, int target);

Classes

Why is findTwoSum a static member function? Just make it a free function. There's no reason for a class. You could instead make TwoSum a namespace but I don't see the point of that either.

The Algorithm

You are checking every pair of indices - that's \$O(n^2)\$. We can do way better. Consider that once we have our first index - there is one unique element that we need to find to see if we have a success case: target - v[i].

We could simply throw all of our indices into a map:

std::unordered_map<int, size_t> indices;
for (size_t i = 0; i < v.size(); ++i) {
    indices.insert(std::make_pair(v[i], i));
}

And then simply search if target - v[i] is in indices for each such i:

for (size_t i = 0; i < v.size(); ++i) {
    auto j = indices.find(target - v[i]);
    if (j != indices.end()) {
        return std::make_pair(i, j->second);
    }
}

This will be \$O(n)\$ to create the hashtable, and then another \$O(n)\$ to do all the searching... for a grand total of \$O(n)\$.

This piece of code returns the indices of array whose values add up to a target "sum". It handles unsorted array with duplicate entries.

In Perl with 0(n) time complexity using hash map:

#!/usr/bin/perl

use strict;

use warnings;

use Data::Dumper;

my @array=(41,7,2,3,4,6,1,10);

#lets assume we want 2 indexes from the above array

#which will add up to sum 10

my $sum = 10;

my ($idx1, $idx2); # these are the indexes

my $i = 0;

#convert the array into hash with keys starting from 0 and

#array elements stored as values

my %my_hash = map {$i++ => $_}@array;

print Dumper \%my_hash;

# create a reverse hash to retrieve keys from values

my %rhash = reverse %my_hash;

#iterate through the first hash, we need to make sure the

#hash is sorted so that the hash keys match with array index order

for my $key (sort keys %my_hash) {

    #this is to store the second value which adds

    #to first value $my_hash{$key} to be equal to sum

    my $second_val = $sum-$my_hash{$key};

    #if the second key exists in reverse hash, we

    #can easily find the index

    if (defined $rhash{$second_val}) {

        $idx1= $key;

        $idx2= $rhash{$second_val};

        print "Indexes are $idx1 $idx2\n";

      }

   }


Caesar cipher in Ruby I must implement a simple Caesar cipher class. Initializer takes two parameters - number of positions to shift the alphabet (integer) and the alphabet used (as string, not necessarily the standard English alphabet). The class must have two instance methods encrypt and decrypt, which take a string and do the respective action based on the parameters of the initializer. These are my rspec tests:

describe "Caesar" do

  latin_encrypter = Caesar.new 4
  dna_encrypter = Caesar.new 3, 'ACTG'

  it 'encrypts correctly' do
    expect(latin_encrypter.encrypt 'hello').to eq "lipps"
  end

  it 'encrypts correctly using DNA alphabet' do
    expect(dna_encrypter.encrypt 'ACCTGA').to eq "GAACTG"
  end

  it 'decrypts correctly' do
    expect(latin_encrypter.decrypt 'lipps').to eq 'hello'
  end

  it 'decrypts correctly using DNA alphabet' do
    expect(dna_encrypter.decrypt 'GAACTG').to eq 'ACCTGA'
  end
end

and this is my implementation:

class Caesar
  def initialize(shift, alphabet = 'abcdefghijklmnopqrstuvwxyz')
    @shift = shift % alphabet.size
    @alphabet = alphabet
  end

  def encrypt(string)
    string.chars.map { |char| @alphabet[@alphabet.index(char) + @shift - @alphabet.size] }.join
  end

  def decrypt(string)
    string.chars.map { |char| @alphabet[@alphabet.index(char) - @shift] }.join
  end
end

Is this an optimal algorithm for encryption/decryption or is there a better one (perhaps using gsub)?

The only (conceptual) problem I see in your code is that both encrypt/decrypt perform a O(n) operation (detect) for every character (so at then end it's a O(n*m) algorithm), that's unnecessarily inefficient. Build a hash object to use as an indexer:

class Caesar
  def initialize(shift, alphabet = ('a'..'z').to_a.join)
    @shift = shift
    @alphabet = alphabet
    @indexes = alphabet.chars.map.with_index.to_h
  end

  def encrypt(string)
    string.chars.map { |c| @alphabet[(@indexes[c] + @shift) % @alphabet.size] }.join
  end

  def decrypt(string)
    string.chars.map { |c| @alphabet[(@indexes[c] - @shift) % @alphabet.size] }.join
  end
end

You can also build encrypter/decrypter hash tables at initialization:

class Caesar
  def initialize(shift, alphabet = ('a'..'z').to_a.join)
    chars = alphabet.chars.to_a
    @encrypter = chars.zip(chars.rotate(shift)).to_h
    @decrypter = @encrypter.invert
  end

  def encrypt(string)
    @encrypter.values_at(*string.chars).join
  end

  def decrypt(string)
    @decrypter.values_at(*string.chars).join
  end
end

Your code does not work with letters like "A..Z".
The following does:

puts "Text for encrypt please: "
text_for_encrypt = gets.chomp

puts "Input encrypt key: "
key = gets.chomp.to_i

text_arrow = text_for_encrypt.split('').to_a
alphabet_az = ("a".."z").to_a.join
alphabet_AZ = ("A".."Z").to_a.join
i = key % alphabet_az.size

encrypt_az = alphabet_az.chars.rotate(i).join
encrypt_AZ = alphabet_AZ.chars.rotate(i).join

res = []
text_arrow.each do |letter|
  if ("a".."z") === letter
    letter = letter.tr( alphabet_az, encrypt_az )
    res << letter
  else
    letter = letter.tr( alphabet_AZ, encrypt_AZ )
    res << letter
  end
end

puts "Your code: #{res.join}"


Simple random number guessing game I made this simple game code. Can you please give me some advice on how I can improve it?

#include<stdio.h>
#include<stdlib.h>
#include<conio.h>
#include <time.h>
#include <windows.h>

void main ()
{
    printf("\t\tWelcome to our gessing game\n\n\n");
    printf("the computer choose a random number between 1-99, try to gess the random number...");

    srand(time(NULL));

    int gess,i=0,found=0,  r = rand()%100;

    while (i<10 && found==0)
    {
        printf("\n My gess is:\t");
        scanf("%d",&gess);
        if (gess==r)
        {
            found=1;
            i++;
        }
        else if(gess>r)
        {
            printf("\n Too big\n");
            i++;
        }
        else if(gess<r)
        {
            printf("\n Too small\n");
            i++;
        }
    }
    if (found==1&&gess==1)
        printf("\n Very good the number is %d this is your %dst gess",gess,i);
    else if(found==1&&gess==2)
        printf("\n very good the number is %d this is your %dnd gess",gess,i);
    else if(found==1&&gess==3)
        printf("\n very good the number is %d this is your %drd gess",gess,i);
    else if(found==1&&gess!=1&&gess!=2&&gess!=3)
        printf("\n very good the number is %d this is your %dth gess",gess,i);
    else
        printf("\n Never mind try again");
    getch();
}

One variable per line please.

int gess,i=0,found=0,  r = rand()%100;

This makes the code hard to read. There is also one corner case (with pointers) were this will not work as expected for beginners. As a result this is usually banned in most companies coding standards.

You are using found as a boolean variable.
Treat it as such. use TRUE/FALSE.

I would change the while() into a for(;;) loop. Then you can remove all the increments.

while (i<10 && found==0)

--

for(int i = 0; i < 10 && !found; ++i)

White space is your friend:

if (found==1&&gess==1)  // This is hard to read and will get worse with more conditions.

if (found && (guess == 1)) // much more readable
//  ^^^^^  boolean use it as such.

Your "st", "nd", "rd", "th" is not correct.

10-19    => th
x1       => st
x2       => nd
x3       => rd
x[4-9]   => th

Also you print basically the same string every time on success. So we not have one print statement and just separate the logic for getting the ending.
I would change the logic:

if (!found)
{
    printf("\n Never mind try again");
}
else
{
    char const* ext = "th";
    if ((gess < 10) || (gess >= 20))  // 10->19 already set
    {
        int end = gess % 10;
        if      (end == 1) { ext = "st";}
        else if (end == 2) { ext = "nd";}
        else if (end == 3) { ext = "rd";}
    }

    printf("\n Very good the number is %d this is your %d%s gess", gess, i, ext);
}

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

int main()  // Main always return int ...ALWAYS...
    {
        printf("\t\tWelcome to our gessing game\n\n\n");   // This could be device specific. If i run it on a minimized screen it might look akward

        /*
            maybe, call a function like take_device_width() then use the value returned to
            print the welcome message
        */
        printf("the computer choose a random number between 1-99, try to gess the random number...");

        /* You could write something like this

            #define MIN_NUM 1
            #define MAX_NUM 99

            printf(""The Computer chooses a random number between %d and %d. \
                   You have to guess it using minumun number of guesses\n", MIN_NUM, MAX_NUM);

            So that when ever you want to change the values for number range you can just
            change it once at pre processor macro.

        */

        srand(time(NULL));     // srand((unsigned) time(NULL));

        /* Dont use bad spellings and always indent code cleanly: i,e int guess, i = 0, found = 0*/

        int gess,i=0,found=0,  r = rand()%100;

        /* INstead of saying that number of guesses is i, you can name it as num_of_guesses. So that next time
           when you look at code you will know what num_of_guesses does . Similarly for r.
        */

         /* Instead of using magic number 10, you could write it as a macro:

            #define MAX_ATTEMPTS 10

        */

        while (i<10 && found==0)        // indent it here: while(num_of_guesses < MAX_ATTEMPT && found == 0)
        {
            printf("\n My gess is:\t"); // no need of \t here
            scanf("%d",&gess);

            /* Indent it. After a block that does something add a comment of what next block does nad write that code */
            if (gess==r)
            {
                found=1;    // instead of found we can do this

                /* if(guess == r)
                   {
                        num_of_guesses++;
                       printf("You guessed the correct answer in %d guesses", num_of_guesses);
                       exit(0);
                   }

                   will avoid checking for while(condition) even after guessing correct answer
                */
                i++;
            }

            else if(gess>r)
            {
                printf("\n Too big\n");
                i++;
            }
            else                            // else is enough here
            {
                printf("\n Too small\n");
                i++;
            }

            /* most of the newline char are unneccesary */
        }

        /* Indent here */

        /* This looks daunting. What are you trying to do here. Why so many conditions */

        if (found==1&&gess==1)
            printf("\n Very good the number is %d this is your %dst gess",gess,i);
        else if(found==1&&gess==2)
            printf("\n very good the number is %d this is your %dnd gess",gess,i);
        else if(found==1&&gess==3)
            printf("\n very good the number is %d this is your %drd gess",gess,i);
        else if(found==1&&gess!=1&&gess!=2&&gess!=3)
            printf("\n very good the number is %d this is your %dth gess",gess,i);
        else
            printf("\n Never mind try again");
        getchar();

        /* it could be
         * if(num_of_guesses >= MAX_ATTEMPTS)
           {
               printf("You couldn't guess it in suggested attempts. Sorry!");
               exit(1);

               you can even add a yes or no condition to play again

           }

        */
       return 0;
    }// end main


Console word wrapping I wrote a text adventure game a while back and today I looked at it to see how I did the word wrapping (to use in a new project); I'm curious if I went about it the right way. I realize I could use an array instead of a list and remove 2 dependencies from the class but wanted some other feedback as I may have missed something obvious.

    public static string GetBorderedText(string Input, int AbsoluteLength, bool splitLines = true, int Offset = -4)
    {
        string outputString = "";

        if(Input.Length > AbsoluteLength + Offset && splitLines)
        {
            List<string> words = Input.Split(' ').ToList();
            string lineConstruction = "";

            foreach (string s in words)
            {
                if (lineConstruction.Length + s.Length + 1 >= AbsoluteLength + Offset)
                {
                    outputString += "+ " + GetPaddedSubstring(lineConstruction, AbsoluteLength, Offset) + " +" + Environment.NewLine;
                    lineConstruction = "";
                    lineConstruction += s + " ";
                }
                else
                {
                    lineConstruction += s + " ";
                }
            }

            outputString += "+ " + GetPaddedSubstring(lineConstruction, AbsoluteLength, Offset) + " +";

            return outputString;
        }
        else
        {
            outputString = "+ " + GetPaddedSubstring(Input, AbsoluteLength, Offset) + " +";
            return outputString;
        }
    }

where GetPaddedSubstring  is used for getting a substring padded to a certain length.

    public static string GetPaddedSubstring(string Input, int AbsoluteLength, int Offset)
    {
        int targetLength = AbsoluteLength + Offset;
        string output = "";

        if(Input.Length > targetLength)
        {
            return Input.Substring(0, targetLength);
        }
        else
        {
            output = Input.PadRight(targetLength);

            return output;
        }
    }

Lastly, for some examples. GetBorderedText can take in something like

"This room looks sturdy, but old. Lots of storage and containers. There is a door on the far side."

And return something like this.

+ This room looks sturdy, but old. Lots of storage and containers. There is a door     +
+ on the far side.                                                                     +

The + on the side are just to create a nice border within the Console window. (Hence the -4 default on Offset)

GetBorderedText(string Input, int AbsoluteLength, bool splitLines = true, int Offset = -4)

You are very inconsistent with naming parameters. Some of them are in PascalCase while only one has the correct camelCase. This makes your code look strange and unprofessional.

if (Input.Length > AbsoluteLength + Offset && splitLines)

Conditions like this are better stored in a variable that describes what its purpose is and split into two. Consider this:

var wrapRequired = input.Length > absoluteLength + offset;
if (wrapRequired && canWrap)

input.Split(' ').ToList()

You don't need to call ToList, it's already an array and can be enumerated.

foreach (string s in words)

If your collection is called words then name the item word and not s. It's very confusing when one looks at the code and wonders what that s is.

lineConstruction.Length + word.Length + 1 >= absoluteLength + offset

The same here. You should name this condition propertly and especially the magic 1.

Other than this

you should use StringBuilder for concatenating strings in loops because it's not only faster but also easier to use.
make the border as prefix & suffix parameters instead of hardcoding them (or leftBorder & rightBorder).
use variables with stronger names and use more of them so that they document your code.

As an example of how you can apply all these suggestions here's the new code with one new method for adding the border.

public static string CreateBorderedText(string input, int maxLength, bool canWrap = true, string prefix = "+ ", string suffix = " +")
{
    const string wordSeparator = " ";

    var lines = new List<string>();
    var lineBuilder = new StringBuilder();
    var borderLength = prefix.Length + suffix.Length;
    var contentLength = maxLength - borderLength;

    var wrapRequired = input.Length > contentLength;
    if (wrapRequired && canWrap)
    {
        var words = input.Split(' ');
        foreach (var word in words)
        {
            var projectedLength = lineBuilder.Length + word.Length + wordSeparator.Length + borderLength;
            var wrap = projectedLength >= maxLength;
            if (wrap)
            {
                PadRight(lineBuilder, contentLength);
                AddBorder(lineBuilder, prefix, suffix);

                lines.Add(lineBuilder.ToString());
                lineBuilder.Clear();
            }

            lineBuilder.Append(word).Append(wordSeparator);
        }
    }
    else
    {
        lineBuilder.Append(input);
    }

    PadRight(lineBuilder, contentLength);
    AddBorder(lineBuilder, prefix, suffix);
    lines.Add(lineBuilder.ToString());

    return string.Join(Environment.NewLine, lines);
}

and the two helpers

private static void AddBorder(StringBuilder lineBuilder, string prefix, string suffix)
{
    lineBuilder
        .Insert(0, prefix)
        .Append(suffix);
}

private static void PadRight(StringBuilder lineBuilder, int targetLength)
{
    lineBuilder.Append(' ', targetLength - lineBuilder.Length);
}

Here is your code cleaned up a bit (even removed 2 dependencies):

namespace Tests
{
    using System;

    class Program
    {
        private static void Main(string[] args)
        {
            Console.WriteLine(GetBorderedText("This room looks sturdy, but old. Lots of storage and containers. There is a door on the far side.", 80));
            Console.ReadKey();
        }

        private static string GetBorderedText(string input, int maxLength, bool splitLines = true, int offset = -4)
        {
            if (input.Length <= maxLength + offset && splitLines)
                return $"+ {GetPaddedSubstring(input, maxLength, offset)} +";

            var outputString = string.Empty;
            var lineConstruction = string.Empty;

            foreach (var s in input.Split(' '))
            {
                if (lineConstruction.Length + s.Length + 1 >= maxLength + offset)
                {
                    outputString += $"+ {GetPaddedSubstring(lineConstruction, maxLength, offset)} +{Environment.NewLine}";

                    lineConstruction = string.Empty;
                }

                lineConstruction += $"{s} ";
            }

            outputString += $"+ {GetPaddedSubstring(lineConstruction, maxLength, offset)} +";

            return outputString;
        }

        private static string GetPaddedSubstring(string input, int maxLength, int offset)
        {
            var targetLength = maxLength + offset;

            if (input.Length > targetLength)
                return input.Substring(0, targetLength);

            return input.PadRight(targetLength);

        }
    }
}

You could put a StringBuilder in there as mentioned earlier but if the number of lines is not many, I wouldn't bother.


Finding the second largest value In an Array findSecondLargestNumberInTheArray() method returns the second highest number from an array.

Could you please review this code to enhance performance.

public static int findSecondLargestNumberInTheArray(int array[]) {
    // Initialize these to the smallest value possible
    int highest = Integer.MIN_VALUE;
    int secondHighest = Integer.MIN_VALUE;

    // Loop over the array
    for (int i = 0; i < array.length; i++) {

        // If current element is greater than highest
        if (array[i] > highest) {

            // assign second highest element to highest element
            secondHighest = highest;

            // highest element to current element
            highest = array[i];
        } else if (array[i] > secondHighest)
            // Just replace the second highest
            secondHighest = array[i];
    }

    // After exiting the loop, secondHighest now represents the second
    // largest value in the array
    return secondHighest;
}

Several comments are unnecessary:

    // Loop over the array
    for (int i = 0; i < array.length; i++) {

    // assign second highest element to highest element
    secondHighest = highest;

I recommend do use braces { ... } for all if-blocks, even if that
consists only of a single statement, and definitely if there is
an additional comment line:

        } else if (array[i] > secondHighest)
            // Just replace the second highest
            secondHighest = array[i];

Iteration over all array elements can be simplified with an
“enhanced for loop:”

    for (int elem : array) {
        // ...
    }

Your function returns Integer.MIN_VALUE for an empty or single-element
array, which is fine if specified as such. An alternative would be
to throw an exception for arrays with less than two elements.

With respect to performance, I doubt there is much to improve.
The function iterates once over all array elements, which looks pretty
much optimal.

public static int findSecondLargestNumberInTheArray(int array[]) {
// Initialize these to the smallest value possible
int highest = Integer.MIN_VALUE;
int secondHighest = Integer.MIN_VALUE;

// Enhanced ForLoop
for (int elements: array ) {

    // If current element is greater than highest
    if (elements > highest) {
        secondHighest = highest;
        highest = elements;
    } else if (elements > secondHighest && elements < highest)

        secondHighest = elements;
}

//Returning the second Highest from the arrays of elements
return secondHighest;

}

** Made changes in your logic so that it gonna give correct results **

and use of for loop will somewhat increase the performance*


Finding the latest file in a folder I'm a bit new to Python and sort of learning on my own. I wrote a small function to help me find the latest file in a directory. Taking a step back, it reads a bit janky and I was curious what steps or what resources I could look into to help me make this more friendly. Should I be returning False? Or 0?
Inside my example/files directory are 3 files which were, for this example, created on the dates specified in the file name:

example/files/randomtext011.201602012.txt
example/files/randomtext011.201602011.txt
example/files/randomtext011.201602013.txt

import os.path
import glob
import datetime

dir = 'example/files'
file_pattern = 'randomtext011.*.txt'

def get_latest_file(file_pattern,path=None):
    if path is None:
        list_of_files = glob.glob('{0}'.format(file_pattern))
        if len(list_of_files)> 0:
            return os.path.split(max(list_of_files, key = os.path.getctime))[1]
    else:
        list_of_files = glob.glob('{0}/{1}'.format(path, file_pattern))
        if len(list_of_files) > 0:
            return os.path.split(max(list_of_files,key=os.path.getctime))[1]
    return False

First of all, I think that your variable names are quite good.

  Should I be returning False? Or 0?

I would recommend None

Don't repeat yourself

As you can see, the two branches of your if else are very similar.

Instead you could do a

if path is None:
    fullpath = file_pattern
else:
    fullpath = path + '/' + file_pattern

But joining paths like this is not very pythonic (and might cause problems on windows).

Instead, fullpath = os.path.join(path, file_pattern) is what you are looking for.

About the arguments

You can take inspiration of the os.path.join even further and change the order of your arguments (and completely remove the branching):

def get_latest_file(path, *paths):
    fullpath = os.path.join(path, paths)
    ...
get_latest_file('example', 'files','randomtext011.*.txt')

Use docstrings

And then you might think that the way to call it is not trivial and want to document it: let's use a docstring !

def get_latest_file(path, *paths):
    """Returns the name of the latest (most recent) file
    of the joined path(s)"""
    fullpath = os.path.join(path, *paths)

Miscellaneous

If you use Python 3, you can use iglob instead.

For the os.path.split, I prefer using it like this (instead of the 1 index):

folder, filename = os.path.split(latest_file)

The import datetime is not used.

Instead of if len(list_of_files)> 0:, you can simply do if list_of_files:

Revised code

def get_latest_file(path, *paths):
    """Returns the name of the latest (most recent) file
    of the joined path(s)"""
    fullpath = os.path.join(path, *paths)
    list_of_files = glob.glob(fullpath)  # You may use iglob in Python3
    if not list_of_files:                # I prefer using the negation
        return None                      # because it behaves like a shortcut
    latest_file = max(list_of_files, key=os.path.getctime)
    _, filename = os.path.split(latest_file)
    return filename

Consider you has the directories in a particular path, then we need the simple code like as shown in below.

import os
files = os.listdir(path)
latest_file = files[0]
for key in files:
    if os.path.getctime(path+key) > os.path.getctime(path + latest_file):
        latest = key
print(latest)


Creating a 2D array of map tiles I am a C++ programmer new to Ruby and someone suggested that I post my code here since it doesn't quite match Ruby standards. Is there a more Ruby way of doing this?

I have created a 2D array full of MapTile class objects and then I draw the tiles to the screen by iterating through the 2D Array using a for loop. Here is some code.

class MapTile
attr_accessor :tileSprite, :attribute

    def initialize(sprite, attr)
    @tileSprite = sprite
    @attribute = attr
    end

    def tileSprite
        @tileSprite
    end

    def attribute
        @attribute
    end

end

def array2D(width,height)
  a = Array.new(width, MapTile.new(123,0))
  a.map! { Array.new(height, MapTile.new(123,0)) }
  return a
end

@mapData = array2D(@mapSize,@mapSize)

for i in 0..@mapSize - 1
   for j in 0..@mapSize - 1
      mapData[i][j].tileSprite = tileNum
      @tile.draw(mapData[i][j].tileSprite)
   end
end

First of all your indentation is a bit wacky. Sometimes you indent by 4 spaces, sometimes by 2 (which is the default in the ruby community) and sometimes not at all (for example you didn't indent the call to attr_accessor or the contents of the initialize method). You should fix that.

attr_accessor :tileSprite, :attribute
# ...
def tileSprite
    @tileSprite
end

def attribute
    @attribute
end

No need to call attr_accessor and define the getter methods yourself. Do one or the other.

def array2D(width,height)
  a = Array.new(width, MapTile.new(123,0))
  a.map! { Array.new(height, MapTile.new(123,0)) }
  return a
end

Creating an array containing a default value and then mapping over that array to replace the default value makes very little sense. Why assign a default value at all if you throw it away right after?

Also it's a very bad idea to make the inner arrays all contain a reference to the same map tile. This just screams for you to introduce hard to find bugs. As a guide line you should never use the 2-argument version of Array.new with a mutable value as its argument. Use the block-version instead.

Also it's customary in ruby to not use explicit return statements in the last line of a method.

Here's how I'd write the array2d method:

def array2D(width,height)
  Array.new(width) do
    Array.new(height) do
      MapTile.new(123,0)
    end
  end
end

you don't need to write code for 2D array in ruby language.
Just below declaration represent 2D array in ruby.

[[1,2], [3,4]]

OR
Array of array

Array.new(3){ [] }


Random Message Generator using Arrays using System;

namespace Friendly_Bot
{
    internal class Program
    {
        private static string[] Greetings = { "Hello, you have a", "Hey, you have a", "Hi, you have a" };
        private static string[] Compliments = { "cool", "fashionable", "stylish" };
        private static string[] Garments = { "coat.", "dress.", "shirt.", "skirt.", "suit.", "swimsuit." };
        private static string[] Farewells = { "ADIOS!", "BYE!", "BYE-BYE!", "FAREWELL!", "GOODBYE!" };

        private static string Greeting;
        private static string Compliment;
        private static string Garment;
        private static string Farewell;
        private static string Message;

        private static void Main(string[] args)
        {
            GenerateMessage();
        }

        private static void GenerateMessage()
        {
            int i = new Random().Next(1, 11);

            //20% chance of including a farewell in the message.
            if (i < 3)
            {
                Greeting = Greetings[new Random().Next(0, Greetings.Length)];
                Compliment = Compliments[new Random().Next(0, Compliments.Length)];
                Garment = Garments[new Random().Next(0, Garments.Length)];
                Farewell = Farewells[new Random().Next(0, Farewells.Length)];
                Message = Greeting + " " + Compliment + " " + Garment + " " + Farewell;
                Console.WriteLine(Message);
                Console.ReadLine();
            }
            //80% chance of NOT including a farewell in the message.
            else
            {
                Greeting = Greetings[new Random().Next(0, Greetings.Length)];
                Compliment = Compliments[new Random().Next(0, Compliments.Length)];
                Garment = Garments[new Random().Next(0, Garments.Length)];
                Message = Greeting + " " + Compliment + " " + Garment;
                Console.WriteLine(Message);
                Console.ReadLine();
            }
        }
    }
}

This code will piece together a random message with the contents in 4 separate arrays: Greetings, Compliments, Garments, Farewells.

There is a 20% chance of the randomly generated message including a farewell at the end of the message. There is an 80% chance for the message not to include a farewell exclamation in the message.

How can I clean this code up and make it produce the same product, but more efficiently? Any help is greatly appreciated! :)

The difference in the two branches of that if statement is just the Farwell so only this generation is what should happen inisde the if.
the name of the variable i is not choosen very well if you later on need a comment to describe what it should represent.
The GenerateMessage() method is doing more than its name implies. It is generating and outputting the message. This is also violating the single responsibility principle which means that each method should have only one responsibility. So let the method return a string which then can be used in the Main().
you shouldn't create each time a new Random but reuse an existing.
if the range out of which the random numbers should be generated starts at 0 you should use the overloaded Next() method which only takes the exlusive upper bound as a parameter.
by using a List<string> the multiple string variables won't be needed and you can take advantage of the string.Join() method

private static Random random = new Random();
private static string GenerateMessage()
{
    IList<string> subMessages = new List<string>(4);
    subMessages.Add(Greetings[random.Next(Greetings.Length)]);
    subMessages.Add(Compliments[random.Next(Compliments.Length)]);
    subMessages.Add(Garments[random.Next(Garments.Length)]);

    int chance = random.Next(1, 11);
    if (chance < 3)
    {
        subMessages.Add(Farewells[new Random().Next(Farewells.Length)]);
    }

    return string.Join(" ", subMessages);
}

Edit based on the comment

  Why do you have a 4 in the parentheses in this line: IList<string> subMessages = new List<string>(4); I removed the 4 to see if the code would still work and it did...

The 4 is pretty bad code style if another programmer needs to ask. Thats what a magic number is which I should have extracted to a meaningful constant like so

private static readonly int maxMessages = 4;

and then used it like so

IList<string> subMessages = new List<string>(maxMessages);

now you know what it is, but you are still missing what is for. That is shown if you take a look at the documentation for this overloaded constructor of the List<T> you will see that this is the initial capacity of the List<T> which is explained more in the Remarks section.

  The capacity of a List is the number of elements that the List can hold. As elements are added to a List, the capacity is automatically increased as required by reallocating the internal array.

And the reason why I used it is shown here

  If the size of the collection can be estimated, specifying the initial capacity eliminates the need to perform a number of resizing operations while adding elements to the List.

If anyone wants to use this in a C# form, here is the code for that and it also works with a text so you'll have to create one.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace Lavender_Voice_Tester
{
    public partial class Form1 : Form
    {
        private static string[] Greetings = { "Hello, you have a", "Hey, you have a", "Hi, you have a" };
        private static string[] Compliments = { "cool", "fashionable", "stylish" };
        private static string[] Garments = { "coat.", "dress.", "shirt.", "skirt.", "suit.", "swimsuit." };
        private static string[] Farewells = { "ADIOS!", "BYE!", "BYE-BYE!", "FAREWELL!", "GOODBYE!" };

        private static string Greeting;
        private static string Compliment;
        private static string Garment;
        private static string Farewell;
        private static string Message;
        public Form1()
        {
            InitializeComponent();
        }

        private void Form1_Load(object sender, EventArgs e)
        {
            int i = new Random().Next(1, 11);

            //20% chance of including a farewell in the message.
            if (i < 3)
            {
                Greeting = Greetings[new Random().Next(0, Greetings.Length)];
                Compliment = Compliments[new Random().Next(0, Compliments.Length)];
                Garment = Garments[new Random().Next(0, Garments.Length)];
                Farewell = Farewells[new Random().Next(0, Farewells.Length)];
                Message = Greeting + " " + Compliment + " " + Garment + " " + Farewell;
                textBox1.Text = (Message);
            }
            //80% chance of NOT including a farewell in the message.
            else
            {
                Greeting = Greetings[new Random().Next(0, Greetings.Length)];
                Compliment = Compliments[new Random().Next(0, Compliments.Length)];
                Garment = Garments[new Random().Next(0, Garments.Length)];
                Message = Greeting + " " + Compliment + " " + Garment;
                textBox1.Text = (Message);
            }
        }
    }
}


Implementation of atoi() I implemented the atoi() function! Here is my code:

int my_atoi(char* pointer)
{
    int result = 0;
    char* pointer1;
    multiplier = 1;
    char sign = 1;

    if(*pointer == '-')
    sign =- 1;

    pointer1 = pointer;

    while(*pointer != '\0')
    {
        if(*pointer >= '0' && *pointer <= '9')
            multiplier = multiplier * 10;

        pointer = pointer + 1;
    }

    pointer = pointer1;

    while(*pointer != '\0')
    {
        if(*pointer >= '0' && *pointer <= '9')
        {
            result = result + ( (*pointer%48)  * multiplier);
            multiplier = multiplier / 10;
        }

        pointer = pointer+1;
    }

    return (result * sign) / 10;
}

I wonder if there is any way that I can improve my function. I know there is a problem with my function. What if the user wants to convert from char* to int this string: "232-19". What should I do then? Any advice would be really helpful!

Things you could improve

Variables/Initialization

Where do you declare multiplier?  I assume that since it is not declared within the method, it is declared as a global variable.  Try to avoid global variables.

The problem with global variables is that since every function has access to these, it becomes increasingly hard to figure out which functions actually read and write these variables.

To understand how the application works, you pretty much have to take into account every function which modifies the global state. That can be done, but as the application grows it will get harder to the point of being virtually impossible (or at least a complete waste of time).

If you don't rely on global variables, you can pass state around between different functions as needed. That way you stand a much better chance of understanding what each function does, as you don't need to take the global state into account.

So instead of using global variables, initialize the variables in main(), and pass them as arguments to functions if necessary.  In this case, I don't see the need for multiplier to be used outside of the function at all, so simply keep it declared within the function.
sign should be an int, and not a char.

Algorithm

Right now you are implementing a complicated and hard to follow method of converting a character into a number.  The easy way is to have isdigit() do the hard work for you.  This also will help you implement the DRY principle.

while(*pointer != '\0')
{
    if(*pointer >= '0' && *pointer <= '9')
        multiplier = multiplier * 10;

    pointer = pointer + 1;
}

pointer = pointer1;

while(*pointer != '\0')
{
    if(*pointer >= '0' && *pointer <= '9')
    {
        result = result + ( (*pointer%48)  * multiplier);
        multiplier = multiplier / 10;
    }

    pointer = pointer+1;
}

See how you have two loops doing almost identical things?  Here is how I simplified all of that by using isdigit().

while (isdigit(*c))
{
    value *= 10;
    value += (int) (*c - '0');
    c++;
}

You loop through the characters in the string as long as they are digits. For each one, add to the counter you're keeping - the value to add is the integer value of the character. This is done by subtracting the ASCII value of '0' from the ascii value of the digit in question.
Note that this code doesn't handle overflow. If you pass in "89384798719061231" (which won't fit in an int), the result is undefined.  The fix is simple enough, just use a long long int to mitigate against that.  We'll still have issues for extremely long numbers, but fixing that so that the function works as intended is a bit more complicated.

Documentation

Where did all of your comments go?  A newer developer would simply gawk at some of your code.

result = result + ( (*pointer%48)  * multiplier);

Comments can really go a long way into helping other understand your code.  Don't go overboard with them though, you will have to balance how much of them to put into your program.

Syntax/Styling

This looks like a typo.

if(*pointer == '-')
    sign =- 1;

Add a space for clarity.

if(*pointer == '-') sign = -1;

You should not be modifying your char* you accept as a parameter into the function. Therefore, declare the parameter as constant.

int my_atoi(const char* pointer)

Use more shorthand operators.

pointer++;         // same as pointer = pointer+1;
multiplier /= 10;  // same as multiplier = multiplier / 10;
multiplier *= 10;  // same as multiplier = multiplier * 10;

Final Code

#include <stdio.h>
#include <assert.h>
#include <ctype.h>

long long int my_atoi(const char *c)
{
    long long int value = 0;
    int sign = 1;
    if( *c == '+' || *c == '-' )
    {
        if( *c == '-' ) sign = -1;
        c++;
    }
    while (isdigit(*c))
    {
        value *= 10;
        value += (int) (*c-'0');
        c++;
    }
    return (value * sign);
}

int main(void)
{
    assert(5 == my_atoi("5"));
    assert(-2 == my_atoi("-2"));
    assert(-1098273980709871235 == my_atoi("-1098273980709871235"));
    puts("All good."); // I reach this statement on my system
}

For a exercise in leetcode, wrote following impl: atoi cpp code

    class Solution {
private:
    bool checkMin(int a,  int b=10, int c=0, int min_val=INT_MIN) {
        /*
        accepts a*b + c, min
        a>min; b>min; c>min
        check a*b+c > min or not
        b>0; a<0 -ive; c<0
        a!=0
        */

        min_val = min_val  -c;

        //std::cout<<"new min input:  "<<a <<" , "<< c<<"   iter: "<<b << "    "<<min_val <<std::endl;

        //compare with a now
        if(a<min_val)
            return false;
        int cur_prod = 0;
        if(a==0)
            return true;
        for(;b>1;b--) {
            cur_prod += a;
           int curr_diff = min_val-cur_prod;
            /*
            subtraction possible because
                min_val<prod,
                min_val-prod<prod-prod
                min_val-prod<0 ---1
                prod<0
                -prod>0
                min_val+(-prod )> min_val+0 [x+ (+ive quantity)>x ]
                min_val-prod>min_val --2
                from 1, 2
                min_val< min_val-prod < 0 ---3
                from 3, min_val-prod can be expressed in integer

            check if curr_diff still can hold a deduction of a
            which means: curr_diff<a should hold, for a further a deduction in prod
                         -5, -6
                            for ex of min_val = 59, a = -6 at b = 2 (9th iteration) prod = -54
                            you can't add -6 now, since it will cross definable limit
            only b-1 iterations
            because at i-1 th iteration, ith product formation is checked
            */
            //std::cout<<"check function for input:  "<<a <<" , "<< c<<"   iter: "<<b << " prod now = "
                    //<< cur_prod << "   diff = " <<curr_diff<<"  is curr_dif<a "<<(curr_diff<a)<<std::endl;
            if(curr_diff>a) {
                //std::cout<<" not possible"<<std::endl;
                return false;
            }

        }
        return true;
    }

    bool checkMax(int a,  int b=10, int c=0, int max_val=INT_MAX) {
        /*
        accepts a*b + c, min
        a<max; b<max; c<max
        check a*b+c < max or not
        b>0; a>0, c>0
        */

        max_val = max_val  -c;

        //std::cout<<"new max input:  "<<a <<" , "<< c<<"   iter: "<<b << "    "<<max_val <<std::endl;

        //compare with a now
        if(a>max_val) return false;
        int cur_prod = 0;
        if(a==0) return true;
        for(;b>1;b--) {
            cur_prod += a;
           int curr_diff = max_val-cur_prod;
            /*
            subtraction possible because
                max_val>prod,
                max_val-prod>prod-prod
                max_val-prod>0 ---1
                prod>0
                -prod<0
                max_val+(-prod )< max_val+0 [x+ (-ive quantity)<x ]
                max_val-prod<max_val --2
                from 1, 2
                0< max_val-prod < max_val ---3
                from 3, max_val-prod can be expressed in integer

            check if curr_diff still can hold a increment of a
            which means: curr_diff>a should hold, for a further a deduction in prod
                         5>6 fails
                            for ex of max_val = 59, a = 6 at b = 2 (9th iteration) prod = 54
                            you can't add 6 now, since it will cross definable limit
            only b-1 iterations
            because at i-1 th iteration, ith product formation is checked
            */
            //std::cout<<"check function for input:  "<<a <<" , "<< c<<"   iter: "<<b << " prod now = "
            //        << cur_prod << "   diff = " <<curr_diff<<"  is curr_dif<a "<<(curr_diff>a)<<std::endl;
            if(curr_diff<a) {
                //std::cout<<" not possible"<<std::endl;
                return false;
            }

        }
        return true;
    }

public:
    int myAtoi(string str) {
      //code to trim string
      int i =0, end=str.length()-1;
      //std::cout<<i<<"    "<<end<<std::endl;
      while(i<end && str.at(i)==' ') {i++;continue;}
      while(end>-1 && str.at(end)==' ') {end--;continue;}

      if(end<i) return 0;

      int sign=1;
      if(str.at(i)=='-') {sign = -1; i++;}
      else if(str.at(i)=='+') {i++;}

      string tr_str = str.substr(i, end-i+1);

      int num = 0;

       for(char& digit : tr_str) {
        if(digit<'0' || digit>'9') return num; // not convertable character - exit
        int c= digit-'0';
        if(sign==-1) {
            //std::cout<<"Evaluating "<<c<<std::endl;
            //number cannot be lower than INT_MIN
            // do a check of num * 10 - c
            //num<0 already

            if(checkMin(num, 10, -c, INT_MIN))
                num = num*10 -c;
            else {
                num = INT_MIN;
                break;
            }
            //std::cout<<"number is"<<num<<std::endl;
        }
        else {
            if(checkMax(num, 10, c, INT_MAX))
                num = num*10 +c;
            else {
                num = INT_MAX;
                break;
            }
            //std::cout<<"number is"<<num<<std::endl;
        }
       }
       return num;

    }
};


Does this work cross browser? Provided Object.prototype hasn't been modified, will the following snippet work in major browsers?

var obj = {length:0};
var push = Array.prototype.push;
push.call(obj,'1st value')
push.call(obj,'2nd value');

Many have responded that the above code is bad practice, but what makes code like this any better?

Array.prototype.slice(arguments,0)

Both are calling Array.prototypes on non Arrays.

I'm going to say no, because this is an array. It works, but you are hammering a screw at this point. Simply use an array.

It will work without crashing. The exact result will be the object with properties "0", "1" and the "length"==2. There is the scope replacement becasue of the "call" function using. However it's not very good practice to use this solution becasue of bad readability and maintainability. I suggest to do it excplicitly.


Simple file recurser I'm using C# for a project and thought I would get a feel for the language by making some simple programs.  My current program is a simple file recurser that prints the names of files and directories.  Any suggestions would be greatly appreciated.

using System;
using System.IO;

namespace FileRecurser
{
    class Program
    {
        private static int indent = -2; // Start off at -2 so first indent
                                        // starts at 0.

        static void Main(string[] args)
        {
            if (args.Length != 1)
            {
                Console.WriteLine("Usage: recurser <directory>");
                return;
            }

            // Check if directory exists.
            string directory = args[0];
            if (!Directory.Exists(directory))
            {
                Console.WriteLine("Error: {0} doesn't exist!", directory);
                return;
            }

            Console.WriteLine("Printing files for {0}\n", directory);
            PrintDirectoryFiles(directory);
        }

        private static void PrintDirectoryFiles(string directory)
        {
            indent += 2;

            // If any subdirectories.
            string[] subdirectories = Directory.GetDirectories(directory);

            if (subdirectories.Length != 0)
            {
                foreach (var subdirectory in subdirectories)
                {
                    // Change color to reflect directory.
                    Console.ForegroundColor = ConsoleColor.Cyan;

                    Console.WriteLine(
                        "{0}{1}:",
                        new String(' ', indent),
                        Path.GetFileName(subdirectory)
                    );

                    Console.ResetColor();

                    PrintDirectoryFiles(subdirectory);
                    Console.WriteLine();
                }
            }

            // Print file names.
            string[] files = Directory.GetFiles(directory);

            if (files.Length != 0)
            {
                foreach (var file in files)
                {
                    Console.WriteLine(
                        "{0}{1}",
                        new String(' ', indent),
                        Path.GetFileName(file)
                    );
                }
            }

            indent -= 2;
        }
    }
}

I don't like how you use static field to indicate level of indentation, I think it would be better if you passed it down the recursion in parameters of the methods.

And while you're at it, you should probably separate your concerns: don't have everything in your Program class, but have one class that recurses directories, and one that writes them out. That way, you can reuse them if you want to write the output in a different way (say in a GUI app) or if you want to recurse through something else than files (say, registry entries).

Also, there is no need to check whether a collection is empty before iterating it, it just unnecessarily clutters the code.

And you should think about handling exceptions: there might be directories you don't have access to. And there's also a chance that a directory was deleted while you were iterating its siblings, so you should handle that too.

I would avoid using return to terminate when you are in Main, and use Environment.Exit() instead.

It allows you to specify an error code (which is usually useless, but anyway), and also it makes your code easier to move to another method.


40 million tweets from 200k users mysql> select count(username) from users where no_tweets  > 50;
+-----------------+
| count(username) |
+-----------------+
|          282366 |
+-----------------+
1 row in set (5.41 sec)

mysql> select sum(no_tweets) from users where no_tweets > 50;
+----------------+
| sum(no_tweets) |
+----------------+
|       38569853 |
+----------------+
1 row in set (1.75 sec)

I have that many users, who have collectively tweeted that many tweets. My aim is to store them in a file and then find out what a user generally tweets about (for starters, my aim is to run vanilla LDA and see if it works well on short documents), but the problem is that I ran the python code like an hour back and it has still not finished even 2% of users. I have posted the python code below.

'''
The plan is : Find out all users  from the users database who have more than 50 tweets
Create a file for them in the directory passed as an argument with the same name as the username and write all the tweets in them
'''
def fifty(cursor):
    ''' Find all users and return , all of them having more than 50 tweets'''
    cursor.execute("select username from users where no_tweets>50")
    return cursor.fetchall()

def connect():
    ''' Cursor for mySQLdb'''
    conn = MySQLdb.connect(host="localhost",user="rohit",passwd="passwd",db="Twitter")
    return conn.cursor()

def tweets(cursor,name):
    ''' All tweets by a given user'''
    cursor.execute("select tweets from tweets where username='"+name+"'")
    return cursor.fetchall()
import sys,os,MySQLdb
directory = sys.argv[1] #Directory to write the user files
cursor = connect()
rows = fifty(cursor) #Find all users who have more than 50 tweets

for i in rows:#For all users
    data = open(os.path.join(directory,i[0]),'w') #Open a file same as their name
    allTweets = tweets(cursor,i[0]) #Find all tweets by them
    for j in allTweets:
        data.write(j[0]+"\n") #Write them
    data.close()

The problem is that the code is running too slow and at this rate, it will take more than a day for it to finish writing all files. So some way that would make it faster would be great. So yeah, that is the question.

Yikes, you're running almost 3,000,000 individual queries.  If you could do 4 a second (and you probably cannot) that is still a day!

How about just

select username, tweets from tweets order by username

Then of course you have to do some fancy footwork to switch from one file to the next when the user changes.

It should only take a few hours to run.

You want better performance?

if you want really better performance you should try to use some of the compiled code languages (like C). Because interpreted language takes much more time.

For example, if you take a simple loop, you will notice the difference

int i = 0;
int j = 0;

for(i = 0; i < 1500000; i++){
   for(j = 0; j < 10000; j++){

   }
}

It's like 50x faster with C comparing to Python.


Displaying MySQL data using PHP I have MySQL data that I am rendering in an HTML table using PHP. This is the full code of my table and PHP. It does output fine and looks how I would expect but I am wondering if there is a better way to write this code. Also, the 'Delete' button specifically, is there a better way to output the button itself as right now, I noticed especially on a mobile device, I will see a text cursor indicator when tapping the button as if I am trying to select the text on the button itself.

                    Description

                    Date Created

                    Created By

                    Edit - Delete

        <?php

        $noLists = "
You have no lists. Why dont you create one?
";

        if ($db->connect_error) {
            die("Connection failed: " . $db->connect_error);
        }

        $sql = "SELECT DISTINCT lists.id, lists.listdescription, lists.createdon, lists.createdby, lists.sharedlist FROM lists INNER JOIN users ON users.fname = '$displayname' AND users.fname = lists.createdby";
        $result = $db->query($sql);

        if ($result->num_rows > 0) {
            // output data of each row
            while($row = $result->fetch_assoc()) {

                $listID = $row["id"];
                $listDescription = $row["listdescription"];
                $createdOn = $row["createdon"];
                $createdBy = $row["createdby"];

                echo "<form>";
                echo "
";
                echo "$listDescription	";
                echo "$createdOn	";
                echo "$createdBy	";
                echo "";
                echo "<form action='account.php' method='post'>";
                echo "<input class=\"btn btn-success\" type=\"submit\" name=\"\" value=\"Edit\">";

                echo "<input class=\"btn btn-danger\" value=Delete style=\"width: 85px;\">

                	";

            }
        } else {
            echo "
";
            echo "
";

            echo "
";
            echo "
";
            echo "
 $noLists
";
        }
        $db->close();

        ?>

Generally speaking, you should keep your markup and your PHP logic separate.

And the intermediate variables don't really have a purpose, just use the values from the array.

You've got two opening form tags and no closing form tags.

Your first condition (i there are records in the table) does not close the thead or the create a tbody.

Your delete button is no button at all, it's an input wrapped in an anchor tag (). That's why it's not working as expected. Either remove the anchor and give the input a type attribute of "button", or remove the input and put some text in there instead.

Use a prepared statement. It's not cool to throw variables into your queries all willy-nilly. You will anger the code gods.

Here's a quick re-write (assumes you're using PDO - If you're using MySQLi, see the docs to convert the prepared statement to MySQLi)

I'm just going to go ahead and post this since i spend three or four minutes on it, but after looking at this code, there is no way this was working as you had it. This site is for reviewing working code, not fixing broken code. If your code doesn't work you need to ask on StackOverflow in the future.

<?php

    // Do all the PHP stuff before you start ouputting
    $noLists = "
You have no lists. Why dont you create one?
";

    if ($db->connect_error) {
        die("Connection failed: " . $db->connect_error);
    }

    $sql = "SELECT DISTINCT lists.id, lists.listdescription, lists.createdon, lists.createdby, lists.sharedlist FROM lists INNER JOIN users ON users.fname = ? AND users.fname = lists.createdby";
    $result = $db->prepare($sql);
    $result->execute(array($displayname));
    $output = array();
    if ($result->rowCount() > 0) {
        // output data of each row

        $output[] = "
";
        $output[] = "
";

        while($row = $result->fetch(PDO::FETCH_ASSOC)) {

            $output[] = "
";
            $output[] = "{$row["listdescription"]}	";
            $output[] = "{$row["createdon"]}	";
            $output[] = "{$createdBy}	";
            $output[] = "";
            $output[] = "<form action='account.php' method='post'>";
            $output[] = "<input class=\"btn btn-success\" type=\"submit\" name=\"\" value=\"Edit\">";
            $output[] = "</form>";
            $output[] = "Delete	";
        }

        $output[] = "
";
        $output[] = "
";

    } else {
        $output[] = "
";
        $output[] = "
";
        $output[] = "
";
        $output[] = "
";
        $output[] = "
 $noLists
";
    }
    $db->close();

    // Turn the output array back into a string
    $output = implode('', $output);

?>

                    Description

                    Date Created

                    Created By

                    Edit - Delete

        <?php echo $output; ?>

Edit... you're not closing your table row in the loop either. I'm not gonna bother updating the code snippet.


How to make Print() method memory & CPU efficient? Problem Statement
You are receiving n objects in a random order, and you need to print them to stdout correctly ordered by sequence number.
The sequence numbers start from 0 (zero) and you have to wait until you get a complete, unbroken sequence batch of j objects before you output them.
You have to process all objects without loss. The program should exit once it completes outputting the first 50000 objects Batch size j = 100
The object is defined as such:
    {
    "id" : "object_id", // object ID (string)
    "seq" : 0, // object sequence number (int64, 0-49999)
    "data" : "" // []bytes
    }

Example Output Statement
    Step                Input Value                Output State j = 1                  Output state j = 3
    0                       6
    1                       0                           0
    2                       4                           0
    3                       2                           0
    4                       1                           0,1,2                               0,1,2
    5                       3                           0,1,2,3,4                           0,1,2
    6                       9                           0,1,2,3,4                           0,1,2
    7                       5                           0,1,2,3,4,5,6                       0,1,2,3,4,5

Solution
func (receiver *Receiver) Print(seqNumber uint64, batchSize uint64, outputFile io.Writer) (error, bool) {

    fmt.Fprintf(outputFile, "[ ")
    if seqNumber >= receiver.outputSequence.length {
        receiver.outputSequence.bufferSizeIncrease(seqNumber)
    }
    receiver.outputSequence.sequence[seqNumber] = true

    printedCount := uint64(0) // check for MAX_OBJECTS_TO_PRINT
    var nthBatchStartingIndex uint64
    MaxObjectsToPrint := config.GetMaxPrintSize()
Loop:
    for nthBatchStartingIndex < receiver.outputSequence.length { // check unbroken sequence
        var assessIndex = nthBatchStartingIndex
        for j := assessIndex; j < nthBatchStartingIndex+batchSize; j++ { // Assess nth batch
            if j >= receiver.outputSequence.length { //index out of range - edge case
                break Loop
            }
            if receiver.outputSequence.sequence[j] == false {
                break Loop
            }
        }

        count, printThresholdReached := receiver.printAssessedBatchIndexes(assessIndex, printedCount, batchSize, MaxObjectsToPrint, outputFile)
        if printThresholdReached { // print sequence threshold reached MAX_OBJECTS_TO_PRINT
            fmt.Fprintf(outputFile, " ]  ")
            fmt.Fprintf(outputFile, " ----for input value %d\n", seqNumber)
            return nil, false
        }
        printedCount += count
        if printedCount >= MaxObjectsToPrint { // print sequence threshold reached MAX_OBJECTS_TO_PRINT
            fmt.Fprintf(outputFile, " ]  ")
            fmt.Fprintf(outputFile, " ----for input value %d\n", seqNumber)
            receiver.Log.Printf("****MaxObjectsToPrint threshold(%d) reached \n", MaxObjectsToPrint)
            return nil, false
        }
        nthBatchStartingIndex = assessIndex + batchSize // next batch
    }
    fmt.Fprintf(outputFile, " ]  ")
    fmt.Fprintf(outputFile, " ----for input value %d\n", seqNumber)
    return nil, true
}

Here is the complete solution, written for this problem.

Print() is the method that does heavy lifting in this code, with varying size of memory & heavy CPU usage:

How to make receiver.outputSequence memory effective by using datastructure other than array? because newBufferSize := 2 * seqNumber is doubling memory...

How to make Print method have effective CPU usage? On some goroutine

First, observe that if you receive an out-of-sequence object, you have to potentially keep all the objects from the latest printed sequence number to the received number. Worst case, you receive the object in reverse order and have to keep them all in memory. There is no way around this. So, what you can optimize is first, when you are printing objects you can find them quickly, and second, you don't want to move them around, which will happen if you use a slice and grow it as necessary.
So, a sketch of an algorithm that will use CPU and memory better than what you have is as follows:
var nextInSequence=0 // The next item you are expecting in the sequence
var storedObjects=map[int]SomeStruct{}

func Print(seqNumber int, obj SomeStruct) {
   if seqNumber==nextInSequence {
     output obj
     nextInSequence++
   } else {
      storedObjects[seqNumber]=obj
   }
   for {
     if stored, ok:=storedObjects[nextInSequence] ; ok {
        output stored
        delete(storedObjects,nextInSequence)
        nextInSequence ++
     } else {
        break
     }
  }
}

Using Map, below is the code:
func (receiver *Receiver) Print(nextSequenceIn uint64, object *data.Object, outputFile io.Writer) bool {
    receiver.receivedObjects[nextSequenceIn] = object
    if receiver.nextSeqNumExpected == nextSequenceIn { // set the next sequence expected
        key := nextSequenceIn + 1
        delete(receiver.receivedObjects, nextSequenceIn)
        for {
            if _, ok := receiver.receivedObjects[key]; !ok {
                receiver.nextSeqNumExpected = key
                break
            }
            key++
        }
    }
    fmt.Fprintf(outputFile, "[ ")
    continu := receiver.printBatch(outputFile)
    fmt.Fprintf(outputFile, "]")
    receiver.printedSequences = 0
    fmt.Fprintf(outputFile, "   ----------for input value %d\n", nextSequenceIn)
    return continu
}

func (receiver *Receiver) printBatch(outputFile io.Writer) bool {
    sequenceNumber := uint64(0)
    batchSize := config.GetBatchSize()
    for sequenceNumber+(batchSize-1) < receiver.nextSeqNumExpected { // received unbroken sequences are [0, receiver.nextSeqNumExpected-1]
        if receiver.printedSequences+batchSize > receiver.maxObjectsToPrint {
            receiver.Log.Printf("****Max objects(%d) to print is reached\n", receiver.maxObjectsToPrint)
            return false
        }
        for j := sequenceNumber; j < sequenceNumber+batchSize; j++ {
            fmt.Fprintf(outputFile, "%d, ", j)
            receiver.printedSequences++

        }
        sequenceNumber += batchSize
    }
    return true
}

Ran below commands:
$ export BATCH_SIZE=3
$ export MAX_OBJECTS_TO_PRINT=25
$
$ ~/code/bin/threadapp
*****Main exit
$

here is the output


Is the string a pangram? The following code checks if a string is a pangram or not:

public static class StringExtensions
{
    public static IEnumerable<char> EnglishAlphabet
    {
        get
        {
            for (char i = 'a'; i <= 'z'; i++)
            {
                yield return i;
            }

            yield break;
        }
    }

    /// <summary>
    /// Check if the source string is a pangram.
    /// </summary>
    /// <remarks>
    /// The default dictionary is the english one.
    /// </remarks>
    public static bool IsPangram(this string source)
    {
        return source.IsPangram(EnglishAlphabet);
    }

    /// <summary>
    /// Check if the source string is a pangram.
    /// </summary>
    /// <remarks>
    /// A pangram is a string that uses every letter in the alphabet.
    /// </remarks>
    public static bool IsPangram(this string source, IEnumerable<char> alphabet)
    {
        var lowerAlphabet = new string(alphabet.ToArray()).ToLower();
        IDictionary<char, bool> alphabetCharacters = lowerAlphabet.ToDictionary(c => c, c => false);
        var lowerSource = source.ToLower();

        foreach(char character in lowerSource)
        {
            alphabetCharacters[character] = true;
        }

        return !alphabetCharacters.Values.Contains(false);
    }
}

Do you see anything wrong/weird in it? Any feedback (style, naming, performance) is more than welcome.

Use a set instead of a dictionary

The current algorithm sets the value to true for each character it finds,
and at the end it checks if there is anything still unset. This is a bit tedious, and inefficient. Checking if any value is false is an \$O(n)\$ operation.

A simpler and more natural solution would be using a set data structure (HashSet in C#). You could add characters that you found, and in the end check the size of the set, and compare it with the size of the alphabet.

If the input may contain letters not in the alphabet, then before adding letters to the found set, don't forget to check the letter is actually part of the alphabet. (Thanks @tym32167 for the reminder.)

Use a simple array instead of a dictionary

If the alphabet is always the English alphabet, then an interesting simple alternative is using a simple boolean array of size 'z' - 'a' + 1. As you iterate over the letters, you can derive the array index to use by letter - 'a'. It will be simpler and more storage efficient than using a dictionary.

Generate the alphabet once

The English alphabet is regenerated on each call of IsPangram.
This is unnecessary, as it never changes between calls.
You could generate it once and reuse.

Without a line by line review
Just an implementation of what Janos answered
Give the check to Janos

    private HashSet<int> panaHash = new HashSet<int>();
    int cI;
    public bool IsPangram(string source)
    {
        panaHash.Clear();
        foreach (char c in source.ToLower())
        {
            cI = (int)c;
            if (cI < 97)   // not going to change in your life time
                continue;
            if (cI > 122)  // not going to change in your life time
                continue;
            panaHash.Add(cI);
            if (panaHash.Count == 26)
                return true;
        }
        return false;
    }

ran some time test and this was the fastest

me 1924
RegX 27436
Lippert 4587
Question 2357


Speech Synthesis to showcase how various voices sound with System.Speech.Synthesis I was wondering if you would be willing to give me some suggestions on shortening this code. I feel as if the amount of if statements I have is a bit much.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Speech.AudioFormat;
using System.Speech.Synthesis;
using System.Text;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Data;
using System.Windows.Documents;
using System.Windows.Input;
using System.Windows.Media;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;

namespace SpeechTutorial
{
    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow : Window
    {
        SpeechSynthesizer synth = new SpeechSynthesizer();

        public MainWindow()
        {
            InitializeComponent();
            comboItems();
        }

        private void comboItems()
        {
                foreach (InstalledVoice voice in synth.GetInstalledVoices())
                {
                    // loop through each voice installed on machine
                    VoiceInfo info = voice.VoiceInfo;
                    // adds each installed voice's Name and location to the combobox's list
                    comboBox.Items.Add(info.Description);
                }
            }

        private void button_Click(object sender, RoutedEventArgs e)
        {
            // set the synthesizer to speak
            synth.Speak(textBox.Text);
        }

        private void comboBox_SelectionChanged(object sender, SelectionChangedEventArgs e)
        {
            // here we are setting the voices for each selected item in the combobox

            try
            {
                if (comboBox.SelectedIndex == 0)
                {
                    synth.SelectVoice("Microsoft Hazel Desktop");
                }

                if (comboBox.SelectedIndex == 1)
                {
                    synth.SelectVoice("Microsoft Heera Desktop");
                }

                if (comboBox.SelectedIndex == 2)
                {
                    synth.SelectVoice("Microsoft David Desktop");
                }

                if (comboBox.SelectedIndex == 3)
                {
                    synth.SelectVoice("Microsoft Zira Desktop");
                }

                if (comboBox.SelectedIndex == 4)
                {
                    synth.SelectVoice("Microsoft Haruka Desktop");
                }

                if (comboBox.SelectedIndex == 5)
                {
                    synth.SelectVoice("Microsoft Heami Desktop");
                }

                if (comboBox.SelectedIndex == 6)
                {
                    synth.SelectVoice("Microsoft Huihui Desktop");
                }

                if (comboBox.SelectedIndex == 7)
                {
                    synth.SelectVoice("Microsoft Tracy Desktop");
                }

                if (comboBox.SelectedIndex == 8)
                {
                    synth.SelectVoice("Microsoft Hanhan Desktop");
                }
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
            }

        }
    }
}

Take a look at arrays. It will allow you to quickly add and remove future options without needing to change the logic of the code. It will also allow you to possibly add more complex logic in a single place, instead of having to add code to every if/switch statements.

string[] options = { "Microsoft Hazel Desktop",
                     "Microsoft Heera Desktop",
                     "Microsoft David Desktop",
                     "Microsoft Zira Desktop",
                     "Microsoft Haruka Desktop",
                     "Microsoft Heami Desktop",
                     "Microsoft Huihui Desktop",
                     "Microsoft Tracy Desktop",
                     "Microsoft Hanhan Desktop"};

try
{
    synth.SelectVoice(options[comboBox.SelectedIndex]);
}
catch (Exception ex)
{
    MessageBox.Show(ex.Message);
}


Outputting the names of cars, without repetitions, with the number of occurrences in order of decreasing repetitions A short while ago, I have submitted a coding exercise to a
potential employer. The response came back the next
morning and you can guess what it was from the subject
of this post.

I am not totally at loss, but I need another programmer's
perspective. Is there anything that jumps out?

The idea of the exercise is simple: I'm given an input file with
names of cars, one per line,  possibly repeated and in no
particular order.

The program should output the same names, except with no
repetitions, the number of occurrences listed next to each
car, and in order of decreasing repetitions.

Example:

Honda\n Audi\n Honda\n -> Honda 2 \n Audi 1\n

#include <iostream>
#include <fstream>
#include <string>
#include <map>
#include <algorithm>
#include <cctype>

using namespace std;

// helper functions ///////////////////////////////////////

// reads lines from instream
void collect_lines(istream &in, map<string, int> &lines);

// given lines->num_occurs map, reverses mapping
void reorg_by_count(map<string, int> &lines,
                    multimap<int, string> &bycount);
///////////////////////////////////////////////////////////

int main(int ac, char* av[])
{
    istream *in;
    map<string, int> *lines = new map<string, int>();
    multimap<int, string> *lines_by_count = new multimap<int, string>();

    if (ac < 2)
    {
        in = &cin;
    }
    else
    {
        in = new ifstream(av[1]);
    }

    if (!in->good()) return 1;

    collect_lines(*in, *lines);
    reorg_by_count(*lines, *lines_by_count);

    if (in != &cin)
          {
        ((ifstream *)in)->close();
        delete in;
    }

    cout << "=====================\n\n";

    multimap<int, string>::reverse_iterator it
        = lines_by_count->rbegin();

    for (; it != lines_by_count->rend(); it++)
    {
        cout << it->second << " " << it->first << '\n';
    }

    delete lines;
    delete lines_by_count;

    return 0;
}

// Read the instream line by line, until EOF.
// Trim initial space. Empty lines skipped
void collect_lines(istream &in, map<string, int> &lines)
{
    string tmp;

    while (in.good())
    {
        getline(in, tmp);

        int i = 0;

        // trim initial space (also skips empty strings)
        for (i = 0; i < tmp.length() && !isalnum(tmp[i]); i++);
        if (i >= tmp.length()) continue;
        tmp = tmp.substr(i);

        for (i = 0; i < tmp.length(); i++)
        {
            if (!isalnum(tmp[i]))
            {
                tmp[i] = ' ';
            }

            // thus, HoNdA == Honda
            if (i == 0)
            {
                tmp[i] = toupper(tmp[i]);
            }
            else
            {
                tmp[i] = tolower(tmp[i]);
            }
        }

        // and record       the counts
        if (lines.count(tmp) == 0)
        {
            lines[tmp] = 0;
        }

        lines[tmp]++;
    }
}

// given lines->num_occurs map, reverses mapping
void reorg_by_count(map<string, int> &lines,
                                                                                multimap<int, string> &bycount)
{
    map<string, int>::iterator it = lines.begin();

    for (; it != lines.end(); it++)
    {
      bycount.insert(pair<int, string>(it->second, it->first));
    }
}

Problems I see:
My problem with your code is that you are newing a lot of stuff that should just be objects.
map<string, int> *lines = new map<string, int>();
multimap<int, string> *lines_by_count = new multimap<int, string>();

Both of these should just be plain objects.
map<string, int>        lines;
multimap<int, string>   lines_by_count;

This one fact would have caused you to be rejected. I would have seen this and I would not have read any-further into your code straight onto the reject pile. This fundamental flaw in your style shows that you are not a C++ programmer.
Next the objects you new are stored in RAW pointers. This is a dead give away that you are not an experienced C++ programmer. There should practically never be any pointers in your code. (All pointers should be managed by an object). Even though you manually do delete these two it is not done in an exception safe way (so they can still potentially leak).
You are reading a file incorrectly.
while (in.good())
{
    getline(in, tmp);

This is the standard anti-pattern for reading a file (even in C). The problem with your version is that the last successful read will read upto but not past the EOF. Thus the state of the file is still good but there is now no content left. So you re-enter the loop and the first read operation getline() will then fail. Even though it can fail you do not test for that.
I would expect to see this:
while (getline(in, tmp))
{
    // Line read successfully
    // Now I can processes it
}

Next you are showing a fundamental misunderstanding of how maps work:
    if (lines.count(tmp) == 0)
    {
        lines[tmp] = 0;
    }
    lines[tmp]++;

If you use the operator[] on a map it always returns a reference to an internal value. This means if the value does not exist one will be inserted. So there is no need to do this check. Just increment the value. If it is not their a value will be inserted and initialized for you (thus integers will be zero). Though not a big problem its usually preferable to use pre-increment. (For those that are going to say it does not matter. On integer types it does not matter. But you have to plan fro the future where somebody may change the type to a class object. This way you future proof your code against change and maintenance problems. So prefer pre-increment).
You are doing extra work you don't need to:
// trim initial space (also skips empty strings)
for (i = 0; i < tmp.length() && !isalnum(tmp[i]); i++);

The streams library already discards spaces when used correctly. Also the ';' at the end of the for. This is considered bad practice. It is really hard to spot and any maintainer is going to ask did he really mean that. When you have an empty body it is always best to use the {} and put a comment in their  {/*Deliberately empty*/}
Here you are basically lower casing the string.
    for (i = 0; i < tmp.length(); i++)
    {
        if (!isalnum(tmp[i]))
        {
            tmp[i] = ' ';
        }

You could use the C++ algorithms library to do stuff like this:
std::transform(tmp.begin(), tmp.end(), tmp.begin(), ::tolower());
                           //                       ^^^^^^^^^^^ or a custom
                           //                        functor to do the task

Const correctness.
void reorg_by_count(map<string, int> &lines, multimap<int, string> &bycount)

The parameter lines is not mutated by the function nor should it be. I would expect it to be passed as a const reference as part of the documentation of the function that you are not going to mutate it. This also helps in future maintenance as it stops people from accidentally mutating the object in a way that later code would not expect.
My final thing is I did not see any encapsulation of the concept of a car. You treated it all as lines of text. If you had invented a car object you can define how cars are read from a stream and written to a stream etc. Thus you encapsulate the concept in a single location.
I would have done something like this:
Probably still overkill.
#include <algorithm>
#include <iterator>
#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <map>
#include <cctype>

class Car
{
    public:
        bool operator<(Car const& rhs) const {return name < rhs.name;}
        friend std::istream& operator>>(std::istream& stream, Car& self)
        {
            std::string   line;
            std::getline(stream, line);

            std::stringstream linestream(line);
            linestream >> self.name;  // This strips white space

            // Lowercase the name
            std::transform(self.name.begin(), self.name.end(), self.name.begin(), ::tolower);
            // Uppercase first letter because most are proper nouns
            self.name[0] = ::toupper(self.name[0]);
            return stream;
        }
        friend std::ostream& operator<<(std::ostream& stream, Car const& self)
        {
            return stream << self.name << "\n";
        }
    private:
        std::string   name;
};

int main(int argc, char* argv[])
{
    if (argc < 2)
    {    exit(1);
    }
    std::ifstream      cars(argv[1]);
    std::map<Car,int>  count;

    Car  nextCar;
    while(cars >> nextCar)
    {
        ++count[nextCar];
    }

    // PS deliberately left the sorting by inverse order as an exercise.
    for(auto const& car: count) {
        std::cout << car.first << ":   " << car.second << "\n";
    }
}

Here's what I would answer:

def countCars(fname):
  carCount = {}
  with open(fname, 'r') as f:
    for car in f:
      car = car.strip()
      carCount[car] = 1 + carCount.get(car, 0)
  return carCount

def printCount(carCount):
  items = carCount.items()
  items.sort(lambda a,b:b[1]-a[1])
  for item in items:
    print "%s %d" % item

if __name__ == "__main__":
  import sys
  printCount(countCars(sys.argv[1]))

And then they'd say "That isn't C++" and I'd say "C++ really isn't appropriate for this kind of work, which is an order of magnitude faster to write in higher-level language and runs IO-bound anyway." and then they wouldn't hire me and I'd go work at a company that uses languages that aren't old enough to rent a car.

Go ahead, flame away, what do I care...

Later:
The thought occurs, maybe the hiring company didn't specify the language and that was the OP's mistake, choosing a Reagan-era hold-over like C++.


Simple PacketHandler class I can recently coded an emulation system and in this project I have a PacketHandler for each player connected to a socket server, and each player receives packets (data) from the client application.

Can someone please let me know if this code can be improved in any way? I will post the code below for the PlayerPacketHandler class, if you need any other classes that are used in this class let me know and I can add them.

Thanks.

How is ProcessPacketData called?

private void IncomingPacket(IAsyncResult iAr)
{
    try
    {
        var bytesReceived = _playerSocket.EndReceive(iAr);

        if (bytesReceived == 0)
        {
            Dispose();
            return;
        }

        var packet = new byte[bytesReceived];
        Array.Copy(_buffer, packet, bytesReceived);
        _playerPacketHandler.ProcessPacketData(packet);
    }
    catch (Exception)
    {
        Dispose();
    }
    finally
    {
        try
        {
            _playerSocket.BeginReceive(_buffer, 0, _buffer.Length, SocketFlags.None, IncomingPacket, _playerSocket);
        }
        catch
        {
            Dispose();
        }
    }
}

The class I am asking to be reviewed:

using System;
using System.IO;
using Sahara.Core.Logging;

namespace Sahara.Base.Game.Players
{
    internal class PlayerPacketHandler
    {
        private readonly Player _player;
        private readonly LogManager _logManager;
        private bool _decryptedData;
        private byte[] _dataReceived;
        private bool _dataCompleted;

        public PlayerPacketHandler(Player player)
        {
            _player = player;
            _logManager = Sahara.GetServer().GetLogManager();
        }

        public void ProcessPacketData(byte[] data)
        {
            try
            {
                if (_player.RC4Client != null && !_decryptedData)
                {
                    _player.RC4Client.Decrypt(ref data);
                    _decryptedData = true;
                }

                if (_dataCompleted)
                {
                    byte[] fullDataRcv = new byte[_dataReceived.Length + data.Length];
                    Buffer.BlockCopy(_dataReceived, 0, fullDataRcv, 0, _dataReceived.Length);
                    Buffer.BlockCopy(data, 0, fullDataRcv, _dataReceived.Length, data.Length);

                    ProcessPacketData(fullDataRcv);
                    _dataCompleted = false;

                    return;
                }

                using (var reader = new BinaryReader(new MemoryStream(data)))
                {
                    if (data.Length < 4)
                        return;

                    int messageLength = HabboEncoding.DecodeInt32(reader.ReadBytes(4));

                    if ((reader.BaseStream.Length - 4) < messageLength)
                    {
                        _dataReceived = data;
                        _dataCompleted = true;
                        return;
                    }

                    if (messageLength < 0 || messageLength > 5120)
                        return;

                    var packet = reader.ReadBytes(messageLength);

                    using (var binaryReader = new BinaryReader(new MemoryStream(packet)))
                    {
                        int packetHeader = HabboEncoding.DecodeInt16(binaryReader.ReadBytes(2));

                        byte[] Content = new byte[packet.Length - 2];
                        Buffer.BlockCopy(packet, 2, Content, 0, packet.Length - 2);

                        IncomingPacket clientPacket = new IncomingPacket(packetHeader, Content);
                        onNewPacket.Invoke(clientPacket);

                        _decryptedData = false;
                    }
                }
            }
            catch (Exception exception)
            {
                var method = System.Reflection.MethodBase.GetCurrentMethod().Name;
                _logManager.Log("Error in " + method + ": " + exception.Message, LogType.Error);
                _logManager.Log(exception.StackTrace, LogType.Error);
            }
        }
    }
}

My two cents : MemoryStream is a disposable object (inherited from Stream)

using (var reader = new BinaryReader(new MemoryStream(data)))

should grant you with a warning CA2000.

You may check this point with something like

Stream stream = null ;
try
{
   Stream = new MemoryStream(data) ;
   using (var reader = new BinaryReader(stream))
...
}
finally
{
    if (stream != null)
    {
        stream.Dispose() ;
    }
}

This should ensure you are freeing the stream in time.

Hope this helps !


LeetCode "Jewels and Stones": counting certain characters in a string LeetCode "Jewels and Stones"

  You're given strings J representing the types of stones that are
  jewels, and S representing the stones you have.  Each character in
  S is a type of stone you have.  You want to know how many of the stones you have are also jewels.

  The letters in J are guaranteed distinct, and all characters in J
  and S are letters. Letters are case sensitive, so "a" is considered
  a different type of stone from "A".

  Example 1:

  Input: J = "aA", S = "aAAbbbb"
  Output: 3

  Example 2:

  Input: J = "z", S = "ZZ"
  Output: 0

  Note:

  S and J will consist of letters and have length at most 50. The
  characters in J are distinct.

My approach:

class Solution {
    public int numJewelsInStones(String J, String S) {

        int count = 0;
        for( int i = 0; i < J.length(); i++ )
        {
            for( int j = 0; j < S.length(); j++ )
            {
                if( J.charAt(i) == S.charAt(j) )
                    count++;
            }
        }
        return count;
    }
}

Time complexity: O(n^2)

Space complexity: O(1)

Another approach:

class Solution {
    public int numJewelsInStones(String J, String S) {
        Set<Character> jSet = new HashSet<>();
        for(Character ch : J.toCharArray()) {
            jSet.add(ch);
        }
        int count = 0;
        for(Character ch: S.toCharArray()) {
            if(jSet.contains(ch)) {
                count++;
            }
        }
        return count;
    }
}

Time complexity: O(n)
Space complexity: O(n)

I have the following questions regarding the above code snippets:

Which approach is better according to you?
Will the interviewer be more impressed by the 2nd method as it has used Set?
How can I further improve my code (sort the string and perform a binary search)-> O(n*log(n))?

Caution — naïve application of big-O analysis will lead you astray here!

First of all, you should be more precise about what you mean by "n".  In this problem, there are |J| and |S|: the lengths of J and S.  Your first approach is O(|J| |S|); your second one is O(|J| + |S|).

More importantly, big-O analysis only tells you how well an algorithm scales to handle large inputs.  In this challenge, though, |J| and |S| are at most 50 — very small inputs, by computer standards.  That means that the constant factors, which are disregarded in big-O analysis, actually matter.  (Another way to look at it: with those limits, |J| and |S| are both O(1), so any sane solution is also O(1)!)

Consider how much code is involved in making a HashSet.  A HashSet is actually a disguise for a HashMap.  A HashMap is implemented as an array of trees, each containing Map.Entry objects.  The array should be optimally sized to hold all the characters of J with no hashcode collisions, but you neglected to specify size hints when calling the HashSet<>() constructor.  Then, you have to insert each character of J, which involves boxing a char into a Character, calculating its hashcode, making a Map.Entry for it, and inserting it into the HashMap's table.  The problem is, Java makes it easy to execute a lot of operations without making you aware of how much work it actually is!

Since the challenge states that all characters are letters — which I interpret to mean the 52 letters in the English alphabet, you could achieve an efficient test of membership in J using a simple lookup table:

public int numJewelsInStones(String j, String s) {
    boolean[] jewels = new boolean[128];
    for (int i = 0; i < j.length(); i++) {
        jewels[j.codePointAt(i)] = true;
    }
    int count = 0;
    for (int i = 0; i < s.length(); i++) {
        if (jewels[s.codePointAt(i)]) {
            count++;
        }
    }
    return count;
}

public long numJewelsInStones(String J, String S) {
    Set<Character> jewels = J.chars().mapToObj(c -> (char) c).collect(Collectors.toSet());
    return jewels.stream().filter(x -> S.contains(x + "")).count();
}

Definitely, second approach is better, java 8 can actually truncate it further.
Also jewels is a better name than jSet.


Multilingual greeter object After coming across Douglas Crockford's views on class-free OOP, and doing away with new and this in Javascript I tried defining an object with its own variable and function, and returning it in JSON, as shown below:

var newObj = function (name, lang){
    var obj = {
            "name":name,
            "greeter":greeter
    };
    return obj;
};

function greeter (lang){
        switch (lang){
                case "en":return "hi";
                case "es":return "hola";
                default: return "hello";
        }
}

//and finally, use the object
var name = "john", lang = "es";
var obj = newObj(name, lang);

console.log(obj.name);
console.log(obj.greeter(lang));

I know this code works in NodeJS, but I wonder if it is actually a good idea to do it this way. More specifically, are there any implications/consequences to returning functions within a javascript object that I should be aware of?

This is not JSON. However JSON was inspired from Javascript Native way of creating object. This is why they're really similar.

However JSON handles only data, not functions.

Furthermore you can rewrite your function like this :

function newObj(name){
    this.name = name;
    return this;
};
newObj.prototype.greeter = function(lang){
    switch (lang){
            case "en":return "hi "+this.name;
            case "es":return "hola "+this.name;
            default: return "hello "+this.name;
    }
}

var obj = new newObj("name");
obj.greeter("de");

As you can see

I got rid of the 2nd parameter in the newObj function since it was not used.
I bind greeter to the Object so now I can use this.name in greeter to greet you by your name.
If you try to JSONify this, everything under prototype will be ignored so you won't have any error.

Yes it's a normal practice. Just be wary of function scope, functions declared outside of your object might not accessible.