Dimension comparisons for calculating image thumbnail size I've been toying around with some code I wrote a while back, in an attempt to make it "cleaner" based on what I've read. Does anything stand out to you as bad conventions here? What are your thoughts about doing some conditional checks as separate variables at the top of the function? I thought of putting those into the if/elseif statements, but I thought this method would be DRYer.

/**
 * Calculate the dimensions for the thumbnails.
 *
 * @param  $width, $height, $target_width, $target_height
 * @return array
 */
private function calculateThumbnailSize($width, $height, $target_width, $target_height) {
    $widthHeightComparison = !is_negative(gmp_cmp($width, $height));
    $widthTargetWidthComparison = !is_negative(gmp_cmp($width, $target_width));
    $heightTargetHeightComparison = is_positive(gmp_cmp($height, $target_height));
    $heightGTETargetHeight = !is_negative(gmp_cmp($height, $target_height));
    $heightCalc = floor($height * ($target_width / $width));

    if ($widthHeightComparison && $widthTargetWidthComparison && ($heightCalc <= $target_height )) {
        $new_width = $target_width;
        $new_height = floor( $height * ($target_width / $width) );
    } elseif ($widthHeightComparison && $widthTargetWidthComparison && ($heightCalc > $target_height )) {
        $new_width = floor( $width * ($target_height / $height) );
        $new_height = $target_height;
    } elseif ($widthHeightComparison && !$heightTargetHeightComparison) {
        $new_width = $width;
        $new_height = $height;
    } elseif($widthHeightComparison && $heightTargetHeightComparison) {
        $new_width = floor( $width * ($target_height / $height) );
        $new_height = $target_height;
    } elseif (!$widthHeightComparison && $heightGTETargetHeight){
        $new_width = floor( $width * ($target_height / $height) );
        $new_height = $target_height;
    } elseif (!$widthTargetWidthComparison && !$heightGTETargetHeight) {
        $new_width = $width;
        $new_height = $height;
    } else {
        $new_width = $target_width;
        $new_height = floor( $height * ($target_width / $width) );
    }
    return array($new_width, $new_height);
}

/**
 * Check if expression is negative integer.
 *
 * @param $str
 * @return bool
 */
private function is_negative($str) {
  return (is_numeric($str) && $str < 0 && $str == round($str));
}

/**
 * Check if expression is positive integer.
 *
 * @param $str
 * @return bool
 */
private function is_positive($str) {
  return (is_numeric($str) && $str > 0 && $str == round($str));
}

Trial and Error programming?

My first feeling is that is poorly written code. Why? It is too complex. Calculating the dimensions of a thumbnail should be an easy task, but this code manages to make it into something that has to be studied for an hour before it can be decrypted.

You use !is_negative() but also is_positive(). Naively I would say: "But those are the same!". But, of course, they are not.

I could live with the fact you feel the need to invoke the GMP library, but really? Nobody has images that big, nor do you use it in places where it matters. You don't even use the three values of the outcome of gmp_cmp(), you only use two.

Since you haven't told us, I start by assuming that your function will always be fed by valid numbers. Normal numbers. If this assumption is wrong you can add an is_numeric() error check, or a conversion at the beginning.

Your variables names are poorly chosen. $widthHeightComparison should be something like $imageIsWiderThanHigh or $imageIsLandscape. Same is true for the other variables: $heightCalc doesn't mean anything.

All your function does is calculate a new width and height, based on four parameters: The width and height of the image and the maximum height and width of the thumbnail.

You've decided that there are seven possible ways in which this must be calculated. Let me number them 1 to 7, from top to bottom. To my surprise they are not all different: 1 = 7, 2 = 4 = 5 and 3 = 6. So really there are only three ways in which you perform your caculation. Did you know this? Clearly not.

I think this code was written, not by thinking about the problem, but by random trial and error. You simply gave it some numbers and every time the outcome wasn't what you expected, you added something to correct that, until, after a couple of hours, it passed all the tests. I guess you could say that this code is the result of Test-driven development. Clearly that is not a good way to approach this problem. I prefer it when somebody thinks about the task at hand before writing down code. Afterall, this is not a complex problem.

So here my first stab at this:

function thumbnailDimensions($imageWidth,$imageHeight,$thumbWidth,$thumbHeight)
// resize to thumbnail while maintaining aspect-ratio
{
  // we resize to the thumbnail width and change the height to keep the aspect-ratio
  $imageHeight = round($thumbWidth*$imageHeight/$imageWidth);
  $imageWidth  = $thumbWidth;
  // the image can now be higher than allowed, so we shrink if needed
  if ($imageHeight > $thumbHeight) {
    $imageWidth  = round($thumbHeight*$imageWidth/$imageHeight);
    $imageHeight = $thumbHeight;
  }
  // return array with result
  return array('width' => $imageWidth,'height' => $imageHeight);
}

So that's all that is needed, as far as I can tell. You see, this is not rocket science.

The answer to your question is: No local variables are needed, but if you do use them, give them proper names. Yes, I am in favor of using local variables if it means you don't have to compute something multiple times.


Split data from single CSV file into several CSV files by column value Okay...so I have been self teaching for about seven months, this past week someone in accounting at work said that it'd be nice if she could get reports split up....so I made that my first program because I couldn't ever come up with something useful to try to make...all that said, I got it finished last night, and it does what I was expecting it to do so far, but I'm sure there are things that have a better way of being done.

I wanted to get it finished without help first, but now I'd like someone to take a look and tell me what I could have done better, or if there is a better way to go about getting the same results....

