Vigenere encryption assignment for CS50 C course My third week on cs50 and looking for some feedback on the general tidyness and style of my code. I took a look at other peoples code after I was done with this and I guess I could've implemented some functions(?) of my own but I noticed he used things that I haven't learned yet.
E.g: Vigenere Cipher in C

Please criticize! :)

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

int i, c;
char* key;

int main(int argc, string argv[])
{
    int int_A = (int) 'A';
    int int_a = (int) 'a';

    // validate the command line argument's parameters
    if (argc != 2)
    {
        printf("Please use a single alphabetic command line argument\n");
        return 1;
    }
    else if (argc == 2)
    {
        for (int z = 0, n = strlen(argv[1]); z < n; z++)
        {
            if (isalpha(argv[1][z]) == false)
            {
                printf("Please use a single alphabetic command line argument\n");
                return 1;
            }
        }
    }

    // get users plaintext
    printf("Plaintext: ");
    char* plaintext = get_string();

    // foo bar mitzva
        key = argv[1];
    int key_len = strlen(argv[1]);
    int key_i[key_len];
    for (i = 0; i < key_len; i++)
    {
        if (isupper(key[i]))
        {
            key_i[i] = (argv[1][i] - int_A);
        }
        if (islower(key[i]))
        {
            key_i[i] = (argv[1][i] - int_a);
        }

    // set i back to 0 for the next loop
    i = 0;
    // iterate over characters in plaintext and shift plaintext character by key

    printf("ciphertext: ");
    for (int j = 0, n = strlen(plaintext); j < n; j++)
    {
        int pt = (int) plaintext[j];
        if (isalpha(plaintext[j]))
        {
            if (isupper(plaintext[j]))
            {
                printf("%c", (((pt - int_A) + key_i[i % key_len]) % 26) + int_A);
            }
            if (islower(plaintext[j]))
            {
                printf("%c", (((pt - int_a) + key_i[i % key_len]) % 26) + int_a);
            }
        }
        else
        {
            printf("%c", plaintext[j]);
        }
        if (isalpha(plaintext[j]))
        {
            i += 1;
        }

    }
    printf("\n");
    return 0;
}

I'm a C++ programmer. I've never written a C program before. But most of what I say should be applicable.

Globals

int i, c;
char *key;

There's no reason for these variables to be global. These variables should be declared in the same place they are used.

These variables also have very bad names. What is i? What is c? I'll have to read the rest of the code to find out.

String typedef

int main(int argc, string argv[])

I don't think using string is a good idea. Somewhere in that cs50 header there's probably

typedef char *string;

When you apply const to this typedef, you don't get what you expect. You may expect that const string is the same as const char *, but it isn't. const string is the same as char *const. You should use a pointer to a character when you want a pointer to a character.

Declare variables as late as possible

int int_A = (int) 'A';
int int_a = (int) 'a';

When you're declaring your variables, you should leave it to the last minute. If you don't need the variables yet, don't declare them.

Conditions

if (argc != 2)
{
    // The wrong number of arguments
}
else if (argc == 2)
{
    // The right number of arguments
}

If the condition argc != 2 is false, we know that the condition argc == 2 must be true so there is no need to confirm that it is true. Also, when there is the wrong number of arguments, the function returns. Your if should become this:

if (argc != 2)
{
    // The wrong number of arguments
}
// The right number of arguments

Unhelpful error messages

When the user doesn't provide exactly 1 argument, this happens:

printf("Please use a single alphabetic command line argument\n");
return 1;

When the user provides a single argument that does not consist of only alphabetic characters, this happens:

printf("Please use a single alphabetic command line argument\n");
return 1;

You should print two different error messages for two different problems. Perhaps "Please provide a single argument" and "Please provide an alphabetic string"

Bad variable names

for (int z = 0, n = strlen(argv[1]); z < n; z++)
{
    if (isalpha(argv[1][z]) == false)
    {
        printf("Please use a single alphabetic command line argument\n");
        return 1;
    }
}

Here, you're referring to the key as argv[1]. At this point, it might make more sense to say "the key must be an alphabetic string" rather than "the first command-line argument must be an alphabetic string". You're also calculating the size of the key twice. You're doing it here in the for loop, then right after it

// foo bar mitzva
    key = argv[1];
int key_len = strlen(argv[1]);
int key_i[key_len];

We've found the place where key is used. You should put this snippet before the for loop and have the for loop refer to key rather than argv[1].

Strings are pointers, pointers can move

The part of your code that makes sure the key is an alphabetic string can be made more efficient. Your code finds the length of the key twice and then checks to see if every character is alphabetic. This is a whopping 3 iterations when only one is necessary.

char *key = argv[1];
for (const char *k = key; *k != 0; ++k)
{
    if (!isalpha(*k))
    {
        printf("The cipher key must be an alphabetic string\n");
        return 1;
    }
}

Is this a heap allocation?

// get users plaintext
printf("Plaintext: ");
char* plaintext = get_string();

I'm not sure if get_string allocates memory for the string or returns a pointer to static memory. You might want to specify that in a comment. If plaintext is heap allocated, you should call free(plaintext) to free the memory when you're done with it (at the very end of the program).

Characters are numbers

for (i = 0; i < key_len; i++)
{
    if (isupper(key[i]))
    {
        key_i[i] = (argv[1][i] - int_A);
    }
    if (islower(key[i]))
    {
        key_i[i] = (argv[1][i] - int_a);
    }

We've finally found the place where that global i is used. You should declare variables right when you need them. I'm pretty sure you're missing a } at the end of this for loop.

Here, you're copying the key into an array of ints. There is a bit of duplicate work here. If you know that a character is alphabetic, then it can only be upper case or lower case. If you know the character is upper case, then you know that it cannot be lower case. You could actually do this in the same place that you are checking you're characters are alphabetic. What I'm saying is:

char *key = argv[1];
const int key_len = strlen(key);
//If this was C++, I would definitely perform a heap allocation but
//heap allocation is a pain in C so I'll just use this C99 trick.
int key_i[key_len];
for (int idx = 0; idx != key_len; ++idx)
{
    const char character = key[idx];

    if (islower(character))
    {
        // characters are numbers, remember?
        key_i[idx] = character - 'a';
    }
    else if (isupper(character))
    }
        key_i[idx] = character - 'A';
    }
    else
    {
        //If the character is not lower case, and not upper case, then its not alphabetic
        printf("The cipher key must be an alphabetic string\n");
        return 1;
    }
}

The actual cipher part of the cipher program

Alright, now I'll find out what a Vigenere cipher is!

// set i back to 0 for the next loop
i = 0;
// iterate over characters in plaintext and shift plaintext character by key

printf("ciphertext: ");
for (int j = 0, n = strlen(plaintext); j < n; j++)

Use proper variable names. What is i? What is j? You should rename them to key_idx and plaintext_idx respectivly.

Conditionals (again)

int pt = (int) plaintext[j];
if (isalpha(plaintext[j]))
{
    if (isupper(plaintext[j]))
    {
        printf("%c", (((pt - int_A) + key_i[i % key_len]) % 26) + int_A);
    }
    if (islower(plaintext[j]))
    {
        printf("%c", (((pt - int_a) + key_i[i % key_len]) % 26) + int_a);
    }
}
else
{
    printf("%c", plaintext[j]);
}
if (isalpha(plaintext[j]))
{
    i += 1;
}

pt never changes so it should be declared const. There's no need to repeatedly index the plaintext when you already have pt just lying around, not doing much. Try not to pack a whole load of math into one line. If you know that an alphabetic character is not upper case then it must be lower case. This snippet could be simplified to this:

const int pt = plaintext[plaintext_idx];
const int key_char = key_i[key_idx % key_len];
if (isupper(pt))
{
    // most of those brackets are unnecessary.
    printf("%c", (pt - 'A' + key_char) % 26 + 'A');
    key_idx += 1;
}
else if (islower(pt))
{
    printf("%c", (pt - 'a' + key_char) % 26 + 'a');
    key_idx += 1;
}
else
{
    printf("%c", pt);
}

Characters, aren't they weird

Fun fact about C, characters literals aren't actually chars, they're ints! In a lot of places when C expects a "character", what it actually expects is an int. isupper and islower expect ints. printf("%c", integer) expects an int. Just keep that in mind when handling strings in C.

Some more optimization

This program could probably be made much smaller, faster and neater (key_i can be removed) but it's 22:00 in Australia and I have school tomorrow so that's all for now.


Javascript + Filter object of values I have the object with values. I trying to filter based on values.
var data = {
  "map": {
    "0": ["abcd"],
    "1": ["efgh"],
    "2": ["xyz"],
  }
}

In above object I am trying to filter xyz which are have in array values.
Expecting output:
 { '0': [ 'xyz' ] }

I done the code but I am not sure it's efficient way. Can someone help me this.

const  data = {
  "map": {
    "0": ["abcd"],
    "1": ["efgh"],
    "2": ["xyz"],
    "3": ["abcd", "xyz"],
  }
}

const filtered = Object.keys(data.map)
  .filter((key) => {
    return data.map[key].includes('xyz');
  })
  .reduce((obj, key, cIndex) => {
    obj[cIndex] = data.map[key];
    return obj;
  }, {});

  console.log('filtered', filtered);

Avoid use dictionary when you need Array
I'm not sure why you would prefer to store your Array-like data in a dictionary. I would suggest use a real Array if you need an Array.
Order of Object.keys
You may read more details about the order of Object.keys return values from this post: Does ES6 introduce a well-defined order of enumeration for object properties? The order of Object.keys is defined in ES2020. It may not work as you want in older environments.
Anyway, relay on the order of Object.keys don't seems to be a good idea to me. You may always sort the keys after Object.keys if you want though.
Use Object.assign to convert to / from Array
When you need an Array, use an Array. You can try Object.assign which convert your dictionary into Array and vice versa. (But this only applied to "when you cannot change the interface due to any reason").
function filterData(data, value) {
    const arrayData = Object.assign([], data.map);
    const filteredArrayData = arrayData.filter(list => list.includes(value));
    const result = Object.assign({}, filteredArrayData);
    return result;
}


How to refactor code to abstract data from a complex object into a single object without using foreach statements I am trying to abstract data from a complex and create a EventDto.
And I was able to do it using foreach but the syntax is dreadful.
Is there a better way of writing this code?

 public class EventDtO
 {
    public string Id { get; set; }
    public string Title { get; set; }
    public string CategoryTitle { get; set; }
    public DateTime DateTime { get; set; }
  }

This is the complex object that I am trying to get the data from:

public class RootObject
{
    public List<Event> Events { get; set; }
}

public class Event
{
    public string Id { get; set; }
    public string Title { get; set; }
    public string Description { get; set; }
    public string Link { get; set; }
    public List<Category> Categories { get; set; }
    public List<Geometry> Geometries { get; set; }
}

public class Geometry
{
    public DateTime Date { get; set; }
    public string Type { get; set; }
    public List<object> Coordinates { get; set; }
}

    public class Category
{
    public int Id { get; set; }
    public string Title { get; set; }
}

The mapping relationship I want is

EventDto.Id->Event.Id
EventDto.Title->Event.Title
Event.CategoryTitle->Category.Title
Event.DateTime->Geometry.Date

The Category class will only contain one value, but the geometry.Date can have multiple values.