What this is doing is: it opens a CSV file (the file I've been practicing with has 27K lines of data) and it loops through, creating a separate file for each billing number, using the billing number as the filename, and writing the header as the first line.  Each instance is overwriting the file if it is already created, this is an area I'm sure I could have done better. After that, it loops through the data again, appending each line of data into the correct file.

import os
import csv

#currentdirpath = os.getcwd()
#filename = 'argos.csv'
#file_path = os.path.join(os.getcwd(), filename) #filepath to open

def get_file_path(filename):
    ''' - This gets the full path...file and terminal need  to be in
same directory - '''
    file_path = os.path.join(os.getcwd(), filename)
    return file_path

pathOfFile = get_file_path('argos.csv')
''' - Below opens and reads the csv file,
    then going to try to loop and write the rows out in files sorted
by Billing Number - '''
with open(pathOfFile, 'rU') as csvfile:
    reader = csv.reader(csvfile)
    header = next(reader)
    for row in reader:
        new_file_name = row[5][:5] + '.csv'
    ''' Create file named by billing number, and print the header to
each file '''
        fb = open(new_file_name, 'w+')
        fb.write(str(header) + '\n')
        #fb.close()
with open(pathOfFile, 'rU') as csvfile:
    reader = csv.reader(csvfile)
    for row in reader:
        new_file_name = row[5][:5] + '.csv'
        ab = open(new_file_name, 'a')
        ab.write(str(row) + '\n')

I've left a few of the things in there that I had at one point, but commented out...just thought it might give you a better idea of what I was thinking...any advice is appreciated!

Don't use string literals as comments. PEP 8 explains how to use comments.
Docstrings should use """ rather than ''', as described in PEP 257. Also your docstring doesn't need the "-", and should probably be rephrased slightly to fit on one line.
Close files, #fb.close() shows you went out of your way to make bad code. Without fb.close or wrapping open in a with, the file is not guaranteed to be closed. I personally prefer with to fb.close, as described here.
Personally, rather than over-riding your files \$n\$ times, I'd use collections.defaultdict, to group all your files into their rows.
You may want to change get_file_path, to be based of __file__. Or leave your path to be relative, as it'll default to that behavior.

import os
import csv
from collections import defaultdict

FILE_DIR = os.path.dirname(os.path.abspath(__file__))

def get_file_path(filename):
    return os.path.join(FILE_DIR, filename)

file_path = get_file_path('argos.csv')
with open(file_path, 'rU') as csvfile:
    reader = csv.reader(csvfile)
    header = next(reader)
    data = defaultdict(lambda:[header])
    _ = data[header[5][:5]]
    for row in reader:
        data[row[5][:5]].append(row)
    for file_name, rows in data.items():
        with open(file_name, 'w+') as f:
            for row in rows:
                f.write(str(row) + '\n')


Unit tests code for a Java class that that checks whether an array is sorted I'm trying to write unit tests code for a java class. The class tests an array for sorted (either way) or not.

Here is the Unit Tests code:

public class SortedOrNotTest
{

   @Test
   public void test()
   {
      int[] A = { 32, 32, 32, 32, 32, 32, 32, 32, 34 };
      assertEquals(true, SortedOrNot.isSorted(A));

      int[] B = { 32, 32, 34, 33 };
      assertEquals(false, SortedOrNot.isSorted(B));

      int[] C = { 32 };
      assertEquals(true, SortedOrNot.isSorted(C));

      int[] D = {};
      assertEquals(true, SortedOrNot.isSorted(D));

      int[] E = { 32, 32, 31, 30, 32 };
      assertEquals(false, SortedOrNot.isSorted(E));
   }
}

Here is the Class, it checks an array is sorted or not in either way:

public class SortedOrNot
{

   public static boolean isSorted(int[] arr)
   {
      int n = arr.length;

      if(n<2) return true;

      int i = 0;
      while (i < n - 1 && arr[i] == arr[i + 1]) // same elements at the beginning
      {
         i++;
      }

      if (i == n - 1)
         return true;   // all same elements considered as sorted.

      if (arr[i] < arr[i + 1]) // candidate for ascending, non-decreasing
      {
         i++;

         for (; i < n - 1; i++)
         {
            if (arr[i] > arr[i + 1])
               return false;
         }

         return true;
      }

      else // candidate for descending, non-increasing
      {
         i++;

         for (; i < n - 1; i++)
         {
            if (arr[i] < arr[i + 1])
               return false;
         }
         return true;
      }
   }
}

Tests works, need to make sure that I'm doing in correct way.

I don't see anything wrong with the way you are testing.

You may want to include negative numbers in your test cases.

I prefer to have test cases broken up. It's easier to track down failing tests:

@Test
public void testIsSortedSingleNode()
{
    int[] C = { 32 };
    assertEquals(true, SortedOrNot.isSorted(C));
}

Even in tests you should follow naming standards. Names such as A, B C are not very descriptive. Also they should start with a lower-case letter. result would be a better name. You use the same variable throughout.

Use assertTrue and assertFalse instead of assertEquals(true/false, x).

IMO arrayLength would be a better name then n. Or you could use arr.length.


Angular 2 sortable lists I wrote a simple and small projects that present 3 buttons, each one loads a different list to the screen on the same place but not in the same time.

In my HTML, I think I can maybe use some more generic way to present the lists:

  <button md-button
          (click)="showFirstList()"
          class="md-primary">List 1
  </button>

  <button md-button
          (click)="showSecondList()"
          class="md-primary">List 2

  </button>

  <button md-button
          (click)="ThirdList()"
          class="md-primary">List 3

  </button>

    <md-content>

            ID: {{item.id}}

 name: {{item.name}}

            ID: {{item.id}}

 age: {{item.age}}

            ID: {{item.id}}

 age: {{item.age}}

    </md-content>

Now this is my component.ts. I know it's horrible and I would love to get suggestions how cI can make it look better and not repetitive. Specifically, how I can use enums instead of the booleans to make it look much better, and where I should put the calls I do for the service to get the data. It's an API calls and it doesn't make sense to call them all three when the class is loaded.

This is my sorting-lists.component.ts:

@Component({
  selector: 'sorting-lists',
  styles: [require('./sorting-lists.css')],
  directives: [DND_DIRECTIVES],
  providers: [DND_PROVIDERS],
  template: require('./sorting-lists.component.html')
})

@Injectable()
export class MyCmp implements OnInit {

  listOneToDisplay  = this._myService.getFirstListData();
  listTwoToDisplay = this._myService.getSecondListData();
  listThreeToDisplay = this._myService.getThirdListData();

  showingListOne = false;
  showingListTwo = false;
  showingListThree = false;

  constructor(private _myService: MyService) {
  };

  public showFirstList(): void {
    this.showingListOne = true;
    this.showingListTwo = false;
    this.showingListThree = false;
  }

  public showSecondList(): void {
    this.showingListTwo = true;
    this.showingListOne = false;
    this.showingListThree = false;
  }

  public showThirdList(): void {
    this.showingListThree = true;
    this.showingListTwo = false;
    this.showingListOne = false;
  }

}

Minimize component state

It's better to use a single field listToShow rather than having a show* field per each list. You don't have to change anything in your template if you wrap the show*List() around listToShow using accessor syntax.

private listToShow: number = 1;
get showingListOne(): boolean { return this.listToShow === 1; };
get showingListTwo(): boolean { return this.listToShow === 2; };
get showingListThree(): boolean { return this.listToShow === 3; };

public showFirstList(): void {
  this.listToShow = 1;
}

public showSecondList(): void {
  this.listToShow = 2;
}

public showThirdList(): void {
  this.listToShow = 3;
}

Injectable

I am very positive your component does not have to be annotated with @Injectable(), therefore it should not be. The component is not really being injected anywhere, right?


Calculate Hand Value in a Blackjack Hand I have built a Blackjack simulator in Python. I need to decrease the execution time. I have used a profiler to identify key bottlenecks in the execution.
The following function is responsible for about 15% of the total run time.

def value(self):
    value = 0
    has_ace = False

    for card in self.cards:
        value += card.value

        if card.rank == "A":
            has_ace = True

    if has_ace and value <= 11:
        value += 10

    return value

The above function is used to calculate the score of a hand.
A hand can have multiple cards.
A card has a value and a rank.

In Blackjack, an Ace can be worth 1 or 11.

Is there a better way to go?

This function doesn't have any really obvious inefficiencies, so I assume we're looking to shave off every possible clock cycle...

Sometimes it's faster to use Python's built-in aggregation functions than to use a for loop.  Here's how you could use sum and any:

def value(self) -> int:
    value = sum(card.value for card in self.cards)
    if value <= 11 and any(card.rank == "A" for card in self.cards):
        value += 10
    return value

Note the ordering of the and expression to make sure that the any iteration only happens if the value condition has already been met!

If you have flexibility over the representation of the cards, you might try making the rank an Enum with integer values.  I'd expect comparing two integer enums to be just a smidge faster than comparing two strings.


Counting letters in a string This little program is self-explanatory. I count letters in a string (can be any string), using a for loop to iterate through each letter. The problem is that this method is very slow and I want to avoid loops.

Any ideas? I thought that maybe if I remove checked letters from the string after each loop, then in some cases, where many letters repeat, that would make a difference.

def count_dict(mystring):
    d = {}
# count occurances of character
    for w in mystring:
        d[w] = mystring.count(w)
# print the result
    for k in sorted(d):
        print (k + ': ' + str(d[k]))

mystring='qwertyqweryyyy'
count_dict(mystring)

The output:

e: 2
q: 2
r: 2
t: 1
w: 2
y: 5

Use the built in Counter in the collections module:

>>> from collections import Counter
>>> Counter('qwertyqweryyyy')
Counter({'y': 5, 'e': 2, 'q': 2, 'r': 2, 'w': 2, 't': 1})


Code to modify string as per instructions I solved this problem statement on dear old codeforces.
The Problem Statement wants users to replace occurrences of '10' in the given string with either '1' or '0' until there is no occurrence of '10' left and the string formed is the lexicographically smallest string from the set of all valid answers.
This is my solution (and it has been accepted). However, I would like to know if we can simplify this solution further:
#include <algorithm>
#include <iostream>
#include <string>
#include <vector>

int main() {
    int T = 0;
    std::cin >> T;
    while (T--) {
        int n = 0;
        std::cin >> n;
        std::string str;
        std::cin >> str;
        while (str.find("10", 0) != std::string::npos) {
            auto it = std::find(str.begin(), str.end(), '1');
            auto begin = it;
            std::string window;
            while (it != str.end() && *it == '1') {
                window.push_back(*it);
                it++;
            }
            while (it != str.end() && *it == '0') {
                window.push_back(*it);
                it++;
            }
            if (std::count(window.begin(), window.end(), '0') == 0 || std::count(window.begin(), window.end(), '1') == 0) {
                std::cout << str << "\n";
                break;
            }
            while (window.size() > 1) {
                auto count0 = std::count(window.begin(), window.end(), '0');
                auto count1 = std::count(window.begin(), window.end(), '1');
                if (count0 > count1) {
                    window.erase(--window.end());
                }
                else if (count1 > count0) {
                    window.erase(window.begin());
                }
                else {
                    if (str.find("10", std::distance(str.begin(), it)) == std::string::npos) {
                        window.erase(window.begin());
                    }
                    else {
                        window.erase(--window.end());
                    }
                }
            }
            *(--it) = window.front();
            str.erase(begin, it);
        }
        std::cout << str << "\n";
    }
    return 0;
}

Please review and suggest :)

Add some comments describing your algorithm
Your code is a single function which is a bit long, and it's not very obvious what it is doing. There are two ways of dealing with this: either creating functions that implement logical steps of the algorithm individually, or add some comments to describe which steps your algorithm is doing. Don't add comments that just descibre exactly what the code does, but give a higher level description.
Unnecessary use of std::count()
You use std::count() a lot, but you don't need to. You can count the number of zeroes and ones when building window, and when you are calling window.erase() you can decrement your counts.
Consider using std::string functions to search for blocks of zeroes and ones
Instead of using std::find() and counting manually using while-loops, you can use std::string::find(). You can also construct a string directly from a subregion of another string.
auto first1 = str.find('1');
auto next0 = str.find('0', first1);
auto next1 = str.find('1', next0);

std::string window(str, first1, next1 - first1);

Once you have that, calculating the number of zeores and ones in window is easy:
auto count1 = next0 - first1;
auto count0 = window.size() - count1;

However, you might not even need the initial window:
Avoid constructing and then partially deconstructing the window
Your window consists of a number of ones followed by a number of zeroes. You are then slowly removing from the start and end until some conditions are met. However, while you are doing this, the only thing you really care about is the count of zeroes and ones, not the string window itself! You can reconstruct the final window from the counts left at the end.
Let's assume we have calculated count0 and count1 like I've written above. Then:
while (window.size() > 1) {

Can be replaced with:
while (count0 + count1 > 1) {

Then you do:
if (count0 > count1)
    window.erase(--window.end());
else if (count1 > count0)
    window.erase(window.begin());

Basically, this removes zeroes or ones until the number of zeroes or ones is equal. This can be replaced with the following:
count0 = count1 = std::min(count0, count1);

Then the next condition is:
if (str.find("10", std::distance(str.begin(), it)) == std::string::npos)
    window.erase(window.begin());
else
    window.erase(--window.end());

Since str is not modified here, you could move the if-condition outside of the loop. And the window.erase() calls can be replaced by count0-- and count1--. So what's left is:
bool has_second_10 =  str.find("10", next0 + count0) != str.npos;

while (count0 && count1) {
    if (count0 != count1) {
        count0 = count1 = std::min(count0, count1);
    } else {
        if (has_second_10) {
            count0--;
        } else {
            count1--;
        }
    }
}

But wait, it gets even better: you always start with a window with both ones and zeroes, and you make the count equal, and then you remove zeroes if the original string had a second occurrence of "10", otherwise you remove ones. So the single character left in the window is completely determined by has_second_10, and the while-loop can be eliminated entirely. You also don't need to count ones and zeroes anymore.
Now it's just a matter of updating the original string. Instead of modifying a character and erasing some, you can use the std::string::replace() function to do that in one go:
str.replace(first1, next1 - first1, 1, has_second_10 ? '1' : '0');

Summary
With the above improvements, the code can be simplified to:
#include <iostream>
#include <string>

int main() {
    int T = 0;
    std::cin >> T;

    while (T--) {
        int n = 0;
        std::cin >> n;
        std::string str;
        std::cin >> str;

        while (str.find("10", 0) != str.npos) {
            auto first1 = str.find('1');
            auto next0 = str.find('0', first1);
            auto next1 = str.find('1', next0); // this might be npos, but that's fine
            auto replacement = str.find("10", next1) == str.npos ? '0' : '1';
            str.replace(first1, next1 - first1, 1, replacement);
        }

        std::cout << str << "\n";
    }
}


Handling multiple queries in C# In my project I am using 10-15 SQL queries, for that I have a common class for database connectivity and using that I have executed the queries by passing the connection, command and dataReader.

With some reference in SO, I learned that my code is not properly structured.  I would like to learn and write high performance code.

DBConnectivity.cs:

class DBConnectivity
    {
        public SqlConnection connection = null;
        public SqlCommand command = null;
        public SqlDataReader dataReader = null;
        public string connectionString = null;
        public List<MasterTableAttributes> masterTableList;
        public DBConnectivity()
        {
            connectionString = ConfigurationManager.ConnectionStrings["Master"].ConnectionString;
            connection = new SqlConnection(connectionString.ToString());

            //-----Master table results
            connection.Open();
            string masterSelectQuery = "SELECT * FROM MASTER_TABLE";
            command = new SqlCommand(masterSelectQuery, connection);
            dataReader = command.ExecuteReader();
            masterTableList = new List<MasterTableAttributes>();

            while (dataReader.Read())
            {
                MasterTableAttributes masterTableAttribute = new MasterTableAttributes()
                {
                    fileId = Convert.ToInt32(dataReader["Id"]),
                    fileName = Convert.ToString(dataReader["FileName"]),
                    frequency = Convert.ToString(dataReader["Frequency"]),
                    scheduledTime = Convert.ToString(dataReader["Scheduled_Time"])
                };
                masterTableList.Add(masterTableAttribute);
            }
            dataReader.Close();
            connection.Close();
        }
    }

Program.cs:

class Program
    {
        static void Main(string[] args)
            {
                DBConnectivity dbConnectivity = new DBConnectivity();
                Transaction txnObj = new Transaction(dbConnectivity); //Query1
                Sales saleObj = new Sales (dbConnectivity);//Query2
           }
    }

Transaction.cs:

class Transaction
    {
        public Transaction(DBConnectivity dbConnectivity)
        {
            dbConnectivity.connection.Open();
            string txnQuery = "SELECT ...";

            dbConnectivity.command = new SqlCommand(txnQuery, dbConnectivity.connection);

            dbConnectivity.dataReader = dbConnectivity.command.ExecuteReader();

            while (dbConnectivity.dataReader.Read())
            {
                ...
            }
            dbConnectivity.dataReder.close();
            dbConnectivity.connection.Close();
        }
    }

Sales.cs:

class Sales
    {
        public Sales(DBConnectivity dbConnectivity)
        {
            dbConnectivity.connection.Open();
            string salesQuery = "SELECT ...";

            dbConnectivity.command = new SqlCommand(salesQuery, dbConnectivity.connection);

            dbConnectivity.dataReader = dbConnectivity.command.ExecuteReader();

            while (dbConnectivity.dataReader.Read())
            {
                ...
            }
            dbConnectivity.dataReder.close();
            dbConnectivity.connection.Close();
        }
    }

The reader/command/connection fields of DBConnectivity are not really being used to represent state of DBConnectivity. This is obvious because of usage like:

dbConnectivity.command = new SqlCommand(salesQuery, dbConnectivity.connection);
dbConnectivity.dataReader = dbConnectivity.command.ExecuteReader();
while (dbConnectivity.dataReader.Read()) {...}

frankly, they should just be local variables:

var command = new SqlCommand(salesQuery, connection);
var dataReader = dbConnectivity.command.ExecuteReader();
while (dataReader.Read()) {...}

Additionally, in all uses, you are not correctly disposing your resources. The IDisposable contract is important: if something is IDisposable, then it is your job to make sure it gets disposed when done. So with the above example:

using(var command = new SqlCommand(salesQuery, connection))
using(var dataReader = dbConnectivity.command.ExecuteReader())
{
    while (dataReader.Read()) {...}
}

This using makes a huge difference, and ensures you don't drop connections etc on the floor when exceptions happen.

Personally, I would not advocate doing this type of work in a constructor - that is not intuitive. A static method that returns a populated instance, sure - or an instance method that populates an existing instance. But a constructor? not so clear.

I also suspect that you could benefit from the use of tooling to avoid boiler-plate code that is error-prone and not important to your actual work. For example, tools like dapper allow much simpler object population. For example:

public class DBConnectivity
{
    private readonly string connectionString;
    public DBConnectivity(string connectionString = null)
    {
        if(string.IsNullOrEmpty(connectionString))
        {
            connectionString = ConfigurationManager.ConnectionStrings["Master"].ConnectionString;
        }
        this.connectionString = connectionString;
    }
    public SqlConnection OpenConnection() {
        var conn = new SqlConnection(connectionString);
        conn.Open();
        return conn;
    }
    public List<MasterTableAttributes> GetMasterTableList()
    {
        using(var connection = OpenConnection())
        {
            const string sql = "SELECT Id as [FileId], FileName, Frequency, Scheduled_Time as [ScheduledTime] FROM MASTER_TABLE";
            return connection.Query<MasterTableAttributes>(sql).ToList();
        }
    }
}

Here the only useful state is the connection string, with a method that creates new connections. Note how this is used with using to create and dispose the connection, with "dapper" then taking the load for handling the data.

If you really wanted to pass this around, you can:

    public List<SomethingSalesEsque> SomeMethodOnSales(DBConnectivity dbConnectivity)
    {
        using(var conn = dbConnectivity.OpenConnection())
        {
            return conn.Query<SomethingSalesEsque>(someQuery).ToList();
        }
    }

From your code, I'm guessing you have:

class MasterTableAttributes
{
    public int fileId;
    public string fileName;
    public string frequency;
    public string scheduledTime;
}

this would preferably be:

class MasterTableAttributes
{
    public int FileId {get;set;}
    public string FileName {get;set;}
    public string Frequency {get;set;} // maybe an enum?
    public string ScheduledTime {get;set;} // datetime? timespan?
}


Generate powerset in JS I came across a mock interview question in which the candidate is asked to generate the powerset of a given set.

The input set is represented as a unique array of integers.

There was no solution provided in this instance, so I tried to see what i could come up with on my own. I did struggle my way to a solution and I'm not particularly dissatisfied with it, but I'm not particularly satisfied either.

I took a recursive approach. Essentially, I generated a tree, depth first, with the uppermost level consisting of the original set and each successive lower level consisting of the immediate subsets of the parent node.

Here's my code. I am using ES5.

    var result = generate_powerset([1, 2, 3, 4]);
    console.log(result);

    function generate_powerset(set) {
      var powerset = generate_subsets(set);
      powerset.push(set);
      return powerset;
    }

    function generate_subsets(set) {
      var subsets = [];
      set.forEach(function(element, i) {
        var immediate_subset = remove_index(set, i);
        maybe_push(subsets, immediate_subset);
        var extended_subsets = generate_subsets(immediate_subset);
        extended_subsets.forEach(function(extended_subset) {
          maybe_push(subsets, extended_subset);
        });
      });
      return subsets;
    }

    function remove_index(array, i) {
      return array.filter(function(el, j) {
        return i != j;
      });
    }

    function maybe_push(set, non_member) {
      var already_member = false;
      set.forEach(function(member) {
        if (are_identical(member, non_member)) already_member = true;
      })
      if (!already_member) set.push(non_member);
    }

    function are_identical(set1, set2) {
      return (JSON.stringify(set1.sort()) == JSON.stringify(set2.sort()));
    }

I'm looking for ways to:

Make this algorithm more efficient
Make this code easier to understand
Make my code shorter

I'm also just generally curious if this approach is considered orthodox or unorthodox.

Here's another take on the powerset algorithm:

function powerset(l) {
    // TODO: ensure l is actually array-like, and return null if not
    return (function ps(list) {
        if (list.length === 0) {
            return [[]];
        }
        var head = list.pop();
        var tailPS = ps(list);
        return tailPS.concat(tailPS.map(function(e) { return [head].concat(e); }));
    })(l.slice());
}

// Test cases:
console.log(powerset([1,2,3,4]));
console.log(powerset([10,30,20]));

It works by using the following inductive observation:

$$\mathcal{P}(X \cup \{e\})=\mathcal{P}(X)\cup\left\{\{e\} \cup Y~|~Y \in \mathcal{P}(X)\right\}$$

Basically, the powerset of any set X plus another element is the powerset of X along with another copy of the powerset where every set contains the element. Concisely, every element is either in a subset or not. This recurrence captures that "obvious" statement.

To specifically critique your code: given the method above, there's no reason to do a quadratic (well, exponential in the input) scan for duplicates, so this will be much more efficient. No need for JSON conversions, which take time, too. Since JavaScript is dynamically typed, you would need to put some handling code for invalid types passed into the function. These can be done before the anonymous-function-call-return gadget (which preserves the input); see the TODO in my code above.

The lack of sorting, JSON conversion, and de-duplication allows this code to create a powerset of 20 elements quickly, whereas the implementation in your question crashes Chrome.


Get value from an async function in Outlook Web Addin I'm working on an Outlook Web Addin that gets the email body. So in the Office API you could get the email body in two types: Simple Text and Html.

Our requirement is to get the HTML format so this is easy, however, even if the email body is empty the HTML format still returns a value which is the HTML elements but does not have contents in it. So my solution is to check first for the Simple Text version of the email body then if there is a content get the HTLM format version.

Please see the below codes:

var mailItem = Office.context.mailbox.item;

mailItem.body.getAsync(Office.CoercionType.Text, function (result) {
    if (result.status === Office.AsyncResultStatus.Succeeded) {
        var normalizeValue = null;

        if (result.value) {
            normalizeValue = result.value.trim();
        }

        if (normalizeValue !== '') {
            mailItem.body.getAsync(Office.CoercionType.Html, function (result) {
                if (result.status === Office.AsyncResultStatus.Succeeded) {
                    // the value will be initialized in input value
                    $('#body').val(result.value.trim());
                }
            });
        }
    }
});

I would like to get the value from the 'Text' type and pass this in a variable then check the variable then run the getAsync for the html version separately instead of having the codes inside the method of body.getAsync. Or anything the code could improve?

I would suggest you could use Promise.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises

If you don't want additional library for Promise, you could use Deferred from jQuery also. https://api.jquery.com/deferred.promise/


Repeated DB connection at max user connections I am trying to create class with connect to mysql database. And if there is max number of connections I want to wait and try it again. I figured out, how it can works, but I am not sure, if its the right way of doing it. So, my code looks like this:
<?php

class DbConnect {

  private $attemps = 1;
  private $errorCode;
  private $maxAttemps = 10;
  private $pdo;
  private $dsn = "mysql:host=localhost;dbname=...;charset=utf8";
  private $options = [
    PDO::ATTR_EMULATE_PREPARES   => false, // turn off emulation mode for "real" prepared statements
    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION, //turn on errors in the form of exceptions
    /*PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,*/ //make the default fetch be an associative array
  ];

  public function __construct(){
    $this->tryConnect();
    while ($this->errorCode == 1040 && $this->attemps <= $this->maxAttemps) {
      usleep(pow(2, $this->attemps)*10000);
      $this->tryConnect();
    }

  }

  protected function tryConnect(){
    try {
      $this->pdo = new PDO($this->dsn, "root", "", $this->options);
    } catch (Exception $e) {
      error_log($e->getMessage());
      $this->errorCode = $e->getCode();
      $this->attemps++;
    }
  }

}

?>

Is there anything i should change? Thank you very much.

You code has several problems. Specificaly it breaks at least two of the SOLID principles, namely single responsibility and dependency inversion principles.
Single responsibility is violated because the class is responsible for:

connecting to db
retrying connection
whatever other methods provided by the class you did not show us

Dependency inversion principle is violated because the class uses hardcoded values (although stored in instance properties).
Let me first separate out the first responsibility. We don't really need OOP for that, so let me do it in FP style.
function createMyPdo(): \PDO
{
  $dsn = ...;
  $user = ...;
  $password = ...;
  $options = ...;
  return new \PDO($dsn, $user, $password, $options);
}

This function now opens a connection with hardcoded credentials. Although the credentials are hardocded and we would better pull those values off of maybe environment variables. It at least has just one responsibility and that is to open the specific database connection.
Now lets implement the retry logic as a separate thing, that only depends on something that creates PDO instance.
function createPdoWithRetry(callable $factory, int $attempts): \PDO
{
  $attemptsMade = 0;
  while ($attempts > $attemptsMade) {
    try {
      return $factory();
    } catch (\PDOException $e) {
      if ($e->getCode() === 1040) {
        \usleep(\pow(2, $attempsMade) * 10000);
        ++$attemptsMade;
      } else {
        throw $e;
      }
    }
  }
  throw new \RuntimeException("All $attemptsMade retry attempts exhausted.");
}

Now I can easily open the connection with retry logic
$pdo = createPdoWithRetry(fn () => createMyPdo(), 20);

And I can also open a different connection using the same thing:
$pdo2 = createPdoWithRetry(fn () => createOtherPdo(), 20);

This approach still has some caveats:

where createMyPdo could only throw PDOException, createPdoWithRetry can also throw a generic RuntimeException.
the type of the $factory parameter has a very vague type, that we have to describe in a docblock and cannot be enforced by PHP runtime.
createPdoWithRetry knows error code 1040 which is mysql specific, but the knowledge that mysql is used is only known to the createMyPdo function.

Let's make it better by going back to OOP style and introducing an interface for the connection factory, including specific exceptions to be thrown (which will abstract away the 1040 speific mysql error code).
class DatabaseConnectionException extends \Exception
{
}

class TooManyConnectionsException extends DatabaseConnectionException
{
}

interface DatabaseConnector
{
  /**
   * @return \PDO
   * @throws DatabaseConnectionException
   * @throws TooManyConnectionsException
   */
  public function createPdo(): \PDO;
}

As you can see I added some @throws annotations to:

inform implementors of the interface which exception they can throw
informs consumers of the interface which exceptions they can catch

Although PHP cannot enforce the actual type of exceptions thrown from the implementations, documenting the exceptations makes it less likely that someone will implement it wrong.
Now lets make an implementaton for our specific connection:
class MyDatabaseConnector implements DatabaseConnector
{
  public function createPdo(): \PDO
  {
    $dsn = ...;
    $user = ...;
    $password = ...;
    $options = ...;
    try {
      return new \PDO($dsn, $user, $password, $options);
    } catch (\PDOException $e) {
      if ($e->getCode() === 1040) {
        throw new TooManyConnectionsException($e->getMessage(), (int) $e->getCode(), $e);
      } else {
        throw new DatabaseConnectionException($e->getMessage(), (int) $e->getCode(), $e);
      }
    }
  }
}

Now maybe you see why the credentials should not be hardocded, because the same code would work for any credentials. Hardcoding the credentials, we are doomed to repeat some code if we want to open a different connection.
But I leave that up to you to find your way to the dependency inversion principle. Basically constructor of a class should accept things that the class instances need to know, rather then having the instances deciding on their own what those values should be (ie. pass the credentials through constructor, rather then hardocing the credentials withing the class).
Anyway, to implement the retry logic we will use the same interface:
class RetryingDatabaseConnector implements DatabaseConnector
{
  private DatabaseConnector $factory;
  private RetryDelayStrategy $delayStrategy;
  private int $maxAttempts;

  public function __construct(DatabaseConnector $factory, RetryDelayStrategy $delayStrategy, int $maxAttempts)
  {
    $this->factory = $factory;
    $this->delayStrategy = $delayStrategy;
    $this->maxAttempts = $maxAttempts;
  }

  public function createPdo(): \PDO
  {
    $attemptsMade = 0;
    while ($this->maxAttempts > $attemptsMade) {
      try {
        return $this->factory->createPdo();
      } catch (TooManyConnectionsException $e) {
          ++$attemptsMade;
          $this->delayStrategy->wait($attemptsMade)
      }
    }
    throw new TooManyConnectionsException("All $attemptsMade retry attempts exhausted.");
  }
}

As you can see I have also extracted the delay logic to a separate interface, because exponentialy growing gaps between attempts is just one of many possible strategies. And again, you don't want to implement a new connector that would repeat a lot of the logic just to change to delay strategy.
interface RetryDelayStrategy
{
  public function wait(int $attemptsMade): void;
}

class ExponentialRetryDelayStrategy implements RetryDelayStrategy
{
  private int $coefficient;
  private int $base;

  public function __construct(int $coefficient, int $base = 2)
  {
    $this->coefficient = $coefficient;
    $this->base = $base;
  }

  public function wait(int $attemptsMade): void
  {
    \usleep(\pow($this->base, $attempsMade) * $this->coefficient);
  }
}

And finaly some consumer code. A controller that opens a database connection (to do something with it) and gives a user friendly messages if the connection fails and a more specific one if the connection fails specificaly because of too many open connections.
class MyController extends BaseController
{
  private DatabaseConnector $connector;

  public function __construct(DatabaseConnector $connector)
  {
    $this->connector = $connector;
  }

  public function myControllerAction(Request $request): Response
  {
    try {
      $pdo = $this->connector->createPdo();
    } catch (TooManyConnectionsException $e) {
      return $this->send(503, "Too many database connections.");
    } catch (DatabaseConnectionException $e) {
      return $this->send(503, "Database unavailable.");
    }

    // do something with PDO here

    return $this->send(200, "All done");
  }
}

Now, you see the controller just depends on a DatabaseConnector. It doesn't really care if and how many times the connector will retry. It just cares that it can fail in a specific (TooManyConnectionsException) or an further unspecified (DatabaseConnectionException) way or, if it succeeds, it returns a PDO instance.
And it will work no matter what connector you pass to it, as long as the interface is implemented correctly. And so you can change the implementation without touching the controller code. Again, thanks to single responsibility a dependency inversion principles.
No matter which is used in the DI setup
$myConnector = new MyDatabaseConnector();
// or maybe if we pull it to envs
// $myConnector = new MyDatabaseConnector($_ENV['DB_DSN'], $_ENV['DB_USER'], $_ENV['DB_PASS']);
$connector = $myConnector;

or
$delayStrategy = new ExponentialRetryDelayStrategy(10000, 2);
$retryingConnector = new RetryingDatabaseConnector($myConnector, $delayStrategy, 20);
$connector = $retryingConnector;

the controller code stays untouched
$controller = new MyController($connector);
...
$controller->run();

Also notice, how I avoided setting an errorCode and checking it in the controller. Instead we just throw exceptions. This forces the caller to actually handle the error or let it bubble up the stack and eventually stop execution of the program.
This is the prefered behaviour because if you would forget to check the error code, it really does not make sense to try to send sql queries over a "not opened connection".
Btw, if you are interested in design patterns, you can notice that:

DatabaseConnector and its implementations are a factory method pattern
RetryingDatabaseConnector and the interface follow the decorator pattern
RetryDelayStrategy and its implementations are the strategy of the strategy design pattern and RetryingDatabaseConnector is the strategy consumer


Distribute items based on the time duration I just finished a small assignment and I would like to get some feedback about the implementation here. Basically, it was all about distributing items ("talks" in this case) throughout the day based on the time constraints (their duration). For instance, a given place have different "tracks" each of which has a morning (9AM - 12PM) and afternoon (1PM - 5PM) session.

The input data is a file with this structure: Talk Name XYmin (where XY are positive numbers).

This is what I came up with (I know it's a little bit silly but I guess it works for the most part):
public final class Conference {
  private static final long MORNING_SPAN = 180; // 09:00 AM - 12:00PM

  private static final long AFTERNOON_SPAN = 240; // 01:00 PM - 05:00 PM

  private final List<Talk> talks;

  private final List<Track> tracks;

  private final long numberOfTracks;

  public Conference(final List<Talk> talks) {
    final var duration = talks.stream().mapToLong(Talk::duration).sum();
    final var daySpan = MORNING_SPAN + AFTERNOON_SPAN;
    this.talks = talks;
    numberOfTracks = duration / daySpan + (duration % daySpan == 0L ? 0L : 1L);
    tracks = new LinkedList<>();
  }

  public List<Track> getTracks() { return tracks; }

  public void displaySchedule() {
    if (tracks.isEmpty()) {
      makeSchedule();
    }
    // More System.out.println statements for the required output
  }

  private void makeSchedule() { // ...this one is the deal!
    for (int i = 0; i < numberOfTracks; i++) {
      final var morning = new LinkedList<Talk>();
      final var afternoon = new LinkedList<Talk>();
      var morningSpan = MORNING_SPAN;
      var afternoonSpan = AFTERNOON_SPAN;
      final var iterator = talks.iterator();
      while (iterator.hasNext()) {
        final var talk = iterator.next();
        if (talk.duration() <= morningSpan) {
          morning.add(talk);
          morningSpan -= talk.duration();
          iterator.remove();
        } else if (talk.duration() <= afternoonSpan) {
          afternoon.add(talk);
          afternoonSpan -= talk.duration();
          iterator.remove();
        }
      }
      tracks.add(new Track(morning, afternoon));
    }
  }
}

public final class Talk {
  public final String title;

  public final String duration;

  public Talk(final String title, final String duration) {
    this.title = title;
    this.duration = duration;
  }

  public long duration() {
    if ("lightning".equalsIgnoreCase(duration)) {
      return Constant.LIGHTNING;
    }
    // Basically this returns the duration for the talk (as long)...that's it
    return Long.parseLong(Constant.NON_NUMBERS.matcher(duration).replaceAll(""));
  }
}

public final class Track {
  private final List<Talk> morning;

  private final List<Talk> afternoon;

  public Track(final List<Talk> morning, final List<Talk> afternoon) {
    this.morning = morning;
    this.afternoon = afternoon;
  }

  // Some more stuff here to display the required output
}

The real deal here is Conference::makeSchedule — that's where I would like to know if there is a better, more efficient way to approach this solution...probably using different data structures, etc. Thanks in advance!

Nice implementation, it's well structured and easy to understand. Few general suggestions:

int instead of long: the constants MORNING_SPAN and AFTERNOON_SPAN contain minutes in a day, using an int is enough.
Input validation: the talk duration needs to be validated. If the talk duration is greater than MORNING_SPAN or AFTERNOON_SPAN it won't be included in any track. If it's negative it will also lead to issues. In the Conference class ensure that the list talks is not empty.
Creating the schedule: I see that the schedule is created (lazily) in the displaySchedule method of Conference. I think that creating the schedule is an important action, it can be the responsibility of another class, like ScheduleMaker. Then, the list of tracks can be added to Conference in the constructor.
Talk interface: talk.duration returns a string like "30min", while talk.duration() returns 30 as a long. I find it a bit confusing. If you need both, consider creating two different variables and two getters. Otherwise, parse the duration in the constructor and use only the long value.

Bin packing problem
Let's consider this example:
morning_span = 2m (2 minutes)
afternoon_span = 1m
daily_span = morning_span + afternoon_span = 3m

talk1 = 2m
talk2 = 2m
talk3 = 2m
total_talks_duration = 6m

Calling conference.makeSchedule() will result in:
numberOfTracks = total_talks_duration / daily_span = 6 / 3 = 2

Schedule:
Track1 = talk1 (morning)
Track2 = talk2 (morning)
talk3 ??

As you can see the current makeSchedule will create two tracks, but talk3 won't be included.
This bug can be fixed by running the other loop till all the talks are placed:
private void makeSchedule() {
    while (talks.size() > 0) {
        // Create a new track
        // Add in the track as many talks as possible
        // Remove talks added to the current track
    }
}

Now all talks will be placed in the tracks, however, there are cases where the algorithm will create more tracks than needed. This is considered a Bin packing problem. Basically, fit the talks in the minimum number of tracks. Interestingly, my university professor developed an exact algorithm for this problem (pdf). I'll leave it to you for additional study.


Project Euler #82 - path sum: three ways Project Euler problem 82 asks:

Here's my solution:

def main():
    matrix = [
        [131, 673, 234, 103,  18],
        [201,  96, 342, 965, 150],
        [630, 803, 746, 422, 111],
        [537, 699, 497, 121, 956],
        [805, 732, 524,  37, 331]
    ]

    size = len(matrix)
    best = [matrix[row][0] for row in range(size)]

    for col in range(1, size):
        column = [matrix[row][col] for row in range(size)]
        tmp = column[:]

        for i in range(size):
            column[i] += best[i] # right

            for j in range(0, i): # up
                if sum([best[j]]+tmp[j:i+1]) < column[i]:
                    column[i] = sum([best[j]]+tmp[j:i+1])

            for k in range(i, size): # bottom
                if sum([best[k]] +tmp[i:k+1]) < column[i]:
                    column[i] = sum([best[k]] +tmp[i:k+1])

        best = column
        #print(best)

    return min(best)

if __name__ == "__main__":
    print(main())

Please advise how it can be improved.

To solve the Project Euler challenge, you'll have to handle a large matrix from a file.  Therefore, your function should accept the matrix as a parameter.

You used size to represent both the number of rows and the number of columns.  I would use two variables to avoid hard-coding the assumption that the input is a square matrix.

Your dynamic programming algorithm is sound.  I would use the min() function to avoid finding the minimum by iteration.  Also, setting best all at once instead of mutating the elements of column avoids having to copy column into a temporary variable.

def euler82(matrix):
    nrows, ncols = len(matrix), len(matrix[0])
    best = [matrix[row][0] for row in range(nrows)]

    for col in range(1, ncols):
        column = [matrix[row][col] for row in range(nrows)]

        best = [
            # The cost of each cell, plus...
            column[row] +

            # the cost of the cheapest approach to it
            min([
                best[prev_row] + sum(column[prev_row:row:(1 if prev_row <= row else -1)])
                for prev_row in range(nrows)
            ])
            for row in range(nrows)
        ]

        #print(best)

    return min(best)


Find all classes A that implement B and aggregate both A and T to build a map out of them In my project I have the following base interfaces used through the application:

interface Node { /* ... */ }
interface NodeService<T> { /* ... */ }

Concrete classes might look as follows:

class User implements Node { /* ... */ }
class UserService implements NodeService<User> { /* ... */ }

class Entity implements Node { /* ... */ }
class EntityService implements NodeService<Entity> { /* ... */ }

In the end I want to have a Map<String, Class<? extends NodeService<Node>>> that in the case above might look as follows:

Map{
    User=UserService.class,
    Entity=EntityService.class
}

My current solution has the following approach:

Using the Reflections library find all classes implementing NodeService
Iterate over all generic interfaces of said classes until we find the NodeService interface
Access its first and single generic type argument
Cast the type to a class and map it to its simple name
Build a tuple (using vavr in this case) containing the simple name and the service class we found in step 1
Flatten the result and collect the tuples into a map

@SuppressWarnings({"unchecked", "rawtypes"})
final Map<String, Class<? extends NodeService<Node>>> nodeServices =
    new Reflections(BackendApplication.class.getPackageName())
        .getSubTypesOf(NodeService.class).stream()
            .map(
                serviceClass ->
                    Stream.of(serviceClass.getGenericInterfaces())
                        .filter(ParameterizedType.class::isInstance)
                        .map(ParameterizedType.class::cast)
                        .filter(type -> type.getRawType().equals(NodeService.class))
                        .map(ParameterizedType::getActualTypeArguments)
                        .map(typeArguments -> typeArguments[0])
                        .filter(Class.class::isInstance)
                        .map(Class.class::cast)
                        .map(Class::getSimpleName)
                        .map(nodeClass -> Tuple.of(nodeClass, (Class) serviceClass))
                        .collect(Collectors.toSet()))
            .flatMap(Collection::stream)
            .collect(Collectors.toMap(Tuple2::_1, Tuple2::_2));

I am surprised that this works in the first place but the code is ridiculously complex and I am looking for suggestions on how to rewrite this. Some of my biggest pain points are the handling of the classes generic interfaces even though in the end I'm only interested in a single one of them making the stream in the middle introduce unnecessary nesting. Furthermore the way I attempt to navigate to the correct type is really bothering me as well.

Thus I am open for your input on how to improve this chunk of code.

The performance of this snippet is surprisingly good. I am mainly looking for input on the readability and refactoring this to make it much more maintainable, even at the cost of performance since this will be only invoked once during the application lifecycle.

Should we expect that there are services that have a different type parameter? Because otherwise just a Set of services may suffice, or you can simply remove the Service part of the name during the mapping. This is an obvious thing from the example, but I'm not sure how far the example matches your actual code.

What I often see when streams are used is that suddenly the split up into methods is forsaken. For instance, in my code I would first build a list of services and then create a map out of it. I don't think that an intermediate list would do much w.r.t. performance here (the number of services will be limited, loading the classes will take much more time, and the streaming will have intermediate representations as well), and the code would clearly be simplified. Method chaining can be nice, but please don't overdo it.

Similarly, a method that tests if a class implements an interface can be clearly split out:

Stream.of(serviceClass.getGenericInterfaces())
    .filter(ParameterizedType.class::isInstance)
    .map(ParameterizedType.class::cast)
    .filter(type -> type.getRawType().equals(NodeService.class))

And actually, if you do that and look for code you will find NodeService.class.isAssignableFrom(clazz). The part is well programmed but please do use library functions when they are available.

All of above may not do much with regards to performance, but taking it into account should definitely help with the readability. Currently you had to describe what the code does in the question description. That should not be necessary for well programmed / documented code.


rand number generator with limits in ruby I have an array of 1000 numbers, randomly generated. 10 of those numbers must be between 0-9 (including 0 and 9), and 10 of those numbers must be between 991 and 1000 (including 991 and 1000). This is what I came up with:

arr = []
980.times do
  arr << (11..989).to_a.sample
end

arr2 = []
10.times do
  arr2 << rand(10)
end

arr3 = []
10.times do
  arr3 << (990..1000).to_a.sample
end

arr4 = []
arr4 = arr + arr2 + arr3

arr4.shuffle

Is there a more elegant way to do this in ruby?

Some notes:

Try to learn some functional programming, you shouldn't write Ruby as it were a low-level language (like C). Favor expressions over statements. My 2-cents on the matter, I hope it helps: http://www.slideshare.net/tokland/functional-programming-with-ruby-9975242
Let me show a simple example of using FP. The first 4 lines of your code could be written like this: arr = 980.times.map { (11..989).to_a.sample }
arr4 = []: Why? the next line assigns it to another expression.
range.to_a.sample. That's very inefficient, better rand(range).

I'd write:

ranges = [0..9, 11..989, 990..1000]
output = ranges.flat_map do |range|
  range.map { rand(range) }
end.shuffle


Project Euler #1 in Go (counting multiples of 3 or 5 up to 1000) I've been working on Project Euler, here was my solution for Problem #1. It gave me the proper answer, but it's egregiously slow. How can I implement this more efficiently? My math skills aren't top-notch, sorry.

package main

import (
    "fmt"
)

// Problem1: find the sum of all the multiples of 3 or 5 below 1000.
// x,y: multiples
// z: upper limit
func Problem1(x, y, z int) int {

    Multiples := make(map[int]struct{})

    var Sum int
    for i := 1; i < z; i++ {
        XFactor := x * i
        YFactor := y * i

        if XFactor < z {
            Multiples[XFactor] = struct{}{}
        }
        if YFactor < z {
            Multiples[YFactor] = struct{}{}
        }
    }

    for key, _ := range Multiples {
        Sum = Sum + key
    }

    return Sum
}

func main() {
    fmt.Println("The answer to problem 1 is: ", Problem1(3,5,1000))
}

Thanks! https://play.golang.org/p/MQLpjtMvmw

Closed form solution

The problem is basically a sum over an arithmetic sequence, which has the well defined solution (first + last) * count / 2

func ArithmeticSum(first, step, count int) int {
    var last int = first + step * (count - 1);
    return (first + last) * count / 2;
}

The only difficulty arises from the requirement to sum numbers that are a multiple of both x and y just once. The easiest way to achieve this is to sum x and y independently and subtract the common multiples.

func Problem1(x, y, z int) int {
    var sumX int = ArithmeticSum(0, x, (z - 1) / x + 1);
    var sumY int = ArithmeticSum(0, y, (z - 1) / y + 1);
    var lcm int = Lcm(x, y);
    var sumBoth int = ArithmeticSum(0, lcm, (z - 1) / lcm + 1);
    return sumX + sumY - sumBoth;
}

Luckily, the common multiples also form an arithmetic series. It's step is the least common multiple of x and y.

func Lcm(a, b int) int {
    return a * b  / Gcd(a, b);
}

The least common multiple can be calculated from the greatest common divisor, which itself can be found via Euclid's algorithm.

func Gcd(a, b int) int {
    if b == 0 {
        return a;
    } else {
        return Gcd(b, a % b);
    }
}


Python Caesar Cipher I am a student doing their AS Computing course and I have created a piece of code in which a message can be typed and then scrambled by moving letters along the alphabet. I was wondering if anyone knew any way in which this could be improved.

while(True):
 message = input("What would you like the message to say? ").lower()
 newmessage = ""
 for i in range(0,26):
    for letter in message:
        decimal = (ord(letter))
        decimal = decimal + i

        if decimal > 122:
            decimal = decimal -26

        elif decimal < 97:
            decimal = decimal + 26

        newmessage += (chr(decimal))

    print (newmessage)
    newmessage = ""

Here are some comments to your code, which will be followed by an alternate implementation:

Use functions to do the rotation – If you have learned about functions I would strongly suggest to use functions to do the actual rotation. You call it with the string to rotate and rotation offset, and you'll get the rotated string in return.
Avoid magic numbers – In your code you use magic numbers like 122 and 97, your code looks cleaner if you replace these by what they actually are: ord('z') and ord('a'). Alternatively use constants like ALPHABET_LENGTH = 26, but I'll come back to this one
Don't hide reserved words or modules – decimal is a module in Python, and to use that as a variable name is considered bad practice.
Naming scheme dictates to use snake_case –  Instead of newmessage it is recommended to use new_message and similar. This reads more easy.
Be consistent on indentation – You use a varying amount of spaces to indent your code, please be consistent and use 4 spaces. The previous and this item is related to conforming to the PEP8 guidelines. Please read and follow these guidelines, as that is quite common when using Python.

Algorithm considerations

If keeping to your format of not using functions, there are still some issues related to your algorithmic choices:

What about letters not in the lowercase alphabet? – You don't handle punctuation chracters and non-alpha characters so these would leave somewhat strange behaviour
You don't have a means to exit your code – You are essential having an eternal loop, leaving you to kill the program execution as only means of leaving it
Range checks can be joined in Python – Instead of doing one lower than and one larger than you could do 97 < var < 122 to verify a complete range (This will make better sense in code example below)
range() default to start at 0 – That is range(0, 26) and range(26) is the same, and the latter is preferred for clarity
You don't need to do (expr), unless you want line continuation – It is better to write new_message += chr(decimal) without the extra parentheses. You could use them if you want to spread the expression within over multiple lines, but normally keep the expression without.

Adhering to my style and algorithmic considerations we arrive at this code:

ALPHABET_LENGTH = 26

while(True):
    message = input("What would you like the message to say? ").lower()

    if message == "quit":
        break

    for i in range(ALPHABET_LENGTH):
        new_message = ""
        for letter in message:
            if 'a' <= letter <= 'z':
                char_pos = ord(letter) - ord('a')

                # Rotate by i, and make sure we're within alphabet
                char_pos = (char_pos + i) % ALPHABET_LENGTH

                new_message += chr(char_pos + ord('a'))

                # new_message += chr((ord(letter) - ord('a') + i) % ALPHABET_LENGTH + ord('a'))
            else:
                new_message += letter

        print (new_message)

Within the rotation block there is a commented line which could replace all of the other code, but it is not so easy to read and understand as the uncommented code.

Alternate implementation with functions

In general I would rather use functions to do stuff like this, and I would encapsulate my entire program within a if __name__ == '__main__': block to allow for module execution. That is that the script could be imported as a module, and you could call the different functions to add caesar-cipher rotation to other programs of yours.

One version of this could be like this:

from string import ascii_lowercase

def rotate_lowercase_char(char, offset):
    """Return lowercase <char> rotated by <offset> characters.

    If <char> is a lowercase character rotate it, or else return
    it unchanged allowing for non-alpha characters to be present."""

    if not char in ascii_lowercase:
        return char
    else:
        position = ascii_lowercase.index(char)
        position = (position + offset) % len(ascii_lowercase)
        return ascii_lowercase[position]

def rotate_lowercase_text(text, offset):
    """Return <text> rotated by <offset> characters."""

    return ''.join(rotate_lowercase_char(letter, offset) for letter in text)

def main():

    while(True):
        message = input("What would you like the message to say? ").lower()

        if message == "quit":
            break

        for i in range(len(ascii_lowercase)):
            print("Offset: {}, Rotated: {}".format(i, rotate_lowercase_text(message, i)))

if __name__ == '__main__':
    main()

This uses string.ascii_lowercase for the set of lowercase letters and a positional approach for rotating a letter a given approach. The methods could be extended using default parameters to allow for other characters sets, but that is left as an exercise to the reader.

Furthermore I've presented two functions here one for rotating the entire string, and one for rotating a single character. These could have been combined, but I like the single responsibility principle of letting one function doing exactly one thing.

Lastly I've kept the main logic from the original post, only embedded it into a main(), and added some extra information related to the output to display the various rotation offsets used for presenting the text.

For even more various alternative implementation look at other posts here at Code Review tagged caesar-cipher (in a variety of languages). Both shorter and longer and simpler and more complex variation exists.


Copying a lot of code to filter four PHP arrays I have made this search filter and it works fine, but I think the code looks a little messy and I have no idea how to clean it up.

I hope the following makes sense, otherwise please tell me which information you need, and I will provide it ASAP.

// GET THE POST VALUE FROM FORM FIELDS
          $field_value_service =  $_POST['service_filter'];
          $field_value_region  =  $_POST['service_region'];
          $field_value_postal  =  $_POST['postal_code'];
          $field_value_gender  =  $_POST['gender_filter'];

          // GET USER INFO FROM USERS WHERE USERS VALUES IS EQUAL TO FORM FIELD VALUES
          $valuesService = get_cimyFieldValue(false, 'service', $field_value_service);
          $valuesRegion  = get_cimyFieldValue(false, 'region', $field_value_region);
          $valuesPostal  = get_cimyFieldValue(false, 'postnummer', $field_value_postal);
          $valuesGender  = get_cimyFieldValue(false, 'gender', $field_value_gender);

          // CREATE EMPTY ARRAYS
          $userArray = array();
          $userArrayTwo = array();
          $userArrayThree = array();
          $userArrayFour  = array();
          $userArrayFive = array();

          foreach ($valuesService as $value) {
            $userArray[] = $value['user_id'];
          }
          foreach ($valuesRegion as $value) {
            $userArrayTwo[] = $value['user_id'];
          }
          foreach ($valuesPostal as $value) {
            $userArrayThree[] = $value['user_id'];
          }
          foreach ($valuesGender as $value) {
            $userArrayFour[] = $value['user_id'];
          }

          $firstResult = array_merge($userArray, $userArrayTwo);
          $duplicates = array_unique(array_diff_assoc($firstResult, array_unique($firstResult)));
          $secondResult = array_merge($duplicates, $userArrayThree);
          $secondDuplicates = array_unique(array_diff_assoc($secondResult, array_unique($secondResult)));
          $thirdResult = array_merge($secondDuplicates, $userArrayFour);
          $thirdDuplicates = array_unique(array_diff_assoc($thirdResult, array_unique($thirdResult)));

comments should explain what the code does, at least the complicated blocks, such as the array merging, and/or a comment to describe the whole thing
Be consistent with naming things: you use camelCase sometimes (e.g. userArray) and underscores other times (e.g. field_value_gender). IIRC, underscores is the PHP style, so you could go with that, but the important thing is to be consistent.
Same goes for your filter parameter names: service_filter, region_filter, postal_code_filter, gender_filter
When you have places where you repeat code, use a function:

function build_array(post_field, user_field) {
    $post_value = $_POST[post_field];
    $user_value = get_cimyFieldValue(false, user_field, $post_value);
    $arr = array();

    foreach ($user_value as $value) {
        $arr[] = $val['user_id'];
    }
    return $arr;
}

build_array('service_filter', 'service');

But really, you probably don't need that. Are you trying to get a unique list of user IDs? Can you use one array and call array_unique on that? Or, are you trying to get duplicates? Or both? Either way, I really doubt you need all that merge/diff/unique'ing. For example, you could keep an array of user_id => times_seen and then loop through that to get entries with times_seen > 1 or whatever.


Import huge XML data set into MongoDB while parsing data I am currently working with the Stack Exchange Data Dump - to be more precise - with the dumped Posts.xml data set from Stack Overflow.

What am I trying to achieve?
I want to read the whole data set and import each row (a post on Stack Overflow) as a document into an MongoDB database.

What am I doing right now?
I am using the iterparse() function from lxml to iterate over each row, without building a DOM. Every row contains attributes which hold the actual data. As every attribute is a String, I need to parse some attributes into Integers, Dates and Lists. This is done by the attrib_to_dict() function. The resulting Dictionary is simply inserted into the database collection.

What is the problem?
The parsing of the attributes is quite slow. The whole process took about two hours on my machine. By using the multiprocessing module I was able to speed up the process substantially. The iteration over the whole data set without doing anything is quite fast.

# main.py
from lxml import etree as et
from tqdm import tqdm
import multiprocessing as mp
import pymongo

from constants import POSTS_SIZE
from posts import attrib_to_dict

client = pymongo.MongoClient("mongodb://localhost:27017/")

# database
stackoverflow = client["stackoverflow"]
# collection
posts = stackoverflow["posts"]

def work(elem):
    try:
        # turn the String back into an element, pass attributes to parsing function
        posts.insert_one(attrib_to_dict(et.fromstring(elem).attrib))
    except pymongo.errors.DuplicateKeyError:
        # skip element
        pass

if __name__ == "__main__":
    pool = mp.Pool(4)
    # progress bar
    pbar = tqdm(total=POSTS_SIZE)
    def update(*args):
        # add one to total processed elements
        pbar.update(1)
    try:
        for event, elem in et.iterparse("Posts.xml", tag="row"):
            # pass element as a String to the worker
            # passing the attribute object directly did not seem to work
            pool.apply_async(work, args=(et.tostring(elem),), callback=update)
            elem.clear()
        pool.close()
    except KeyboardInterrupt:
        pool.terminate()
    finally:
        pbar.close()
        pool.join()

# posts.py
from datetime import datetime as dt

def attrib_to_dict(attrib):
    result = {}

    result["_id"] = int(attrib.get("Id"))

    result["PostTypeId"] = int(attrib.get("PostTypeId"))

    # nullable attributes
    acceptedAnswerId = attrib.get("AcceptedAnswerId")
    if acceptedAnswerId: result["AcceptedAnswerId"] = int(acceptedAnswerId)

    result["CreationDate"] = dt.fromisoformat(attrib.get("CreationDate"))

    # about 10 more conversions ...

    tags = attrib.get("Tags")
    # "<python><mongodb>" -> ["python", "mongodb"]
    if tags: result["Tags"] = [tag[:-1] for tag in tags.split("<")[1:]]

    return result

Some performance metrics:

no inserts, no parsing, passing None to worker: 13427.88 items/s
no inserts, no parsing, passing et.tostring(elem) to worker: 10177.07 items/s
no inserts, parsing, passing et.tostring(elem) to worker: 9637.41 items/s
inserts, parsing, passing et.tostring(elem) to worker: 7185.15 items/s

Since inserting into the DB takes a non-negligible amount of time, you should try to use insert_many, instead of insert_one. If you used a single thread this would be easy, just chunk your file and insert a whole chunk. Since you are using multiprocessing, this is a bit more complicated, but it should still be doable

(untested code)

from itertools import islice
import pymongo
import multiprocessing as mp
from tqdm import tqdm
import et

def chunks(iterable, n):
    it = iter(iterable)
    while (chunk := tuple(islice(it, n))):  # Python 3.8+
        yield chunk

def work(chunk):
    try:
        posts.insert_many([attrib_to_dict(elem.attrib) for _, elem in chunk],
                          ordered=False)
    except pymongo.errors.BulkWriteError:
        # skip element
        pass

if __name__ == "__main__":
    pool = mp.Pool(4)
    # progress bar
    pbar = tqdm(total=POSTS_SIZE)
    n = 100
    try:
        for chunk in chunks(et.iterparse("Posts.xml", tag="row"), n):
            pool.apply_async(work, args=(chunk,),
                             callback=lambda: pbar.update(len(chunk)))
        pool.close()
    except KeyboardInterrupt:
        pool.terminate()
    finally:
        pbar.close()
        pool.join()

Here I used this to ignore duplicate keys.


Simpler state manament alternative to Redux, MobX, Context API, etc I got frustrated with the jargon and created an alternative state management solution. Its up at https://github.com/smakazmi/react-soliit. It goes something like this

import React from "react";

export default class StateStore<T>  {
  private listeners: Array<Function>;
  constructor(public state: T) {
    this.listeners = new Array<() => void>();
  }

  private fireOnSetState() {
    this.listeners.forEach(l => {
      try {
        l();
      }
      catch { }
    })
  }

  protected setState(newState: Partial<T>) {
    this.state = { ...this.state, ...newState };
    this.fireOnSetState();
  }
  subscribe(callback: () => void) {
    if (this.listeners.indexOf(callback) < 0)
      this.listeners.push(callback);
  }
  unsubscribe(callback: () => void) {
    this.listeners = this.listeners.filter(l => l !== callback);
  }
}

export const withStores = function (WrappedComponent: any, stores: { [index: string]: StateStore<{}> }) {
  return class extends React.Component {
    updateState = () => {
      this.forceUpdate();
    }
    componentDidMount() {
      Object.keys(stores).forEach(k => {
        stores[k].subscribe(this.updateState);
      });
    }
    componentWillUnmount() {
      Object.keys(stores).forEach(k => {
        stores[k].unsubscribe(this.updateState);
      });
    }
    render() {
      return (<WrappedComponent {...this.props} {...stores} />);
    }
  }
}

Can be used as follows

const counterStore = new StateStore({ count: 0 });

const Counter = function({ counterStore }) {
  const increment = () =>
    counterStore.setState({ count: counterStore.state.count + 1 });
  const decrement = () =>
    counterStore.setState({ count: counterStore.state.count - 1 });
  return (

      <button onClick={decrement}>-</button>
      {counterStore.state.count}
      <button onClick={increment}>+</button>

  );
};
const ConnectedCounter = withStores(Counter, { counterStore });
render(<ConnectedCounter />, document.getElementById("root"));

The sample is available at https://stackblitz.com/edit/soliit-simple-counter

Seems to work fine, at the same time seems too good to be true. So looking for reasons to why to go for Redux, MobX or even Unstated and not something as simple as this.

After looking at the implementation, this is not an implementation of global state management. The entire point of things like redux is so you can directly connect components to a global store. In your case, it looks like every instance of StateStore has its own unique data structure, which seems to imply that you are only going to use StateStore once.

You can only import it at the highest level and keep passing it down in order to persist the data. This has very heavy performance implications. Read up on how React decides on whether it should scan an entire component for any changes (rerenders can severely add up) and how the reconciliation algorithms relates to it.

Your implementation is essentially this.state but also handles functions. Technically the same functionality, but it's not a true global state like how the context api or redux works.


Displaying items in a circular queue I am dealing with the following problem:

  Write a method for the Queue class in the queue.java program (Listing 4.4) that displays the contents of the queue.  Note that this does not mean simply displaying the contents of the underlying array.  You should show the queue contents from the first item inserted to the last, without indicating to the viewer whether the sequence is broken by wrapping around the end of the array.  be careful that one item and no items display properly, no matter where front and rear are.

I have managed to solve it, however, I am not sure if my solution is efficient or the best possible solution. I am always willing to learn from my mistakes.  I have posted the code below and in particular please take a look at my display() method and let me know if it's correct and also if I could have written a better solution. I am open to all suggestions and criticism.

public class Queue {

    private int maxSize;
    private int[] queArray;
    private int front;
    private int rear;
    private int nItems;

    public Queue(int s) {
        maxSize = s;
        queArray = new int[maxSize];
        front = 0;
        rear = -1;
        nItems = 0;
    }

    public void insert(int j) {
        if (rear == maxSize - 1)
            rear = -1;

        queArray[++rear] = j;
        nItems++;

    }

    public int remove() {
        int removed = queArray[front++];
        if (front == maxSize)
            front = 0;

        nItems--;

        return removed;
    }

    public int peek() {
        return queArray[front];
    }

    public boolean isEmpty() {
        return (nItems == 0);
    }

    public boolean isFull() {
        return (nItems == maxSize);
    }

    public int size() {
        return nItems;
    }

    public void display() {
        System.out.println("First Inserted Item to Last Inserted Item");

        if (rear < front && (!isEmpty())) {
            for (int i = front; i < maxSize; i++) {
                System.out.println(queArray[i]);
            }

            for (int i = 0; i <= rear; i++) {
                System.out.println(queArray[i]);
            }

        }

        else if (front >= rear && (!isEmpty())) {
            for (int i = front; i <= rear; i++) {
                System.out.println(queArray[i]);
            }
        }

        else {
            System.out.println("Queue is Empty!");
        }

    }

}

I realize that you were only supposed to implement the display method, but, the rest of the queue is code that can be significantly improved too.

Your queue is a fixed-capacity queue. There is nothing wrong with that (except that the array should be declared as final ... ), and, it actually makes some things easier.

Like, one of the most complicated things with a circular buffer/queue is to manage whether the queue is full, or empty. at both times, the rear and front cursor are the same. There is a really nice way to solve that problem, which is to not use a rear pointer at all, and to only use the front, and size.

Now, your code has the front, rear, and size variables, and it sometimes uses the rear variable, and other times uses the size... and this leads to confusing code. The thing is, that you can easily calculate the rear from the front and size... or, alternatively, you can calculate any one of those three variables from the other two. Bottom line, is you only need two of them, and keeping the size makes other things easy.

Here's your code, stripping out the entire 'rear' variable.... Other things I did are:

removed the maxSize variable since the queArray's length is the same.
added validation to the insert/remove methods.
some formatting changes
use modulo % operator to 'wrap' indexes nicely in the new adjust() method.

Notice how the display method is now really simple:

public void display() {
    System.out.println("First Inserted Item to Last Inserted Item");

    if (isEmpty()) {
        System.out.println("Queue is Empty!");
    } else {
        for (int i = 0; i < nItems; i++) {
            System.out.println(queArray[adjust(front + i)]);
        }
    }
}

The full code is:

import java.util.NoSuchElementException;

public class Queue {

    private final int[] queArray;
    private int front;
    private int nItems;

    public Queue(int s) {
        queArray = new int[s];
        front = 0;
        nItems = 0;
    }

    /*
     * Simple method converts an un-wrapped index to a wrapped index.
     */
    private final int adjust(int index) {
        // add the array length to make things like adjust(-1) work.
        return (index + queArray.length) % queArray.length;
    }

    public void insert(int j) {
        if (isFull()) {
            throw new IllegalStateException("Queue is full");
        }
        queArray[adjust(front + nItems)] = j;
        nItems++;
    }

    public int remove() {
        if (isEmpty()) {
            throw new NoSuchElementException("Queue is empty");
        }
        int removed = queArray[front];
        // for an object-based queue, this would
        // be the same as setting it to null
        queArray[front] = 0;
        front = adjust(front + 1);
        nItems--;
        return removed;
    }

    public int peek() {
        if (isEmpty()) {
            throw new NoSuchElementException("Queue is empty");
        }
        return queArray[front];
    }

    public boolean isEmpty() {
        return nItems == 0;
    }

    public boolean isFull() {
        return nItems == queArray.length;
    }

    public int size() {
        return nItems;
    }

    public void display() {
        System.out.println("First Inserted Item to Last Inserted Item");

        if (isEmpty()) {
            System.out.println("Queue is Empty!");
        } else {
            for (int i = 0; i < nItems; i++) {
                System.out.println(queArray[adjust(front + i)]);
            }
        }
    }

}


Install helm chart automatically I use the following code which works ok!
The code is installing helm charts in loop in k8s cluster, and I want to verify that I'm writing it ok.
tmpfile, err := ioutil.TempFile(kp, kcp)
if err != nil {
    log.Error(err, "error")
}

defer os.Remove(tmpfile.Name())

if _, err := tmpfile.Write(cfg); err != nil {
    return err
}
if err := tmpfile.Close(); err != nil {
    return err
}

kcfgFilePath := tmpfile.Name()
settings := cli.New()
ac := new(action.Configuration)
clientGetter := genericclioptions.NewConfigFlags(false)
clientGetter.KubeConfig = &kcfgFilePath

for _, chartInstallation := range charts {
    chart, err := loader.Load(chartInstallation.Path)
    if err != nil {
        return err
    }

    releaseName := releaseName + "-" + chartInstallation.Name
    if err := ac.Init(clientGetter, settings.Namespace(), os.Getenv("HELM_DRIVER"), func(format string, v ...interface{}) {

    }); err != nil {
        return err
    }
    releasePresent := true
    statusAction := action.NewStatus(ac)
    status, err := statusAction.Run(releaseName)
    if err != nil {
        if strings.Contains(err.Error(), driver.ErrReleaseNotFound.Error()) {
            releasePresent = false
        } else {
            return err
        }
    }

    if !releasePresent {
        // install chart
        installAction := action.NewInstall(ac)
        installAction.CreateNamespace = true
        installAction.Namespace = chartInstallation.Namespace
        installAction.ReleaseName = releaseName

        _, err := installAction.Run(chart, nil)
        if err != nil {
            return err
        }
        log.Info("chart installed: ", "releaseName", releaseName)
    }

    if status != nil {
        if releasePresent && status.Info.Status.String() == release.StatusFailed.String() {
            upgradeAction := action.NewUpgrade(ac)
            upgradeAction.Wait = true
            upgradeAction.ReuseValues = false
            upgradeAction.Recreate = false
            _, err := upgradeAction.Run(releaseName, chart, nil)
            if err != nil {
                return err
            }
        }
    }

The error from ioutil.TempFile is unhandled, should also return err since everything after it won't work if the temporary file
couldn't be created.
The strings.Contains check is definitely the least preferred option
to check for the kind of error.  If at all possible avoid it since
it's actually fairly easy for that string to change with, say, a
library upgrade without any notice, which would break the logic here.
Same with status.Info.Status.String() == release.StatusFailed - are
you sure it can't just be compared without the String() conversion?
tmpfile.Name() is used twice, could already be moved to a variable.
In fact, one of tmpfile and kcfgFilePath is redundant, I'd suggest
directly assigning to kcfgFilePath.
IMO new is obsolete and could very well be replaced by a regular
variable declaration.
statusAction could be inlined.

Actually it would be great if the imports were mentioned, that would
give readers a chance to find the actual signatures and such.  Without
that I'll leave it at those points.
Apart from these details it looks okay, though at some point you might
want to refactor things into smaller functions to bring a little order
into it.  Right now it's still a rather long block of code.  E.g. it
might be worth moving the inner part of the loop into a new function.
Or creating functions for each "action" that's currently only marked
with a comment, say runInstall, runUpgrade.  The temporary file bit
at the start could also very well be its own function (keeping the
os.Remove call at this level of course).
Edit: As requested to elaborate on the second point, it's better to do a
direct comparison of error values instead of string comparisons, since
they're just values too, this will be faster and not depend on string
values which might change in unexpected ways, like so:
// this is defined in the helm API
var ErrReleaseNotFound error = errors.New("release: not found")

// ...

// some time later, we get an error
{
    statusAction := action.NewStatus(ac)
    status, err := statusAction.Run(releaseName)
    if err != nil {
        if err == driver.ErrReleaseNotFound {
            releasePresent = false
        } else {
            return err
        }
    }

    // ...
}

But, actually, we should all probably start using
errors.Is so that even nested
errors would be correctly handled:
// ...
if errors.Is(err, ErrReleaseNotFound) {
    releasePresent = false
} else {
    return err
}
// ...


Data model for tracking currency in a safe I am attempting to design an application that tracks currency and recent transactions in a safe.  I am using entity-framework code-first in a wpf desktop application.

The first goal is to be able to track whats currently in the safe. For example - 87 Ones, 55 Twos, 76 fives, etc..

The second goal is to be able to track recent deposits/withdrawals into/out of the safe. For example - user X deposited 5 ones, 10 fives on 1/1/14.

Is the following a reasonable entity framework data model to accomplish this?

class Safe
{
    public int SafeId { get; set; }

    public CurrencySet Totals { get; set; }

    public ICollection<SafeTransaction> SafeTransactions { get; set; }
}

class CurrencySet
{
    public int CurrencySetId { get; set; }

    public int Ones { get; set; }
    public int Twos { get; set; }
    public int Fives { get; set; }
    public int Tens { get; set; }
    public int Twenties { get; set; }
    public int Fifties { get; set; }
    public int Hundreds { get; set; }
}

class SafeTransaction
{
    public int SafeTransActionId { get; set; }

    public TransactionType TransactionType { get; set; }

    public User User { get; set; }

    public CurrencySet Currency { get; set; }

    public DateTime TimeStamp { get; set; }

}

enum TransactionType
{
    Withdrawal,
    Deposit
}

One concern I have is tracking non-US currency. Not a requirement currently, but maybe in the future.

One concern I have is tracking non-US currency. Not a requirement currently, but maybe in the future.

You're right to be concerned about the flexibility of your approach: what would Twos return in a currency that doesn't have toonies?
Taking future considerations into account, I think you need to re-think your model.

CurrencySet

By modeling the CurrencySet entity the way you have, you've pretty much hard-coded the currency paper's nominal values:

class CurrencySet
{
    public int CurrencySetId { get; set; }

    public int Ones { get; set; }
    public int Twos { get; set; }
    public int Fives { get; set; }
    public int Tens { get; set; }
    public int Twenties { get; set; }
    public int Fifties { get; set; }
    public int Hundreds { get; set; }
}

I'd suggest something like this:
public class Currency
{
    public int Id { get; set; }
    public string Code { get; set; }
    public string CultureName { get; set; } // for number formats

    public virtual ICollection<CurrencyExchangeRate> CurrencyExchangeRates { get; set; }
    public virtual ICollection<CurrencyPaper> CurrencyPapers { get; set; }
}

public class CurrencyExchangeRate
{
    public int Id { get; set; }
    public int CurrencyId { get; set; }
    public int ReferenceCurrencyId { get; set; }

    public decimal Rate { get; set; }

    public virtual Currency Currency { get; set; }
    public virtual Currency ReferenceCurrency { get; set; }
}

public class CurrencyPaper
{
    public int Id { get; set; }
    public int CurrencyId { get; set; }

    public decimal NominalValue { get; set; }
    public string SerialNumber { get; set; }
    public string Issuer { get; set; }
    public int YearIssued { get; set; }

    public virtual Currency Currency { get; set; }
}

Now you can detect currency paper with consecutive serial numbers if you want, and your Safe can contain papers in many currencies, and calculate the total value in $USD, CDN$, €EUR or ¥YEN, given the exchange rate for $USD as a reference currency.
Granted, me putting SerialNumber, Issuer and YearIssued in there is pushing it, but it makes a point: this model is much closer to the actual real-world objects - an approach with its own pros and cons.

Safe

Doing this turns your Safe into an actual safe that literally contains money - it also means the calculating the Totals is a method that has nothing to do in the data itself:
public class Safe
{
    int Id { get; set }

    //...

    public virtual ICollection<CurrencyPaper> CurrencyPapers { get; set; }
    public virtual ICollection<TransactionLog> TransactionLogs { get; set; }
}

SafeTransaction

It's really a log entry more than the transaction itself: I think I'd call it TransactionLog though.
Now what your code is doing, is logging the number of individual CurrencyPaper instances that were involved in a transaction. To do this with the approach I'm suggesting, you might want to break the transaction logs in two tables for full details: one with "header", another with "details" (enabling tracking of individual CurrencyPaper instances) - but that's certainly overkill. You could also just track the total amount deposited/withdrawn, given a Currency.

Navigation properties should be virtual
Be consistent about FK properties - your SafeTransaction doesn't have them.
Naming is ok - although I prefer the entity key to be called Id.


Filtering a list of names based on certain conditions I wrote this code to filter a list of names based on certain conditions, and  wanted to see if I can reduce this further and eliminate redundancy in code.

names1 = ["Jane", "Jake", "Bradley", "Bill", "Betty", "Kara", "Kris", "Jil"]
names2 = ["George", "Kate", "Karen", "Kurt", "Greg", "Gary"]
selection_criteria = ["full_list", "four_characters", "three_characters", "start_with_k", "start_with_z"]

def sublist (name_list, condition):
    return_list = []
    if condition == "full_list":
        return name_list
    if condition == "four_characters":
        for name in name_list:
            if len(name) == 4:
                return_list.append(name)
    if condition == "three_characters":
        for name in name_list:
            if len(name) == 3:
                return_list.append(name)
    if condition == "start_with_k":
        for name in name_list:
            if name[0] == 'K':
                return_list.append(name)
    if condition == "start_with_z":
        for name in name_list:
            if name[0] == 'Z':
                return_list.append(name)
    return return_list

for criteria in selection_criteria:
    print(sublist(names1, criteria))

for criteria in selection_criteria:
    print(sublist(names2, criteria))

I would make this two different functions, one for filtering all names of length n and one for all names starting with some letter. I made them into generator comprehensions, so they are more memory efficient on their own (even though here I cast them to lists, as soon as I want to use them).

Then there is a function which is similar to your function, except that it is configurable. By default it just yields the original list. If additional n's or c's are passed, it applies these criteria.

Finally there is a print function, which has your concrete realization of n = 4, 3 and c = "K", "Z".

def n_chars(names, n):
    return (name for name in names if len(name) == n)

def starts_with(names, c):
    return (name for name in names if name.lower().startswith(c))

def print_sublists(names):
    print(names)
    print(list(n_chars(names, 4))
    print(list(n_chars(names, 3))
    print(list(starts_with(names, "k"))
    print(list(starts_with(names, "z"))

if __name__ == '__main__':
    names1 = ["Jane", "Jake", "Bradley", "Bill", "Betty", "Kara", "Kris", "Jil"]
    names2 = ["George", "Kate", "Karen", "Kurt", "Greg", "Gary"]
    for names in names1, names2:
        print_sublists(names)


Return explicitly or implicitly? Consider the following two code examples:

1.

void foo(boolean cake) {
    if (!cake)
        return;

    // TODO some work.
}

2.

void bar(boolean cake) {
    if (cake)
        //TODO some work.
}

Which one is the most readable/ semantic correct? With the first example you avoid additional indentation on the below code.

Should one return right away when there is no cake or not?

It depends on your intent. Your code should use idioms to match what its actually trying to do.

void foo(boolean cake) {
    if (!cake)
        return;

    // TODO some work.
}

Indicates that !cake is a precondition and as such is insignificant to what the function is doing.

void bar(boolean cake) {
    if (cake)
        //TODO some work.
}

Indicates that cake is an important part of what the function is doing.


Trying to convert an extended Array.prototype to a function I got a random image rotator working by using the following script (demo). But I was told that it is a bad practice to extend Array.prototype. And it does. It conflicts with the chained select box unless I modify the selected box script. So I guess it's better to not use Array.prototype.shuffle ,isn't it?

Here's The script:

                Array.prototype.shuffle = function() {
                var s = [];
                while (this.length) s.push(this.splice(Math.random() * this.length, 1));
                while (s.length) this.push(s.pop());
                return this;
            }

            var picData = [
                ['img1','url_1'],
                ['img2','url_2'],
                ['img3','url_3'],

            picO = new Array();
            randIndex = new Array();
            for(i=0; i < picData.length; i++){
                picO[i] = new Image();
                picO[i].src = picData[i][0];
                picO[i].alt = picData[i][1];
                randIndex.push(i);
            }
            randIndex.shuffle();
            window.onload=function(){
                var mainImgs = document.getElementById('carouselh').getElementsByTagName('img');

                for(i=0; i < mainImgs.length; i++){
                    mainImgs[i].src = picO[randIndex[i]].src;
                    mainImgs[i].parentNode.href = picData[randIndex[i]][1];
                    mainImgs[i].alt = picData[randIndex[i]][1];
                }

            }

I'm not sure where to start. I'm reading this article, should I do something like

    Array.shuffle = function() {for(var
a=[];this.length;)a.push(array.splice(Math.random()*array.length,1));
for(;a.length;)this.push(a.pop());return this};

Would anyone please point me in some directions to rewrite that part?

My advice is to use a known shuffle algorithm for this.

This stackoverflow answer describes the Fisher-Yates shuffle and provides a function for you to use.

https://stackoverflow.com/a/2450976/423413

Bottom line use that function and replace your Array.prototype.shuffle function and call it something like shuffleArray

var shuffleArray = function(arr) {
    ...
}

Then replace the line that says randIndex.shuffle() with a call to the new function.

shuffleArray(randIndex);