So the output I want is:

Title           Categories          Date
"Iceberg B42"   Sea and Lake Ice    2020-04-23T14:24:00Z
"Iceberg B42"   Sea and Lake Ice    2017-09-15T00:00:00Z

I am able to get the correct information if I use the following code:

var Event = new List<EventDTO>();
foreach (var con in content.Events)
{
    var data = new EventDTO
    {
        Title = con.Title,
        Id = con.Id
    };

    foreach (var cat in con.Categories)
    {
        data.CategoriesTitle = cat.Title;
    }

    foreach (var geo in con.Geometries)
    {
        data.DateTime = geo.Date;
        Event.Add(data);
    }
}

An example of the JSON:

   {
        "id": "EONET_2881",
        "title": "Iceberg B42",
             "description": "",
        "categories": [
            {
                "id": 15,
                "title": "Sea and Lake Ice"
            }
        ]
        "geometries": [
            {
                "date": "2017-04-21T00:00:00Z",
                "type": "Point",
                "coordinates": [ -107.19, -74.63 ]
            },
            {
                "date": "2017-09-15T00:00:00Z",
                "type": "Point",
                "coordinates": [ -107.11, -74.08 ]
            }
        ]
    }

First of all, there is a bug in your example. In this loop:

foreach (var geo in con.Geometries)
{
    data.DateTime = geo.Date;
    Event.Add(data);
}

You are just overwriting date in the same object. You should create a new object instead. Your final list will have the same object twice with the same date.

As for better solution for this, you can use linq and create something like this:

IEnumerable<EventDTO> events =
    from e in content.Events
    from c in e.Categories
    from g in e.Geometries
    select new EventDTO { Id = e.Id, Title = e.Title, CategoryTitle = c.Title, DateTime = g.Date };


Parsing a string of the form "key1=value1;key2=value2;" into a Hash I need to parse the following input string into a hash table with the following format:

Input:

'1001=23;1001B=5;1002=0;1003=5;'

Output:

=> {'1001'=>23,'1001B'=>5,'1002'=>0,'1003'=>5}

My solution:

def parser input
  output = {}
  input.split(';').map {|ar| ar.split('=')}.each {|id, c| output[id]=c.to_i}
  output
end
parser '1001=23;1001B=5;1002=0;1003=5;'

Any insight is appreciated.

You can make it a single expression by taking advantage of Hash[[key, value], …] (available since Ruby 1.9).

The function name could probably be improved.

When using String#split, I consider it good practice to limit the number of splits whenever a limit may be applicable.  Here, you wouldn't be able to tolerate a second = sign within each key=value string, so you should use pair.split('=', 2).

def to_hash(str)
  Hash[
    str.split(';').map do |pair|
      k, v = pair.split('=', 2)
      [k, v.to_i]
    end
  ]
end


Rotation of a 2D array The first line will contain numbers m (number of rows), n (number of columns), and b (number of rotations to be performed).

Suppose I have following 4X4 matrix:

1 2 3 4
5 6 7 8
9 10 11 12
13 14 15 16

and b = 1,the output must be:

2 3 4 8
1 7 11 12
5 6 10 16
9 13 14 15

The problem I am facing is 3 test case are timing out and 9 test case successfully passed. Can somebody suggest a way or help me with optimizing my code?

Here is the link to the question.

I need a way to figure out how to minimize b, because some rotations will be useless as it will result in giving us the original array.

import java.io.*;
import java.util.*;
import java.text.*;
import java.math.*;
import java.util.regex.*;
public class So {
    private static BufferedReader br;
    private static int n,m,b;
    private static int [][]arr;
    public static void main(String[] args)throws IOException {
        int i,j;
        br=new BufferedReader(new InputStreamReader(System.in));
        String []str;
        str=br.readLine().split(" ");
        m=Integer.parseInt(str[0]);
        n=Integer.parseInt(str[1]);
        b=Integer.parseInt(str[2]);
        arr=new int[m][n];
        for(i=0;i<m;++i)
        {
            str=br.readLine().split(" ");
            for(j=0;j<n;++j)
             arr[i][j]=Integer.parseInt(str[j]);
        }
        i=0;
        while(i++<b)
         rotate();
        for(i=0;i<m;++i)
        {
            for(j=0;j<n;++j)
             System.out.print(arr[i][j]+" ");
            System.out.println();
        }
    }
    public static void rotate()
    {
        int sr=0,sc=0,r=m,c=n,i,j,prev,res;
        while(sr<=r-2&&sc<=c-2)
        {prev=arr[sr][sc];
        res=arr[sr+1][sc];
        for(i=sr+1;i<=r-2;++i)
        {
            arr[i][sc]=prev;
            prev=res;
            res=arr[i+1][sc];
        }
        for(i=sc;i<=c-2;++i)
        {
            arr[r-1][i]=prev;
            prev=res;
            res=arr[r-1][i+1];
        }
        for(i=r-1;i>=sr+1;--i)
        {
            arr[i][c-1]=prev;
            prev=res;
            res=arr[i-1][c-1];
        }
        for(i=c-1;i>=sc+1;--i)
        {
            arr[sr][i]=prev;
            prev=res;
            res=arr[sr][i-1];
        }
        arr[sr][sc]=prev;
        sr++;sc++;r--;c--;
    }
    }
 }

Reduce the number of rotations

For each "ring" you rotate, the number of rotations you can do before you reach the original position again is: \$2*(rows + columns) - 4\$, where rows and columns are the number of rows and columns for the ring.  So you can reduce the number of rotations like this:

sizeOfRing = 2*(r-sr + c-sc) - 4;
rotationsForThisRing = totalRotations % sizeOfRing;

You will of course need to modify your program to use a variable number of rotations per ring.


Register page for website I've been working on a simple forum website as a toy project, to start learning PHP and SQL. I've implemented all of the functionality I want, but I thought I'd run the code by the Code Review community to see how I could improve it.

<?php
session_start();
class database{
 protected static $connection;
 protected $state=0; //0 is fine, 1 is connection error, 2 is query error

 public function connect(){
  if(isset(self::$connection)){
   return self::$connection;
  }
  else{
   self::$connection=new mysqli('localhost','root','','mydummydatabase');
   if(self::$connection===false){
    $this -> state=1;
    return false;
   }
   return self::$connection;
  }
 }
 public function query($query){
  $connection=$this -> connect();
  $result=$connection -> query($query);
  if($result===false){
   $this -> state=2;
  }
  return $result;
 }
 public function sanitise($input){
  $connection=$this -> connect();
  return "'".$connection -> real_escape_string($input)."'";
 }
 public function get_first_row($result){
  if($result){
   return $result -> fetch_assoc();

  }
  else{
   return false;
  }
 }
 public function state(){
  return $this->state;
 }

}
$db=new database();
$_SESSION['email-exists-in-database']=false;
$_SESSION['something-weird-happened']=false;
if(isset($_POST['email'])){
 $firstname=$db->sanitise($_POST['firstname']);
 $lastname=$db->sanitise($_POST['lastname']);
 $email=$db->sanitise($_POST['email']);
 $hash=password_hash($_POST['password'],PASSWORD_DEFAULT);
 $result=$db -> query("SELECT `user-id` FROM `users` WHERE email=".$email);
 $record=$db->get_first_row($result);
 if($record!=false){
  $_SESSION['email-exists-in-database']=true;

 }
 else{
  $db -> query("INSERT INTO `users` (firstname,lastname,email,passwordhash) VALUES (".$firstname.",".$lastname.",".$email.",'".$hash."')");

  if(!($db->state())){
   $result=$db -> query("SELECT `user-id` FROM `users` WHERE email=".$email);
   $record=$db->get_first_row($result);
   $_SESSION['id']=$record['user-id'];
   header('Location: welcome.php');
   exit;
  }
  else{
   $_SESSION['something-weird-happened']=true;

  }
 }
}
?>

<link rel="stylesheet" media="screen" href="styles2.css"/>

<form action="<?php echo $_SERVER['PHP_SELF'];?>" method="post">
<input type="text" maxlength="30" name="firstname" placeholder="First name"class="sign-in-box" required/>
<input type="text" maxlength="60" name="lastname" placeholder="Last name"class="sign-in-box" required/>
<input type="email" maxlength="100" name="email" placeholder="Email"class="sign-in-box" required/>
<input type="password" maxlength="200" name="password" placeholder="Password"class="sign-in-box" required/>

<input type="submit" name="Submit" value="Sign up"class="submit-button"/>
</form>

<?php
if($_SESSION['email-exists-in-database']){
 echo("
We already have an account associated with that email.
");
}
if($_SESSION['something-weird-happened']){
 echo("
Something went wrong, please try again.
");
}?>

This is the Register page for the website. Some issues I'm concerned about are security, and good practice.

Any thoughts?

About your HTML

DOCTYPE

You are missing a DOCTYPE. You probably want to use

<!DOCTYPE html>

title element

The title element is a required element (in the very most cases). It has to be added to the head:

  <title>Register …</title>

alt attribute

The alt attribute is required for img elements.

If you account-logo.png is just decoration, the alt value should be empty:

label elements and placeholder attributes

You shouldn’t use the placeholder attribute to "label" input elements. Add label elements instead.

If you want to use the placeholder attribute, it should show an example of what should be entered, or a description of the expected format. See placeholder definition in HTML 5.1.

Email
<input type="email" maxlength="100" name="email" id="email" placeholder="alice@example.com" class="sign-in-box" required />

Usability: maxlength

Do you really want a max length here?

Because there are certainly names longer than 30/60 chars, and email addresses longer than 100 chars. Those users would have to abbreviate their names or use a different email address.

strong for warnings

You could make use of the strong element for the warnings/errors.

A heading

Your page has no headings. You’ll probably add site-wide stuff (like a header, navigation etc.) that contains headings, but even your form (i.e., this page’s main content) should get a heading.

meta-charset

Not required if you provide it via HTTP headers, but it’s typically a good idea to provide a meta-charset element in the head (as first element), e.g. if you use UTF-8:

  <meta charset="utf-8" />


Min Heap implementation [C++] I've never had my C++ code reviewed before so I'd appreciate any feedback. Although my main focus was not on the implementation of the algorithm itself, any suggestions regarding improving it would be welcome. I'm mostly looking for C++ related feedback though.

#include <iostream>
#include <vector>

using namespace std;

class MinHeap {
public:
  void push(int num) {
    /* add num to end of the heap and rearrange by up-heap */
    heap.push_back(num);
    int ind = heap.size() - 1;

    if (heap.size() > 1) {
      int par = parent_index(ind);

      while (heap[ind] < heap[par]) {
        swap(heap[ind], heap[par]);
        ind = par;
        if (ind == 0) break;
        par = parent_index(ind);
      }
    }
  }

  void pop() {
    /* pop the element at the of the heap and finding a new top
    by replacing with the last element and performing a down-heap */
    if (heap.size() == 1) heap.pop_back();
    else if (heap.size() > 1) {
      swap(heap[0], heap.back());
      heap.pop_back();
      int ind = 0;
      int min_c = get_min_child(ind);

      while (heap[min_c] < heap[ind]) {
        swap(heap[min_c], heap[ind]);
        ind = min_c;
        if (child_count(ind) == 0) break;
        min_c = get_min_child(ind);
      }
    }
  }

  int top() {
    /* return element at the top of the heap (should be the min) */
    return heap.front();
  }

private:
  vector<int> heap;

  int get_min_child(int index) {
    /* find the smaller child of a node given by index */
    if (index < heap.size() - 2) {
      int left = left_child_index(index);
      int right = right_child_index(index);
      return (heap[left] < heap[right]) ? left : right;
    }
    else if (index < heap.size() - 1) {
      return left_child_index(index);
    }
    else throw invalid_argument("node has no children");
  }

  int parent_index(int index) {
    /* return index of parent node */
    if (index > 0) {
      return (index - 1) / 2;
    }
    else throw out_of_range("Trying to access root parent");
  }

  int child_count(int index) {
    /* return the number of children a given node has */
    if (2*index + 2 < heap.size()) return 2;
    else if (2*index + 1 < heap.size()) return 1;
    else return 0;
  }

  int left_child_index(int index) {
    int ind = 2*index + 1;
    if (ind < heap.size()) return ind;
    else throw out_of_range("Index out of range");
  }

  int right_child_index(int index) {
    int ind = 2*index + 2;
    if (ind < heap.size()) return ind;
    else throw out_of_range("Index out of range");
  }
};

Here's my two cents:

Do not using namespace std;.  It causes serious problems and is considered bad practice.  See Why is using namespace std; considered bad practice?.
Consider wrapping your class in a header so that it can be reused.  Use an include guard.
MinHeap is a common name that may cause name clash.  Consider placing it in a namespace.
MinHeap only supports ints.  You can make MinHeap a template.
Right now MinHeap can only be default initialized to empty.  Consider adding a constructor to initialize the MinHeap with a list of values.
Your comments apply to the function, instead of the first line of the function body.  They should really go outside the function.

// this function foos the bar
void foo(int bar)
{
    ...
}

You use int for indexes.  This is not good because int may be too narrow — INT_MAX may be as small as 32767.  Use std::size_t instead.
All of your functions that return an index have the suffix _index, except get_min_child.  I would suggest renaming it to min_child_index to be consistent.
Don't write the whole branch on the same line.  This decreases readability.  Instead of

if (2*index + 2 < heap.size()) return 2;
else if (2*index + 1 < heap.size()) return 1;
else return 0;

Do

if (2*index + 2 < heap.size())
    return 2;
else if (2*index + 1 < heap.size())
    return 1;
else
    return 0;

In left_child_index and right_child_index, ind can be computed in the if statement.  (C++17)
You swap(heap[0], heap.back()) then immediately heap.pop_back().  You are not doing heap sort, you are popping the value.  Just heap[0] = heap.back().
Incidentally, the standard library offers a couple of heap operations, and also priority_queue.  Maybe you can use them to simplify your implementation.


Solution for puzzle 3d x 1d = 2d x 2d in Python I wanted to resolve the puzzle from this link using Python 3. It works and gives me the correct value but I would like to know if it is possible to resolve this problem with a few lines or how I can improve/optimize my code.

import time
start = time.time()
max=0
gx=0
gy=0
gi=0
gj=0

for i in range(999,99,-1):
    for j in range(9,0,-1):
        mul=i*j

        for x in range(99,9,-1):
            for y in range(99,9,-1):
                mul2=x*y

                if mul==mul2:
                    if mul>max:
                        gx=x
                        gy=y
                        gi=i
                        gj=j
                        max=mul

print ("i: " + str(gi))
print ("j: " + str(gj))
print ("x: " + str(gx))
print ("y: " + str(gy))
print ("max: " + str(max))
end = time.time()
print (end - start)

And this is the result:

i: 992
j: 9
x: 96
y: 93
max: 8928
17.678768157958984

Style

Python has a style guide called PEP8. It is definitly worth a read. Among the few things you could do to make your code more pythonic : spacing around operators. Also, you should try to avoid giving your variables the same name as builtins. Furthermore, you can assign all variables in one go if it corresponds to a single logical operation (as in "saving the best result found so far"). Finally, you can remove explicit conversion when printing :

import time
start = time.time()
maxi, gx, gy, gi, gj = 0, 0, 0, 0, 0

for i in range(999, 99, -1):
    for j in range(9, 0, -1):
        mul = i * j

        for x in range(99, 9, -1):
            for y in range(99, 9, -1):
                mul2 = x*y

                if mul == mul2:
                    if mul > maxi:
                        maxi, gx, gy, gi, gj = mul, x, y, i, j

print("i: ", gi)
print("j: ", gj)
print("x: ", gx)
print("y: ", gy)
print("max: ", maxi)
end = time.time()
print(end - start

Algorithm

For each i, j, it only makes sense to check if it can be written under the form _ _ * _ _ if it is bigger than the current best value.

for i in range(999, 99, -1):
    for j in range(9, 0, -1):
        mul = i * j

        if mul > maxi:
            for x in range(99, 9, -1):
                for y in range(99, 9, -1):
                    mul2 = x*y
                    if mul == mul2:
                            maxi, gx, gy, gi, gj = mul, x, y, i, j

This makes your code go roughly 500 times faster.

From this point, it is hard to make things actually faster but let's try.

Once you have mul, to find 2 divisors under of 2 digits, you don't need have two loops : one is enough and then you check if the number actually divides mul and if the result has 2 digits.

    if mul > maxi:
        for x in range(99, 9, -1):
            y, r = divmod(mul, x)
            if r == 0 and 10 <= y < 100:
                axi, gx, gy, gi, gj = mul, x, y, i, j

Once you have it, you can break because it will only find the same solution.

    if mul > maxi:
        for x in range(99, 9, -1):
            y, r = divmod(mul, x)
            if r == 0 and 10 <= y < 100:
                maxi, gx, gy, gi, gj = mul, x, y, i, j
                break

Similarly, you can break out of the j loop when mul <= maxi because mul goes decreasing.

for i in range(999, 99, -1):
    for j in range(9, 0, -1):
        mul = i * j

        if mul <= maxi:
            break
        else:
            for x in range(99, 9, -1):
                y, r = divmod(mul, x)
                if r == 0 and 10 <= y < 100:
                    maxi, gx, gy, gi, gj = mul, x, y, i, j
                    break

Also, in the x loop, it doesn't make sense anymore to do backward.

for i in range(999, 99, -1):
    for j in range(9, 0, -1):
        mul = i * j
        if mul <= maxi:
            break
        else:
            for x in range(10, 100):
                y, r = divmod(mul, x)
                if r == 0 and 10 <= y < 100:
                    maxi, gx, gy, gi, gj = mul, x, y, i, j
                    print(maxi)
                    break

This seems to be roughly 10 000 times faster than it used to be.


Unit Testing Datalayer written in ADO.net I have datalayer code as below which is implementing an interface as:
Note: I have hand written this code on Notepad so it may contain some typo errors, which you can gracefully ignore.  I am searching for an approach here.

public interface ISimpleInterface{
    void DosSomething(long someValue);
}

public class ConcreteImplementation: ISimpleInterface
{
    public void DosSomething(long someValue){
        //add code for creating new command, connection objects here
        using(var connection=new NpgsqlConnection(ConnectionString)){
            connection.Open();
            long ID=GetNextValueFromSeq1(connection);
            using(var command=new NpgsqlCommand()){
                command.Connection=connection;
                command.CommandText="Insert INTO testTable(col1, col2) VALUES (:pcol1, :pcol2)";
                command.Parameters.AddRange(
                    new NpgsqlParameters{ ParameterName="pcol1", ParameterType=NpgsqlDbType.BigInt, Value=ID},
                    new NpgsqlParameters{ ParameterName="pcol1", ParameterType=NpgsqlDbType.BigInt, Value=someValue}
                );
            }
        }
    }

    long GetNextValueFromSeq1(NpgsqlConnection connection){
        //code to get next value from Sequence1 goes here
    }
}

A typical Unit Test class might be:

public class ConcreteImplementationTests{

    public void DoSomething_SimpleInsert_InsertsRecord(long someValue){
        //code for deleting or truncating table  testTable goes here

        var sut=new ConcreteImplementation();

        sut.DoSomething(13);

        //write a select command to check whether a record has been inserted in testTable

        //If there is one record in table then test passes, else it fails.
    }
}

Note: My concerns are that how I can assert that Dosomething method correctly takes value from sequence and inserts in table, because in real life, a developer might even forget to get value from GetNextValueFromSeq1 method.  He/she might not know this thing unless/until at a later stage when running the application they get a run time exception.

I am also open to any suggestions in improving my code so as long as it provides greater testability, test maintainability.

In real life, the DosSomething method might even have a list of long values.

I think this ID should be generated at database level (autoincrement). But let's ignore this, for testing you have to make sure your methods do not have a dependency on eachother.

It would be better if you write it like this

public void DoSomething(long someValue, long id)
{
     // Insert new value
}

public long GetInsertId()
{
     // Return ID
}

Now you can test these methods independently

public void GetInsert_ShouldReturnNewId()
{
     // Your requirements for a new ID should be tested here
}

public void DoSomething_ShouldInsertNewRecord()
{
    var sut=new ConcreteImplementation();
    var id = sut.GetInsertId();

    sut.DoSomething(13, id);
}

By using this approach the developer MUST provide an ID. The developer can obtain an ID by calling your new method. Just keep in mind that if you want to make your code more testable you have to make your methods independant. They must work on there own.


MVC refactoring parameter check I had this code repeated in many Actions:

    public ActionResult History(int? patientId)
    {
        if (patientId == null)
        {
            return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
        }

        var patient = patientService.GetPatient(patientId.Value);

        if (patient == null)
        {
            return HttpNotFound();
        }
       ...
    }

So I extracted a method to:

    private ActionResult CheckPatientId(int? patientId, ref Patient patient)
    {
        if (patientId == null)
        {
            return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
        }

        patient = patientService.GetPatient(patientId.Value);

        if (patient == null || patient.PharmacyId != User.PharmacyId())
        {
            return HttpNotFound();
        }

        return null;
    }

Which I call like so:

 var patient = new Patient();
            if (CheckPatientId(patientId, ref patient) != null)
            {
                return CheckPatientId(patientId, ref patient);
            }

I don't like that I have to call CheckPatient twice if it fails and the returning of null - is there a better way to do this or is it ok?

The primary intent of your function is to get Patient instance. Secondary intent is to notify of the error. You should always attempt to translate the intent to code and function names. So, your function should return a Patient instance (happy path), or an ActionResult as an out parameter in case something goes wrong. Usually an out keyword is used when the variable is initialized inside of the function, not the ref keyword. This is done mainly to make your intent more declarative and obvious.

private Patient GetPatient(int? patientId, out ActionResult errorResult)
{
    errorResult = null;
    var patient = null;
    if (patientId == null)
    {
        errorResult = HttpStatusCodeResult(HttpStatusCode.BadRequest);
    }
    else
    {
        patient = patientService.GetPatient(patientId.Value);

        if (patient == null || patient.PharmacyId != User.PharmacyId())
        {
            errorResult = HttpNotFound();
        }
    }

    return patient;
}

and the call would look like:

 ActionResult errorResult;
 var patient = GetPatient(patientId, out errorResult);
 if (errorResult!= null)
 {
     return errorResult;
 }
//do the rest of the Patient processing


Object with highest count in a Groovy list I have a list of Parcel objects and am trying to write a closure that picks the parcel object with highest count:

class Parcel {
int id,
int count,
String sender,
String recipient
Parcel(int _id, int _count) {
 id = _id
 count = _count
 }
}

def parcels = [new parcel(1,5), new parcel(2,1), new parcel(3,3), new parcel(4,2), new parcel(5,4) ]

I have tried the below code which works fine:

parcels.sort{it.count}
parcels.reverse().first()

Is there a better way to pick the parcel with the highest count?

You could obviously try parcels.max{it.count}.

max and other useful operations on Groovy Collections are explained in the following article:
https://groovy.codeplex.com/wikipage?title=Collections


Python internet speed testing I understand there are a few similar tools available for testing speed but that's not what I'm looking for. I'm looking at seeing how long it takes to request a site's HTML. However, I'm getting slightly odd results.

The strange results are incredibly slow internet speeds so I'm confused about whether I'm doing something wrong or something isn't working.

It's reading at roughly 30 kilobytes a second which is quite odd.

Important Info

OS X (would like it to work the same on Windows as well)
Running using Sublime build, have also tried Python Launcher, but appears to be the same.

def getInternetSpeed():
    path = sys.argv[0]
    path = list(path)
    while True: # Just cuts back from say C:\User\Example\File.txt to
        if path[len(path)-1] == chr(92) or path[len(path)-1] == '/':
            path = ('').join(path)
            break
        else:
            path.pop(len(path)-1)
    testInternet();
    start = str(datetime.datetime.now())
    x = urllib2.urlopen('https://google.com')
    end = str(datetime.datetime.now())
    x = x.read()
    start = list(start)
    end = list(end)
    for i in range(17):
        start.pop(0)
        end.pop(0)
    start = ('').join(start)
    end = ('').join(end)
    start = float(start)
    end = float(end)
    diff = end - start
    f = open(path+'TEMP.txt','w')
    f.write(x)
    f.close()
    size = os.path.getsize(path+'TEMP.txt')
    os.remove(path+'TEMP.txt')
    print 'It took ' + str(diff) + ' second(s)'
    print 'to get a file that was ' + str(size / 1000) + ' kilobytes large!'

    scale = 1 / diff
    size = size * scale
    size = size / 1000

    print 'Download speed of ' + str(int(size)) + ' kilobytes a second'

Concept

Internet "speed" actually consists of multiple factors, including bandwidth, latency, and packet loss.  What exactly are you trying to measure?  If it's bandwidth (data transfer speed in bits per second), then you should also measure transfers of larger payloads, because the time to complete small transfers will tend to be dominated by latency effects.  If it's latency (milliseconds for a round trip) that you are want to measure, then a tiny packet (typically an ICMP echo request, commonly called a "ping") would be more appropriate.

In addition, there are confounding issues:

Upload vs. download speed: Residential Internet connections tend to significantly favour downloads, since that's what most consumers care about.
Traffic shaping: Some ISPs will throttle the bandwidth to penalize very large transfers, so that smaller responses feel more responsive.
Server performance: It does take a few milliseconds for Google's server to formulate the response payload; that delay would be a more significant fraction of the measured round-trip time for a short response than for a long response.
HTTP and HTTPS overhead: You're considering only the size of the HTTP response payload.  Since you're using HTTPS, there are also multiple TCP round trips needed to perform the TLS handshake, and a few kilobytes to transfer the certificates.  The HTTP headers could add another kilobyte or so.  All of this is significant when the payload is only ~10 kB short.
DNS overhead: Resolving the google.com hostname to an IP address might involve a DNS lookup, which would add a few milliseconds.  The time for that DNS lookup is accounted for in your benchmark, but not the traffic.
Proximity: The "Internet" is not one entity.  Connectivity to various parts of the Internet will have different performance characteristics.

Time calculation bug

When you do str(datetime.datetime.now()), you are converting the timestamp from a datetime object that is suitable for arithmetic into a string, which is a horrible format for calculations.  You then try to salvage the information by dropping the first 17 characters of the string and converting the seconds portion back into a float.

What happens if the start and end times straddle a minute boundary?  You'll get an elapsed time of approximately -60 seconds!

A better way would be:

start = datetime.datetime.now()
…     # Do some work here…
end = datetime.datetime.now()
diff = (end - start).total_seconds()

Inefficiencies

The code expects a path as sys.argv[0].  What is it for?  It seems to be used only to derive a directory to use for a temporary file, and the temporary file is used just to determine the length of the response payload.

All you had to do to obtain the length was len(urllib2.urlopen('https://google.com')).  No filesystem access is needed!

Lint

By PEP 8, getInternetSpeed() should be get_internet_speed().  The function should probably accept a parameter for the URL to test.  Furthermore, if the function is named "getSomething", I would expect it to return a value rather than print its result.
For filename manipulation, use the os.path module.
Avoid redefining variables, especially in a way that changes their type:

x = urllib2.urlopen('https://google.com')
…
x = x.read()

x is a very poor variable name, by the way.  I suggest req.
open() should almost always be used in the context of a with block, so that the closure will be handled correctly for you:

 with open(os.path.join(temp_dir, 'TEMP.txt'), 'w') as f:
     f.write(x)

To generate temporary files, use the tempfile library to guarantee that you will not overwrite an existing file.


First-time F#: A simple Rock Paper Scissors This is my first F# program. It's a simple Rock-Paper-Scissors implementation.
I am looking for feedback of all sorts, e.g. the choice of types (e.g. List vs Array), whether there are parts that could be solved more elegantly / idiomatic, formatting, etc.
open System

let allowedActions = [ "r"; "p"; "s" ]

let randomItem list (rng: Random) =
    List.item (rng.Next(List.length list)) list

let computerAction () = randomItem allowedActions (Random())
let rec playerAction () =
    printfn "Write 'r' for Rock, 'p' for Paper, 's' for Scissors. Type 'q' to Quit."

    let input = Console.ReadLine()

    let isInputAllowed = List.contains input allowedActions || input = "q"
    match isInputAllowed with
    | true -> input
    | _ -> printfn "invalid input '%s'" input; playerAction()

let computerWin playerAction computerAction =
    printfn "The computer did win. '%s' beats '%s'." computerAction playerAction
let playerWin playerAction computerAction =
    printfn "The player did win. '%s' beats '%s'." playerAction computerAction
let tie anAction = printfn "It's a tie. '%s'" anAction

let ruling playerAction computerAction =
    match (playerAction, computerAction) with
    | ("r", "p")
    | ("p", "s")
    | ("s", "r") -> computerWin playerAction computerAction
    | ("r", "s")
    | ("p", "r")
    | ("s", "p") -> playerWin playerAction computerAction
    | (_, _) -> tie playerAction

let playGame playerAction computerAction =
    printfn "You've chosen '%s'." playerAction
    printfn "The computer chose '%s'." computerAction
    ruling playerAction computerAction
    ()

let game () =
    let playerAction = playerAction ()
    let computerAction = computerAction ()
    match playerAction with
    | "q" -> false
    | _ ->
        playGame playerAction computerAction
        true

[<EntryPoint>]
let rec main arg =
    printfn "Hello World to Rock Paper Scissors"
    match game () with
    | true -> main arg
    | false -> 0
```

As a first attempt it looks in many ways ok.
I have three "complaints":
a) You rely too much on string literals as identifiers for input. That is considered bad practice in - I think - every programming language and should be avoided because it's error prone (typos and illegal input etc.) hence not very robust.
Instead of "r", "p" and "s" you could use a discriminated union - which is a very important part of the F#-toolbox to be familiar with:
type Weapon =
    | Rock
    | Paper
    | Scissors

This will for instance make the match-statement in ruling much more readable and type safe:
let ruling playerWeapon computerWeapon =
    match (playerWeapon, computerWeapon) with
    | (Rock, Paper)
    | (Paper, Scissors)
    | (Scissors, Rock) -> computerWin playerWeapon computerWeapon
    | (Rock, Scissors)
    | (Paper, Rock)
    | (Scissors, Paper) -> playerWin playerWeapon computerWeapon
    | (_, _) -> tie playerWeapon

b) Don't repeat yourself. In the functions where you print the results, you do the same thing, and therefore you should extract the common parts to a generalized function, so that:
let computerWin playerAction computerAction =
    printfn "The computer did win. '%s' beats '%s'." computerAction playerAction
let playerWin playerAction computerAction =
    printfn "The player did win. '%s' beats '%s'." playerAction computerAction

changes to:
let showWinner name winnerWeapon looserWeapon =
    printfn "The %s did win. '%A' beats '%A'." name winnerWeapon looserWeapon
let computerWin playerWeapon computerWeapon = showWinner "computer" computerWeapon playerWeapon
let playerWin playerWeapon computerWeapon = showWinner "player" playerWeapon computerWeapon

c) You recursively call the main function. I don't know if you actually violate any formal or informal rules but it just looks "ugly" to me. I would make a dedicated function that runs the game recursively.

FYI - I have below refactored your program while incorporating my suggestions above:
// HH: Instead of string literals you should use discriminated unions as identifiers for "weapons"
//     This is much more robust in respect to typos etc.
type Weapon =
    | Rock
    | Paper
    | Scissors
    static member Abbreviation w = (w.ToString().[0]).ToString().ToLower()
    static member ToWeapon ch =
        match ch with
        | "r" -> Rock
        | "p" -> Paper
        | "s" -> Scissors
        | _ -> failwith "Invalid Weapon char"

let weapons = [ Rock; Paper; Scissors ]

// HH: You should only instantiate a single random object - used throughout the session.
let rand = new Random()
let computerAction () = weapons.[rand.Next(weapons.Length)]

// HH: This now returns an optional value of None if the user wants to quit
//     or Some (Weapon) if a valid weapon is chosen
let rec playerAction () =
    let allowedActions = weapons |> List.map Weapon.Abbreviation
    let choices = weapons |> List.map (fun w -> sprintf "'%s' = %A" (Weapon.Abbreviation w) w)
    printfn "Enter:\n%s.\n'q' to Quit." (String.Join("\n", choices))
    let input = Console.ReadLine()

    let validWeapon w = List.contains w allowedActions
    match input with
    | "q" -> None
    | w when validWeapon w -> Some (Weapon.ToWeapon w)
    | _ -> printfn "invalid input '%s'" input; playerAction()

//HH: Never repeat yourself: extract a function to print the winner...
let showWinner name winnerWeapon looserWeapon =
    printfn "The %s did win. '%A' beats '%A'." name winnerWeapon looserWeapon
// HH: ... and call that from the dedicated winner functions
let computerWin playerWeapon computerWeapon = showWinner "computer" computerWeapon playerWeapon
let playerWin playerWeapon computerWeapon = showWinner "player" playerWeapon computerWeapon
let tie anAction = printfn "It's a tie. '%A'" anAction

let ruling playerWeapon computerWeapon =
    // HH: By using discriminated unions this match
    //     expression is much more readble and robust
    match (playerWeapon, computerWeapon) with
    | (Rock, Paper)
    | (Paper, Scissors)
    | (Scissors, Rock) -> computerWin playerWeapon computerWeapon
    | (Rock, Scissors)
    | (Paper, Rock)
    | (Scissors, Paper) -> playerWin playerWeapon computerWeapon
    | (_, _) -> tie playerWeapon

let playGame playerWeapon computerWeapon =
    printfn "You've chosen '%A'." playerWeapon
    printfn "The computer chose '%A'." computerWeapon
    ruling playerWeapon computerWeapon
    ()

let runGame () =
    let playerAction = playerAction ()
    match playerAction with
    | Some playerWeapon ->
        let computerWeapon = computerAction ()
        playGame playerWeapon computerWeapon
        true
    | None -> false

// HH: Personally I don't like,that you call main recursively.
//     You probably don't violate any formal or informal rule, but it just look wrong to me
//     So make a dedicated function to start the game by
let rec play () =
    match runGame() with
    | true -> play()
    | false -> ()

I have changed some of the names so that they matches my naming. For the example I show that you can extent a discriminated union with members (static and instance). You could chose to have these methods as normal function instead.


Calculate the number of days passed from a given year I have this code for calculating the number of days passed from a given year:

for (int currentYear = localDate.getYear() - 1; currentYear >= STARTING_YEAR; currentYear--){
            LocalDate date = LocalDate.of(currentYear,5,5); //an arbitrary day of year
            jdn += date.lengthOfYear();
        }
jdn += localDate.getDayOfYear();

This code is working as expected.

I am not comfortable with choosing an arbitrary day to only get the length of the year. Is there any better approach?

Your code scales poorly if there are many years between STARTING_YEAR and localDate.

Worse, if localDate occurs before the STARTING_YEAR, then your code behaves as if STARTING_YEAR is the same year as localDate, which is weird behavior.

To calculate the number of days between two dates, use DAYS.between().

import java.time.LocalDate;
import static java.time.temporal.ChronoUnit.DAYS;

…
long jdn = 1 + DAYS.between(LocalDate.ofYearDay(STARTING_YEAR, 1), localDate);

The 1 + is necessary because the convention when subtracting dates is inclusive-exclusive, whereas your code uses an inclusive-inclusive range.


Aho-Corasick string matching in Haskell I implemented Aho-Corasick string matching in Haskell. It is a purely functional implementation compared to many other implementations.

The input to build an automata is [([a],b)]. which is a list of (pattern,output) pairs.
b has to form a monoid, so we know what to do when more than one match occurs at the same place.

I borrowed many idea from the KMP implementation by Twan van Laarhoven(which I suspect is a implementation of the MP instead of KMP algorithm).

import Data.List
import Data.Maybe
import Data.Monoid
import Data.Function
import Control.Arrow
data Automaton a b = Node {delta  :: a -> Automaton a b,
                           output :: b
                         }
-- assume the input function eq is a equivalence relation, then this
-- produce all the equivalent classes.
equivalentClasses :: (a->a->Bool)->[a]->[[a]]
equivalentClasses eq = foldl parts []
  where parts [] a = [[a]]
        parts (x:xs) a
         | eq (head x) a = (a:x):xs
         | otherwise     = x:parts xs a

buildAutomaton :: (Monoid b,Eq a) => [([a],b)] -> Automaton a b
buildAutomaton xs = automaton
  where automaton = build (const automaton) xs mempty

-- this builds one node of the automaton. Tying the knot happens everywhere
build :: (Monoid b,Eq a)=> (a -> Automaton a b) -> [([a],b)] -> b -> Automaton a b
build trans' xs out = node
  where node  = Node trans out
        trans a
          | isNothing next = trans' a
          | otherwise      = fromJust next
          where next = lookup a table
        table =  map transPair $ equivalentClasses (on (==) (head . fst)) xs
        transPair xs = (a, build (delta (trans' a)) ys out)
         where a  = head $ fst $ head xs
               (ys,zs) = partition (not . null . fst) $ map (first tail) xs
               out = mappend (mconcat $ map snd zs) (output $ trans' a)

-- this function simulates the automaton.
match :: Eq a => Automaton a b -> [a] -> [b]
match a xs = map output $ scanl delta a xs

-- an example to check if a list is a sublist of another list
isInfixOf' :: Eq a => [a] -> [a] -> Bool
isInfixOf' xs ys = getAll $ mconcat $ match (buildAutomaton [(xs, All True)]) ys

It would be nice if the code was ordered for exposed functions versus internal helpers. It looks like buildAutomaton is the exposed API along with the Automaton data type. However, buildAutomaton is the one function missing any comments or docs.

trans looks a lot like maybe from the Prelude/Data.Maybe.

Use explicit imports to make clear where your functions are coming from.


Minesweeper implementation Please review my implementation of the Minesweeper game Kata. Any and all feedback will be much appreciated. I would love to hear your feedback on readability, simplicity, performance, and any code smells. I even welcome nitpicking. Overall I want to know is the code clean (I know there is always room for improvement).

The rules of the application can be found here: Minesweeper. I have modified it a bit I'm not reading from a file.

public class MineSweeper {
    public char[][] getSweptMatrix(char[][] matrix) {
        if(matrix == null) throw new IllegalArgumentException();
        buildSweepMatrix(matrix);
        return matrix;
    }

    private void buildSweepMatrix(char[][] matrix) {
        for (int i = 0; i < matrix.length; i++) {
            for (int j = 0; j < matrix[0].length; j++) {
                int count = checkAdjacentsForBombs(matrix, i, j);
                if (matrix[i][j] != '*') matrix[i][j] = (char) (count + '0');
            }
        }
    }

    private int checkAdjacentsForBombs(char[][] matrix, int i, int j) {
        int count = 0;
        count += checkNorthForBomb(matrix, i, j);
        count += checkNorthEastForBomb(matrix, i, j);
        count += checkEastForBomb(matrix, i, j);
        count += checkSouthEastForBomb(matrix, i, j);
        count += checkSouthForBomb(matrix, i, j);
        count += checkSouthWestForBomb(matrix, i, j);
        count += checkWestForBomb(matrix, i, j);
        count += checkNorthWestForBomb(matrix, i, j);
        return count;
    }

    private int checkNorthForBomb(char[][] matrix, int i, int j) {
        return i - 1 >= 0 && matrix[i - 1][j] == '*' ? 1 : 0;
    }

    private int checkNorthEastForBomb(char[][] matrix, int i, int j) {
        return j + 1 < matrix[0].length && i - 1 >= 0 && matrix[i - 1][j + 1] == '*' ? 1 : 0;
    }

    private int checkEastForBomb(char[][] matrix, int i, int j) {
        return j + 1 < matrix[0].length && matrix[i][j + 1] == '*' ? 1 : 0;
    }

    private int checkSouthEastForBomb(char[][] matrix, int i, int j) {
        return j + 1 < matrix[0].length && i + 1 < matrix.length && matrix[i + 1][j + 1] == '*' ? 1 : 0;
    }

    private int checkSouthForBomb(char[][] matrix, int i, int j) {
        return i + 1 < matrix.length && matrix[i + 1][j] == '*' ? 1 : 0;
    }

    private int checkSouthWestForBomb(char[][] matrix, int i, int j) {
        return j - 1 >= 0 && i + 1 < matrix.length && matrix[i + 1][j - 1] == '*' ? 1 : 0;
    }

    private int checkWestForBomb(char[][] matrix, int i, int j) {
        return j - 1 >= 0 && matrix[i][j - 1] == '*' ? 1 : 0;
    }

    private int checkNorthWestForBomb(char[][] matrix, int i, int j) {
        return j - 1 >= 0 && i - 1 >= 0 && matrix[i - 1][j - 1] == '*' ? 1 : 0;
    }

It's mostly minor issues, but since you want nitpicking... here we go:

In your public method you check the sanity of the input for null. Assuming you want to check for sanity, then you should check also that none of the sub-arrays is null, and that they all have the same size. But that becomes a bit tedious to do in a "constructor", so you'd better extract it into a "checkArgument" method.
Stop using i and j with the matrix. Row and Col force you to reason more about their meaning, and will prevent you from silly mistakes where you switch them (though this is my personal preference)
Your method is called "int checkAdjacentsForBombs", but if it's an int it would be better named "int countAdjacentsBombs"
While I see their convenience, I don't like your "checkNorth"... style of returning ints. I'd rather have it being "isCellABomb(char[][], int row, int col) and then you can generalize all methods in a single one and do:

if (isCellABomb(matrix, row-1, col)) count++;

if (isCellABomb(matrix, row-1, col+1)) count++; ....

This will allow you to reuse it also in the buildSweepMatrix method check

You pass the matrix around everywhere, bc it's in the signature of your methods. But those are all private, so you may ease it up a bit by storing the matrix in the class and sharing it, since no one else will access those methods, though it's debatable.
You should really extract your bomb marker ('*') to a final static const BOMB_SYMBOL, to ease readability and maintainability (so if tomorrow you mark bombs by % instead, you need to change in a single place
You first do the "costly" operation of counting the bombs, and then checking if it was needed here:

int count = checkAdjacentsForBombs(matrix, i, j);

if (matrix[i][j] != '*') matrix[i][j] = (char) (count + '0');

Try doing the opposite so you only compute stuff when needed.


Let's "Iterator" through Fibonacci, forever and ever Following-up to my previous question, this class is now based on an iterator. The class, instead of forcing the programmer to use up a bunch of memory just to get each progressive number once, they can now use an iterator. The class itself is also now based on an iterator, which makes my life easier too. No more initFirst()!

Edit note: the main method was there to test, and is there for no other reason.

import java.math.BigInteger;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;

/**
 *
 */
public class InfiniteFibonacci {

    public static void main(String[] args) {
        InfiniteFibonacci i = new InfiniteFibonacci();
        System.out.println(i.getNumberAt(0));
        System.out.println(i.getNumberAt(100));
    }

    /**
     * The default starting amount of numbers in the sequence to be generated.
     */
    public static final int DEFAULT_STARTING_CAP = 64;

    private final Iterator<BigInteger> iterator = new FibonacciIterator();

    private List<SequencePart> parts;

    private int currentSize = 0;

    /**
     * Creates a new InfiniteFibonacci object, with the default
     * amount of numbers.
     */
    public InfiniteFibonacci() {
        this(DEFAULT_STARTING_CAP);
    }

    /**
     * Creates a new InfiniteFibonacci object, with the specified
     * amount of numbers.
     *
     * @param startCap
     *            The amount of numbers to generate. It will be rounded up to
     *            the nearest 16.
     */
    public InfiniteFibonacci(int startCap) {
        parts = new LinkedList<>();
        initTo(startCap);
    }

    private void initTo(int index) {
        for (; currentSize <= index; currentSize += SequencePart.SIZE) {
            SequencePart part = new SequencePart();
            for (int i = 0; i < SequencePart.SIZE; i++) {
                part.values[i] = iterator.next();
            }
            parts.add(part);
        }
    }

    /**
     * Gets the nth number in the sequence, zero-based.
     *
     * @param index
     *            the n as described
     *
     * @return the specified in the sequence, zero-based.
     */
    public BigInteger getNumberAt(int index) {
        if (index > currentSize) {
            initTo(index);
        }
        return parts.get(index / SequencePart.SIZE).values[index
                % SequencePart.SIZE];
    }

    /**
     * Provides a faster Iterator for code that uses only each
     * number once.
     *
     * @return An Iterator that iterates through each of the
     *         numbers.
     */
    public static Iterator<BigInteger> getIterator() {
        return new FibonacciIterator();
    }

    private class SequencePart {

        private static final int SIZE = 16;

        private BigInteger[] values = new BigInteger[SIZE];

        private SequencePart() {

        }

    }

    private static final class FibonacciIterator implements
            Iterator<BigInteger> {

        private BigInteger current = BigInteger.ONE;
        private BigInteger previous = BigInteger.ZERO;

        private FibonacciIterator() {

        }

        @Override
        public boolean hasNext() {
            // Always has next
            return true;
        }

        @Override
        public BigInteger next() {
            BigInteger temp = current;
            current = previous.add(current);
            previous = temp;
            return temp;
        }

    }

}

Concerns:

Was implementing Iterator<BigInteger> a good idea?
Was the changes an improvement, or was it "poison"?

Judging by the title of the question and the main class, I expected to see a simple iterator over Fibonacci numbers to infinity. Instead I see a class with too many responsibilities. It would be better to separate these concerns:

the infinite iterator
one class to get the nth Fibonacci number with caching
a utility class that implements the cache storage from a linked list of segments, the logic of which is encapsulated and hidden from users
the runner class with the main method

The Iterator<BigInteger> is a very good idea. I thought of perhaps providing an Iterator<Integer> too, but then, Fibonacci numbers tend to get very big very fast, so there's probably not much point. So just the Iterator<BigInteger> makes sense.

What you did with the sequence parts is an interesting way to implement a dynamically growing array-like storage without resize operations. As mentioned, this has nothing to do with Fibonacci numbers, and this logic could live in its own utility class. But I think it's probably not worth it. You can use a simple ArrayList instead. When the performance of the ArrayList becomes a real concern, you could try your alternative List implementation.


Simple C++ SDL2 Wrapper for small game I'm in the process of creating a simple SDL2 wrapper for a breakout clone. The real learning objectives for this project are:

Learn how to manage resources properly via wrappers
Learn more about SDL2
I'm building these wrapper as I step through Lazy Foo's tutorial and wanted to get some feedback on how I'm doing, and what other things I could be doing.

Here's my SDL_Surface wrapper so far:

// Surface.h
#pragma once
#include <iostream>
#include <SDL.h>
#include <string>
#include "Window.h"

namespace SDL2 {
    class Surface
    {
    private:
        SDL_Surface* ScreenSurface = nullptr;
        bool IsWindowSurface = false;
        std::string ImagePath = "";

    public:
        // This function creates a Surface object associated with a Window (gets cleaned when Window is destroyed)
        Surface(SDL2::Window& window);
        Surface(std::string path);
        // Move constructor
        Surface(Surface&& source);
        // Disable copy constructor
        Surface(const Surface& source) = delete;
        ~Surface();
        // Disable copy by assignment
        Surface& operator=(const Surface& source) = delete;
        // Move assignment
        Surface& operator=(Surface&& source);
        void FillRect();
        void Update(SDL2::Window& window);
        bool LoadBMP();
        // TODO: Add more parameters at future SDL2 tutorial
        void BlitSurface(SDL2::Surface& destination);
    };
}

// Surface.cpp
#include "Surface.h"

SDL2::Surface::Surface(SDL2::Window& window) :
    ScreenSurface{ SDL_GetWindowSurface(window.GetSDLWindow()) },
    IsWindowSurface{ true }
{
}

SDL2::Surface::Surface(std::string path) :
    ImagePath{ path },
    IsWindowSurface{ false }
{
}

SDL2::Surface::Surface(Surface && source) :
    ScreenSurface{ source.ScreenSurface },
    IsWindowSurface{ source.IsWindowSurface },
    ImagePath{ source.ImagePath }
{
    source.ScreenSurface = nullptr;
    source.IsWindowSurface = false;
    source.ImagePath = "";
}

SDL2::Surface::~Surface()
{
    // If surface is not associated with Window, need free the surface with SDL2 call
    if (!IsWindowSurface) {
        SDL_FreeSurface(ScreenSurface);
    }
}

SDL2::Surface & SDL2::Surface::operator=(Surface && source)
{
    if (&source == this)
        return *this;

    // If surface is not associated with Window, need free the surface with SDL2 call
    if (!IsWindowSurface) {
        SDL_FreeSurface(ScreenSurface);
    }

    ScreenSurface = source.ScreenSurface;
    IsWindowSurface = source.ScreenSurface;
    ImagePath = source.ImagePath;

    // Return source to stable state
    source.ScreenSurface = nullptr;
    source.IsWindowSurface = false;
    source.ImagePath = "";

    return *this;

}

// TODO: Define SDL_Rect parameter and Color parameter (make wrappers for these)
void SDL2::Surface::FillRect()
{
    SDL_FillRect(ScreenSurface, NULL, SDL_MapRGB(ScreenSurface->format, 0x00, 0x00, 0xFF));
}

void SDL2::Surface::Update(SDL2::Window& window)
{
    SDL_UpdateWindowSurface(window.GetSDLWindow());
}

// This function is called after a non-window surface is created.  Check on this return value for success/fail.
bool SDL2::Surface::LoadBMP()
{
    // Load image from path
    ScreenSurface = SDL_LoadBMP(ImagePath.c_str());

    if (!ScreenSurface) {
        std::cout << "Unable to load image " << ImagePath << " SDL Error: " << SDL_GetError() << '\n';
        return false;
    }
    return true;
}

// The source Surface is `this`.  This function applies the image to the destination surface (though not updated on the screen)
void SDL2::Surface::BlitSurface(SDL2::Surface & destination)
{
    // Args: Source, FUTURE, Destination, FUTURE
    SDL_BlitSurface(this->ScreenSurface, NULL, destination.ScreenSurface, NULL);
}

Any pointers would be much appreciated!

Wrapping SDL_Surface like this to get automatic cleanup in the destructor is a good idea. However, the current implementation makes a few assumptions that aren't necessarily correct.

Surface Creation

There are lots of ways to get hold of a surface. Getting it from a Window, loading from a file, manually creating it with SDL_CreateSurface, or using something like SDL_TTF to render a font.

The Surface class shouldn't care about how the surface is created. It only needs to accept a surface, and know whether that surface should be cleaned up on destruction, e.g.:

Surface(SDL_Surface* surface, bool freeOnDestruction);

should replace the current Window and string constructors. The LoadBMP function could be made a free function that takes a string and returns a Surface.

Window

The dependency of Surface on Window seems backwards. Since an SDL_Window owns an SDL_Surface of its own, it would make more sense for the dependency to be the other way around. Thus the Window class could have a GetSurface() function, returning a Surface, and the Update() function would make more sense as UpdateSurface() in the Window class (it doesn't make sense for non-window surfaces anyway).

Interface and Error Checking

BlitSurface is confusing because you have to remember whether to call source.BlitSurface or dest.BlitSurface. As member functions it might be better to have both a BlitFrom(source) and BlitTo(dest), that both call a single Blit function.

It might be better to use free functions instead:

void FillSurface(Surface& surface, Color const& color); // fill entire surface!
void FillSurface(Surface& surface, Rect const& rect, Color const& color); // fill a rect on the surface
void BlitSurface(Surface& source, Surface const& dest); // blit whole surfaces
// TODO: rect versions...

SDL_BlitSurface has some conditions you should check before and after (it doesn't work on locked / null surfaces, it returns an error code on failure). These should be handled and output a message / throw an error / terminate the program.

It would make sense to have a GetSDLSurface() function in the Surface class (similar to the GetSDLWindow() function). We can check (assert / throw / print message) that the surface pointer is non-null in this function before returning it. If the Blit and Fill functions are free functions, and not members, they can't access the private member directly and must use this function. This means they will automatically do the non-null error check when using the surface.

Using the Standard Library

The C++ Standard Library already contains a class to manage an object's lifetime like this: std::unique_ptr.

Instead of checking whether we have to free the surface in copy / move constructors, we can wrap it in a unique_ptr with a custom deleter. We can then use the default move constructor and move operators:

#include <iostream>
#include <functional>
#include <memory>
#include <cassert>

// placeholder for testing...
struct SDL_Surface
{
    SDL_Surface() { std::cout << "constructor" << std::endl; }
    ~SDL_Surface() { std::cout << "destructor" << std::endl; }
};

// placeholder for testing...
void SDL_FreeSurface(SDL_Surface* surface)
{
    delete surface;
}

class Surface
{
public:

    Surface(SDL_Surface* sdlSurface, bool freeOnDestruction);

    Surface(Surface&&) = default;
    Surface& operator=(Surface&&) = default;

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

    SDL_Surface* GetSDLSurface() const; // Blit / Fill etc. should use this to get the SDL_Surface.

private:

    using SDLSurfacePtrDeleterT = std::function<void(SDL_Surface*)>;
    using SDLSurfacePtrT = std::unique_ptr<SDL_Surface, SDLSurfacePtrDeleterT>;
    SDLSurfacePtrT m_sdlSurfacePtr;
};

Surface::Surface(SDL_Surface* sdlSurface, bool freeOnDestruction)
{
    assert(sdlSurface != nullptr); // no null surfaces!
    // (means we don't have to check the inner pointer every time we e.g. blit).

    auto deleter = freeOnDestruction ?
        SDLSurfacePtrDeleterT([] (SDL_Surface* surface) { SDL_FreeSurface(surface); }) :
        SDLSurfacePtrDeleterT([] (SDL_Surface*) { });

    m_sdlSurfacePtr = SDLSurfacePtrT(sdlSurface, deleter);
}

SDL_Surface* Surface::GetSDLSurface() const
{
    assert(m_sdlSurfacePtr != nullptr); // check this surface hasn't been moved from
    // (if the m_sdlSurfacePtr is valid, the inner surface should be valid (or at least non-null) due to the assert in the constructor).

    return m_sdlSurfacePtr.get();
}

int main()
{
    std::cout << "Construct + Destruct:" << std::endl;

    {
        auto sdlSurface = new SDL_Surface();
        auto surface = Surface(sdlSurface, true);
        // should get destructor call...
    }

    std::cout << "Construct + Leak:" << std::endl;

    {
        auto sdlSurface = new SDL_Surface();
        auto surface = Surface(sdlSurface, false);
        // will not get destructor call...
    }

    std::cout << "Construct + Move + Destruct:" << std::endl;

    {
        auto sdlSurface = new SDL_Surface();
        auto surface = Surface(sdlSurface, true);
        auto surface2 = std::move(surface);
        // should get destructor call...
    }
}


Forum "read/unread" in JavaScript I've seen plenty of examples and questions on how to do this, but the issue is that they all rely on a massive database on the server-side. I figured I could try to move that to the client-side. Here's what I have:

(function() {
var data, pos, read, write, fcnt, tcnt, i, forums, threads, l, fid, id, t, allunread, post;
data = {forum:{},thread:{}};
if( localStorage[USERID+'.forumhistory']) {
         data = JSON.parse(localStorage[USERID+'.forumhistory']);
}
switch(FORUM_READ.mode) {
     // in the initialisation, FORUM_READ is an object with:
     //   mode: "index" for the list of forums, "forum" for an individual forum
     //         "thread" for an individual thread.
case "forum":
    threads = document.querySelectorAll('.f_thread');
              // each thread element has a "data-id" attribute indicating the thread's ID
              // and a "data-ts" attribute indicating the timestamp of the most recent post
    l = threads.length;
    fid = FORUM_READ.id;
    for( i=0; i<l; i++) {
        id = threads[i].getAttribute("data-id");
        data.thread[fid] = data.thread[fid] || {};
        data.thread[fid][id] = data.thread[fid][id] || data.forum[fid] || DATE;
        if( threads[i].getAttribute("data-ts")*1000 > data.thread[fid][id]) {
            threads[i].cells[0].children[0].src = "/img/misc/f_unread.gif";
        }
        else delete data.thread[fid][id];
    }
    data.forum[fid] = DATE;
    allunread = true;
    for( i in data.thread[fid]) {
        allunread = false;
        break;
    }
    if( allunread) {
        delete data.thread[fid];
    }
    // Fall through to deal with subforums
case "index":
    forums = document.querySelectorAll('.f_forum');
              // similarly to threads, forums have "data-id" and "data-ts" attributes
    l = forums.length;
    for( i=0; i<l; i++) {
        id = forums[i].getAttribute("data-id");
        data.forum[id] = data.forum[id] || DATE;
        if( forums[i].getAttribute("data-ts")*1000 > data.forum[id])
                           forums[i].cells[0].children[0].src = "/img/misc/f_unread.gif";
    }
    break;
case "thread":
              // the last post, #f_post, has a "data-ts" attribute giving its timestamp
    post = document.getElementById('f_post').getAttribute("data-ts")*1000;
    fid = FORUM_READ.forum;
    id = FORUM_READ.id;
    if( data.thread[fid] && data.thread[fid][id] && data.thread[fid][id] < post) {
        data.thread[fid][id] = post;
    }
    break;
}
localStorage[USERID+'.forumhistory'] = JSON.stringify(data);
})();

Now, as far as I can tell this works. However, I haven't really been able to test much due to not having anyone else who can test for me (sometimes working alone sucks...). So I'm hoping that someone here will be able to spot any errors in my logic.

No big issues that I can see, but here are a few tips.

When using for-in loops, add a hasOwnProperty check, like this:

for (var i in obj) {
    if (obj.hasOwnProperty(i) {
        // do stuff
    }
 }

Otherwise the loop will also iterate over properties of Object.prototype.

data can more simply and efficiently be declared like this:

data = localStorage[USERID + '.forumhistory'] ?
    JSON.parse(localStorage[USERID+'.forumhistory']) :
    {forum:{},thread:{}};

The switch statement might be more legibly written as a series of if else-if statements (with nesting), but that's mostly a stylistic choice.

data is, actually, the worst variable name there is. It is impossible to create a less specific name. I would suggest renaming it.

In loops where you iterate over an array, like this:

var i, l = forums.length;
for (i = 0; i < l; i++) {
    // use `forums[i]`
}

If you know every element in forums is truthy (not 0, null, undefined, false, or ""), you can use this loop, which is shorter and more efficient, and in my opinion more legible.

var i, forum;
for (i = 0; forum = forums[i]; i++) {
    // use `forum`
}


Better yielding tuple-d union case values in list generation I have a situation where I'm stitching three different unions into a triplet tuple for building a map/dictionary of strongly typed values keyed on a string.

One of the union cases is very large, the second has three options, and the third has only two. Right now I loop through the large with reflection, and have six yield statements to handle the combinations of the second and third unions.

My code works fine as far as I can tell so far, but I don't quite like having yield in my code six times. How can I better represent this situation?

*If needed I can post the code for the unions too.

**Note that (RawOreName ore).Value is just a string.

let OreDataMap =
    [
        for oreType in FSharpType.GetUnionCases typeof<EveOnline.OreDomain.Types.OreType> do
            let ore = FSharpValue.MakeUnion (oreType, [| |]) |> unbox

            yield (RawOreName ore).Value, (ore, Common, IsCompressed)
            yield (RawOreName ore).Value, (ore, Uncommon, IsCompressed)
            yield (RawOreName ore).Value, (ore, Rare, IsCompressed)
            yield (RawOreName ore).Value, (ore, Common, IsNotCompressed)
            yield (RawOreName ore).Value, (ore, Uncommon, IsNotCompressed)
            yield (RawOreName ore).Value, (ore, Rare, IsNotCompressed)
    ]
    |> Map.ofList

Why not use nested loops?

let OreDataMap =
    [
        for common in [ Common; Uncommon; Rare ] do
            for compressed in [ IsCompressed; IsNotCompressed] do
                for oreType in FSharpType.GetUnionCases typeof<EveOnline.OreDomain.Types.OreType> do
                    let ore = FSharpValue.MakeUnion (oreType, [| |]) |> unbox
                    yield (RawOreName ore).Value, (ore, common, compressed)
    ]
    |> Map.ofList

That's probably the idiomatic way, but if you don't like excessive indentation you can always align your successive for loops - I think this looks nicer. Also, you can replace the last do with -> to eliminate the need for the yield keyword.

let OreDataMap =
    [
        for common in [ Common; Uncommon; Rare ] do
        for compressed in [ IsCompressed; IsNotCompressed] do
        for oreType in FSharpType.GetUnionCases typeof<EveOnline.OreDomain.Types.OreType> ->
            let ore = FSharpValue.MakeUnion (oreType, [| |]) |> unbox
            (RawOreName ore).Value, (ore, common, compressed)
    ]
    |> Map.ofList


My library for 2 dimensional shapes This is my first OOP PHP library and wanted to get advice on how I can improve what I wrote. This is what I needed to do:

I'm also not sure on what it meant by "proportionally resize the shape up or down, given a floating-point scale factor" I think what I did what it meant but I could be wrong.

  Build a simple PHP library for defining 2-dimensional geometric shapes and making basic calculations with them.

  For each shape, the library should be able to do the following:

  calculate the area
  calculate the perimeter
  proportionally resize the shape up or down, given a floating-point scale factor

<?PHP
echo "Circle
";
$circle = new circle(5);
$circle->getArea();
$circle->getPerimiter();
$circle->scale(up, .5);
$circle->getArea();
$circle->getPerimiter();

echo "
Right Triangle
";
$rt = new RightTriangle(4, 5);
$rt->getArea();
$rt->getPerimiter();
$rt->scale(up, .5);
$rt->getArea();
$rt->getPerimiter();

class Circle {
    public function __construct( $radius )
    {
        $this->radius = $radius;
    }
    public function getArea()
    {
        echo pow($this->radius, 2) * M_PI."
";
    }
    public function getPerimiter()
    {
        echo 2 * M_PI * $this->radius."
";
    }
    public function scale($direction, $scale)
    {
        if($direction == 'up')
        {
            $this->radius = $this->radius + ($this->radius * $scale);
        }
        else
        {
            $this->radius = $this->radius - ($this->radius * $scale);
        }
    }
}

class RightTriangle {
    public function __construct( $a, $b )
    {
        $this->a = $a;
        $this->b = $b;
    }
    public function getArea()
    {
        echo ($this->a*$this->b/2)."
";
    }
    public function getPerimiter()
    {
        echo $this->a + $this->b + sqrt(pow($this->a, 2) + pow($this->b, 2))."
";
    }
    public function scale($direction, $scale)
    {
        if($direction == 'up')
        {
            $this->a = $this->a + ($this->a * $scale);
            $this->b = $this->b + ($this->b * $scale);
        }
        else
        {
            $this->a = $this->a - ($this->a * $scale);
            $this->b = $this->b + ($this->b * $scale);
        }
    }
}
?>

The floating point scale factor could control both magnitude and direction without the need for your $direction parameter.  If you want to double the size the $scale = 2.0, if you want to halve it the $scale = 0.5

and this is just personal preference if you were to stick with your $direction parameter in your scaling functions.  Create a static class constant so you don't have to hardcode 'up' and 'down' all over the place.

<?
    $circle = new Circle(5);
    $circle->scale(Circle::UP, 2);

    Class Cirlce {
        const UP = 'up';
        const DOWN = 'down';
    .
    .
    .
    }


Searching for an idiomatic Rust implementation of Minimum Edit Distance (LeetCode #72) I am solving LeetCode Dynamic Programming challenges (this one is #72, at https://leetcode.com/problems/edit-distance).
Here below, I've implemented a Rust solution for the minimum edit distance between two strings problem. (Note that this solution works and passes all the test cases on LeetCode)
use std::collections::HashMap;
use std::cmp::min;

impl Solution {
    pub fn min_distance(word1: String, word2: String) -> i32 {
        let mut D: HashMap<(i32, i32), i32> = HashMap::new();
        let m = word1.len() as i32;
        let n = word2.len() as i32;

        let w1: Vec<char> = word1.chars().collect();
        let w2: Vec<char> = word2.chars().collect();

        for i in 0..=m {
            D.insert((i, 0), i);
        }
        for j in 0..=n {
            D.insert((0, j), j);
        }

        for i in 1..=m {
            for j in 1..=n {
                if w1[(i-1) as usize] == w2[(j-1) as usize] {
                    D.insert((i, j), *D.get(&(i-1, j-1)).unwrap());
                } else {
                    let p = *D.get(&(i - 1, j - 1)).unwrap();
                    let q = *D.get(&(i - 1, j)).unwrap();
                    let r = *D.get(&(i, j - 1)).unwrap();

                    D.insert((i, j), 1 + min(p, min(q, r)));
                }
            }
        }

        return *D.get(&(m, n)).unwrap();
    }
}

I've actually also solved the same thing in JAVA, which I have a much better command on...
public class EditDistance {
    public static int minDistance(String word1, String word2) {
        int m = word1.length();
        int n = word2.length();
        int[][] D = new int[m+1][n+1];

        for(int i=0; i<=m; ++i) D[i][0] = i;
        for(int j=0; j<=n; ++j) D[0][j] = j;

        for(int i=1; i<=m; ++i) {
            for(int j=1; j<=n; ++j) {
                if(word1.charAt(i-1) == word2.charAt(j-1)) D[i][j] = D[i-1][j-1];
                else D[i][j] = 1 + Math.min(D[i-1][j-1], Math.min(D[i-1][j], D[i][j-1]));
            }
        }
        return D[m][n];
    }

    public static void main(String[] args) {
        System.out.println(minDistance("intention", "execution"));
    }
}

Something about my Rust solution seems off to me. Not really sure but here are some of my irritations:

I couldn't find a better way to iterate a string and refer to it by its index easily other than having to convert it to a vector.

I think I'm unwrap()-ing too much. Although I'm not aiming for code golf here, it still feels subconsciously that I could have avoided so many unwrap()s.

I feel a bit bad because I kind of word to word translated my solution from Java... perhaps there's a more Rust-idiomatic way of doing whatever I did?

Looking forward to your advise and help :-) Thanks in advance!

The first thing I noticed about both versions of your code is the plenitude of single-letter names, something you can improve upon.  Incidentally, Rust uses snake_case for variables, so the compiler would have warned you about D.
Now, let's talk about the struggles I presume you have by reading the Rust code.
First of all, you probably felt forced to convert between i32 and usize all the time.  The decision to return i32 instead of usize is a typical indicator of the less-than-ideal quality of LeetCode interfaces (as usual).  In fact, it goes worse - impl Solution is not a good idea in Rust, and there is no reason to take the ownership of strings here.  My advice here would be to perform all calculations using usize and only convert to i32 at the very end.
I assume HashMap<(i32, i32), i32> results from the lack of multi-dimensional vector support in Rust.  Indeed, my first choice would be to use ndarray in this scenario, which is not available in the standard library.  You can make a small wrapper for this:
struct Vec2<T> {
    elements: Vec<T>,
    n_rows: usize,
    n_columns: usize,
}

and derive Index<(usize, usize)> and IndexMut<(usize, usize)>.  Or you could sacrifice some performance and use Vec<Vec<usize>>.
Instead of
let w1: Vec<char> = word1.chars().collect();
let w2: Vec<char> = word2.chars().collect();

I would simply use .as_bytes(), assuming that LeetCode doesn't consider multi-byte characters.

Here's how I reassembled everything according to the Java version: (not tested)
pub fn min_distance(word_a: String, word_b: String) -> i32 {
    let word_a = word_a.as_bytes();
    let word_b = word_b.as_bytes();

    let len_a = word_a.len();
    let len_b = word_b.len();

    let mut matrix = vec![vec![0; len_b + 1]; len_a + 1];

    for i in 0..=len_a {
        matrix[i][0] = i;
    }
    for j in 0..=len_b {
        matrix[0][j] = j;
    }

    for i in 1..=len_a {
        for j in 1..=len_b {
            matrix[i][j] = if word_a[i - 1] == word_b[j - 1] {
                matrix[i - 1][j - 1]
            } else {
                1 + matrix[i - 1][j - 1]
                    .min(matrix[i - 1][j])
                    .min(matrix[i][j - 1])
            };
        }
    }

    matrix[len_a][len_b] as i32
}

The restriction of LeetCode (poor interface, no external crates) caused me to be lazy, so I used nested Vecs.  In a real-world scenario, ndarray::Array2 would be a far better choice.
Besides, this is still far from optimal for Rust, where we tend to avoid manual indexing and - 1.  Taking into account the algorithmic nature of this application, I won't go deeper into this.


Integrate the product of four infinite series functions I wrote the following code to integrate the product of four functions.

Two of the functions are hypergeometric functions and the other two are incomplete gamma functions. I have written each function as an infinite series, one per nested loop. The first loop (variable k) is for time.

y1 = 0;
for k = 1:L-1
    for i = 0:N
        total1 = i*log(x) + 2*(n-m-i+k)*log(sig);
        for j = 0:N
            total2 = j*log(y) + 2*(n-m-j+k)*log(sig);
            for p = 0:100
                total3 = 0;
                for q = 0:100
                    temp = (n-m+i+k+p)*log(a);
                    temp = temp - 2*(2*n - 2*m + i + j + 2*k + p + q)*log(sig);
                    temp = temp - func1(k, j+1, q+1) + func2(i+1, j+1, p+1, q+1, k);
                    temp = temp + (i + j + 2*k + p + q)*log(sig^2/(1+a));
                    temp = temp + log(gammainc(b*(1 + x)/sig^2,i + j + p + q));
                    total3 = total3 + exp(temp);
                end
                total4 = func3(k, i+1) + (n - m + i + k + p)*log(a/sig^2);
                total4 = total4 + func5(k, i+1, p+1);
                total4 = total4 + (n - m + i + k + p + 2)*log(sig^2/a);
                total4 = total4 + log(gammainc(a*b/sig^2, i + k + p)));
                total4 = exp(total4) - total3;
                y1 = y1 + exp(total1 + total2 - func5(k, i+1, p+1) + log(total4));
            end
        end
    end
end

Thereafter, I removed the innermost loop the following way to improve speed:

y1 = 0;
for k = 1:L-1
    for i = 0:N
        total1 = i*log(x) + 2*(n-m-i+k)*log(sig);
        for j = 0:N
            total2 = j*log(y) + 2*(n-m-j+k)*log(sig);
            for p = 0:100
                q = 0:100;
                temp = (n-m+i+k+p)*log(a);
                temp = temp - 2*(2*n - 2*m + i + j + 2*k + p + q)*log(sig);
                temp = temp - reshape(func1(k, j+1, q+1),1,101)
                temp = temp + reshape(func2(i+1, j+1, p+1, q+1, k), 1, 101) ;
                temp = temp + (i + j + 2*k + p + q)*log(sig^2/(1+a));
                temp = temp + log(gammainc(b*(1 + x)/sig^2,i + j + p + q));
                total3 = total3 + exp(temp);
                total4 = func3(k, i+1) + (n - m + i + k + p)*log(a/sig^2);
                total4 = total4 + reshape(func5(k, i+1, p+1), 1, 101);
                total4 = total4 + (n - m + i + k + p + 2)*log(sig^2/a);
                total4 = total4 + log(gammainc(a*b/sig^2,i + k + p)));
                total4 = exp(total4) - total3;
                y1 = y1 + exp(total1 + total2 - reshape(func5(k, i+1, p+1),1,101) + log(total4));
            end
        end
    end
end

Can you suggest further changes to improve speed?

Basically this is the function I am trying to integrate:

\begin{eqnarray}
F &=& \sum\limits_{k=1}^{L-1}\sum\limits_{i=1}^{\infty}\sum\limits_{j=1}^{\infty}\sum\limits_{p=1}^{\infty}\left(\frac{x^i\sigma^{2\left(n-m-i+k\right)}}{\left(n-m+1\right)_ii!}\frac{y^j\sigma^{2\left(n-m-j+k\right)}}{\left(n-m+1\right)_jj!}\frac{1}{s\left(s+1\right)\cdots\left(s+p\right)}\right)\nonumber\\
&&\left[\left(\Gamma\left(n-m+j+k\right)\left(\frac{a}{\sigma^2}\right)^{n-m+i+k+p}\int_0^{b*}e^{-\frac{ab}{\sigma^2}}b^{n-m+i+k+p+1}db\right)\right]\nonumber\\
&-&\left(\sum\limits_{q=0}^{\infty}\frac{a^{n-m+i+k+p}}{\sigma^{2\left(2n-2m+i+j+2k+p+q\right)}}\frac{1}{s(s+1)\cdots(s+q)}\int_0^{b*}e^{-\frac{ab+b}{\sigma^2}b^{2n-2m+i+j+p+q+2k+2}}db\right)
\end{eqnarray}

Caveat: I am not a Matlab expert, nor am I competent at an advanced undergraduate level in mathematics.
Efficiency
Removing data structure impedance and redundant or unnecessary calculations is likely to improve the running time of the code under review.

func5 [whatever the func that is] is called twice with exactly the same arguments.

Each time it is wrapped in reshape which adds to the cost.

Indeed func1, func2, and func5 are defined each returning a data structure that requires reshaping. This can be avoided by either working with their data structure directly in the code under review or redefining the functions to return the data structure used by the code.

(n - m + i + k + p)*log(a/sig^2) is calculated twice. Sure in one location it needs to be doubled.

Each time through the inner loop (n - m + i + k + p) increases by one. There's no need to recalculate it in full. It can be passed as a parameter from the enclosing function.

a [defined elsewhere?] is a constant within the code under review. Therefore log(a/sig^2) is also a constant. Yet is being calculated twice in the inner loop.

(log sig) [sig defined elsewhere?] is calculated for each iteration at the each level of the loop. It's a constant.

There are probably other places where redundant computations can be avoided.

Code Clarity
The code is hard to read.
The more that the names of items in the code reflect the way we would label the pieces of the problem domain [mathematics here], the easier it becomes to reason about the way the code implements the problem.
sigma_k better expresses the mathematics than total1. And though I know temp = temp + ... does something, but it's not obvious what. function1_result = start_value + ... is better. partialgamma1_result = start_value + ... is better.
Final thoughts
Giving things meaningful names helps a lot. logsig = log(sig) seems like more work because it is more typing [by 8 more keystrokes]. But it's a false economy because debugging and refactoring code for speed is orders of magnitude more difficult than banging on the keyboard.


Reading and Writing in same script I just finished working through Exercise 16 of Learn Python The Hard Way.
To further study, I wanted to create a script that will read and display a file's contents on screen, and then have the user re-write the file with three lines.

Can my code be written in a much cleaner, pythonic way?

from sys import argv

script, filename = argv

print "The name of the script is: %r" % script

# open txt file and display on screen
text1 = open(filename, 'r')
print text1.read()
text1.close()

# ask user to overwrite the file
print "To overwrite this file, hit <enter>."
print "Otherwise, CTRL-C to escape."
raw_input('> ')

# open the txt file again; overwrite it with different data
# have user input new data for each line
text2 = open(filename, 'w')
print "Write three lines to the file."
line1 = raw_input('line 1: ')
line2 = raw_input('line 2: ')
line3 = raw_input('line 3: ')

text2.write('\n'.join([line1, line2, line3, '']))
text2.close()

# read and display reconstructed file
text3 = open(filename, 'r')
print text3.read()
text3.close()

First of all I would recommend you to move the code to function so that you can re-use it by importing it in some other module as well.

To differentiate between whether the module has been ran directly or it was imported use the if __name__ = "__main__" condition at the bottom of the code.

Now coming to your code:

You don't really need to open the file 3 times to do this. With r+ mode this can be done in a single go. Note that good thing about r+ mode is that it also allows us read as well as write to the file.
Use a with statement to handle a file object, it makes sure that the file object gets closed even when an error occurs.
Instead of asking the user to hit Enter or CTRL-c ask them for a simply 'yes' or 'no' or may be 'y' or 'n'.
You're always writing three lines to the file, you can also ask the user for the number of lines they would like to write. Make this a default variable to the function with value 3 if you want them to write 3 lines by default. You can pass this value using another argument to the script: python foo.py file.txt 10. Using this variable you can run a loop and ask the user to enter the lines they would like to write.
Don't create unnecessary variables like line1, line2 and line3, better use a list and append the lines to it within in a loop.
Lastly, this is not an issue. But I would recommend that you should use the new-style string formatting over the old printf style string formatting as it is much more powerful and expressive in my opinion.

Code:

from sys import argv

def file_overwrite(filename, n=3):

    with open(filename, 'r+') as f:
        content = f.read()
        print content
        # ask user to overwrite the file
        while True:
            feedback = raw_input("Type 'y' to overwrite the file or 'n' to exit: ")
            if feedback.lower() == 'y':
                # move the file pointer back to the start and then truncate the file
                f.seek(0)
                f.truncate()
                break
            elif feedback.lower() == 'n':
                # return instantly, another advantage of using a function
                return
            else:
                print "Please enter either 'y' or 'n'."

        print 'Write {} lines to the file.'.format(n)
        # Loop n times and ask for user input and save it in the list.
        lines = []
        for line_num in xrange(1, n+1):
            line = raw_input('line {}: '.format(line_num))
            lines.append(line)

        f.write('\n'.join(lines))

        # After writing the file pointer is again at the end of the file.
        # To read the content again move it back to the start and then
        # read file's content.
        f.seek(0)
        print f.read()

if __name__ == '__main__':

    # The condition will run only when the user runs this file
    # directly.

    script, filename = argv[:2]
    print "The name of the script is: {!r}.".format(script)
    try:
        n = int(argv[2])
        file_overwrite(filename, n)
    except (IndexError, ValueError):
        file_overwrite(filename)