C function to find and delete a node from a singly linked list I'm looking for the most commonly used style for writing the delete_item() function of a singly linked list, that find a matching item and deletes it. Is what I have the 'typical' or 'normal' solution? Are there more elegant ones?

What seems inelegant to me about my solution below, although I don't know a better way to express it, is that the code needs to check the first record individually (i.e. a special case), then as it goes through the iteration, it's not checking iter, it's checking iter->next, ahead of the iterator's present location, because in a singly linked list you can't go backwards.

So, is there a cleaner way to write the delete_item() function?

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

struct node {
    int x;
    struct node *next;
};

struct node *head;

struct node *create_item(int x);
void print_list();
void delete_item(int x);

int main(int argc, int argv) {

    struct node *n;
    int i;

    // initialise a linked list with a few items
    head = create_item(1);
    n = head;

    for (i = 2; i < 10; i++) {
        n->next = create_item(i);
        n = n->next;
    }

    // before
    print_list();

    // delete 7.
    delete_item(7);

    // after
    print_list();

    // lets delete all odd numbers for effect.
    delete_item(1);
    delete_item(3);
    delete_item(5);
    delete_item(9);

    print_list();
}

struct node *create_item(int x) {
    struct node *new;

    new = (struct node *) malloc (sizeof(struct node));
    new->x = x;
    return new;
}

void print_list() {
    struct node *iter;

    iter = head;

    while (iter != NULL) {
        printf("num: %i\n", iter->x);
        iter = iter->next;
    }
}

//We're looking for the best way to right this.
//This is _my_ standard solution to the problem.
// (that is, to test the first element explicitly
// the use current->next != NULL to be one behind
// the search).
//I wondered what other people's is or if there
//is a convention?
void delete_item(int x) {

    struct node *iter;
    iter = head;

    if (iter == NULL) {
        printf("not found\n");
        return;
    }

    if (iter->x == x) {
        printf("found in first element: %i\n", x);
            head = head->next;
        return;
    }

    while (iter->next != NULL) {
        if (iter->next->x == x) {
            printf("deleting element: %i\n", x);
            iter->next = iter->next->next;
            return;
        }

        iter = iter->next;
    }

    printf("not found\n");
}

This is a complete example that can be compiled and tested. The output:

23:28: ~$ gcc -o ll linked_list.c
23:28: ~$ ./ll
num: 1
num: 2
num: 3
num: 4
num: 5
num: 6
num: 7
num: 8
num: 9
deleting element: 7
num: 1
num: 2
num: 3
num: 4
num: 5
num: 6
num: 8
num: 9
found in first element: 1
deleting element: 3
deleting element: 5
deleting element: 9
num: 2
num: 4
num: 6
num: 8

The neatest way to deal with this is to use what I was taught as the "2 pointer trick" (thank you Charles Lindsay all those years ago):

Instead of holding a pointer to the record you hold a pointer to the pointer that points to it - that is you use a double indirection. This enables you to both modify the pointer to the record and to modify the record without keeping track of the previous node.

One of the nice things about this approach is that you don't need to special case dealing with the first node.

An untested sketch using this idea for delete_item (in C++) looks like:

void delete_item(node** head, int i) {
  for (node** current = head; *current; current = &(*current)->next) {
    if ((*current)->x == i) {
      node* next = (*current)->next;
      delete *current;
      *current = next;
      break;
    }
  }
}

This loop will break after the first entry it finds, but if you remove the "break" then it will remove all entries that match.


Stylish range input, Google Chrome only I have an input[type=range] element which is customized using CSS to look more modern.

I'm unhappy with my CSS because of the need to use position:relative on the wrapping element and position:absolute on an invisible range input.

I'm wondering if I made this logic unnecessarily convoluted. Is there a way to achieve the same presentation in a simpler manner? Maybe just by customize the input itself instead of hiding and emulating it?

Here's a JSFiddle and here is the accompanying CSS and HTML:

        01:47

        <input class="time-range" type="range" />

        03:36

*, *:before, *:after {
    box-sizing: border-box;
}

input:focus, button:focus {
    outline: none
}

.time-progress {
    display: flex;
    align-items: center;
    width: 300px;
    margin-top: 30px;
    margin-left: 30px;
}

.time-label {
    margin: 0 5px;
}

.time-slider {
    background: #ebebeb;
    height: 3px;
    flex: 1;
    margin: 0 10px;
    position: relative;
}

.progress {
    background: #4ea6ea;
    height: 100%;
}

.time-range {
    bottom: -1px;
    position: absolute;
    width: 100%;
}

input[type=range] {
    -webkit-appearance: none;
    background: transparent;
    height: 1px;
}

input[type=range]::-webkit-slider-thumb {
    -webkit-appearance: none;
    box-shadow: 0 0 5px #666666;
    background: #666666;
    border-radius: 100%;
    height: 10px;
    width: 10px;
}

Your code validates at the HTML and CSS validators, except for one thing:

You need to place a doctype at the top of the page:

You also need to provide head and body elements and place your code in the body tag, like this:

<!doctype html>

    <meta charset="UTF-8">
    <title>Page title</title>

I was unable to completely remove the position properties in the CSS, but I was able to remove one.  I also adjusted position of the progress bar as it was not lining up with the input bar nicely.

Here I changed the margin padding:

.time-progress {
    /* ... */
    padding-top: 30px;
    padding-left: 30px;
}

I removed two values from this class:

.time-slider {
    background: #ebebeb;
    height: 3px;
    flex: 1;
}

This is the adjusted .progress class:

.progress {
    background: #4ea6ea;
    height: 100%;
    vertical-align: top;
    position: relative;
}

This is the adjusted .time-range class:

.time-range {
    margin-left: 0px;
    padding: 0px;
    width: 100%;
    vertical-align: top;
}

I changed the progress div to this:

This prevents the blue bar from overlapping the button at the default position.

*, *:before, *:after {
    box-sizing: border-box;
}

input:focus, button:focus {
    outline: none
}

.time-progress {
    display: flex;
    align-items: center;
    width: 300px;
    padding-top: 30px;
    padding-left: 30px;
}

.time-label {
    margin: 0 5px;
}

.time-slider {
    background: #ebebeb;
    height: 3px;
    flex: 1;
}

.progress {
    background: #4ea6ea;
    height: 100%;
    margin-top: 0px;
    vertical-align: top;
    position: relative;
}

.time-range {
    margin-top: -2px;
    margin-left: 0px;
    padding: 0px;
    width: 100%;
    vertical-align: top;
}

input[type=range] {
    -webkit-appearance: none;
    background: transparent;
    height: 1px;
}

input[type=range]::-webkit-slider-thumb {
    -webkit-appearance: none;
    box-shadow: 0 0 5px #666666;
    background: #666666;
    border-radius: 100%;
    height: 10px;
    width: 10px;
}
<!doctype html>

    <meta charset="UTF-8">
    <title>Page title</title>

            01:47

            <input class="time-range" type="range" />

            03:36

Edit: The above is for FireFox, I didn't see the Chrome only section; however, it displays correctly in both FF and Chrome, so you may want to consider using it.  It also displays almost correctly in IE, but you should make the track as tall as you want the button to be in this case because the thumb is designed to be no taller than the bar.

To get rid of the position properties in Chrome, you should change the .time-range class to this:

.time-range {
    width: 100%;
}

Delete the position: relative; and the margin: 0 10px; in the .time-slider class.

Add this line to the input[type-range]::-webkit-slider-thumb property:

margin-top: -21px;

*, *:before, *:after {
    box-sizing: border-box;
}

input:focus, button:focus {
    outline: none
}

.time-progress {
    display: flex;
    align-items: center;
    width: 300px;
    margin-top: 30px;
    margin-left: 30px;
}

.time-label {
    margin: 0 5px;
}

.time-slider {
    background: #ebebeb;
    height: 3px;
    flex: 1;
}

.progress {
    background: #4ea6ea;
    height: 100%;
}

.time-range {
    width: 100%;
}

input[type=range] {
    -webkit-appearance: none;
    background: transparent;
    height: 1px;
}

input[type=range]::-webkit-slider-thumb {
    -webkit-appearance: none;
    box-shadow: 0 0 5px #666666;
    background: #666666;
    border-radius: 100%;
    height: 10px;
    width: 10px;
    margin-top: -18px;
}
<!doctype html>

    <meta charset="UTF-8">
    <title>Page title</title>

            01:47

            <input class="time-range" type="range" />

            03:36


Output digits of a number This is a simple C++ program I had to write for class. It prompts the user to input an integer and then outputs both the individual digits of the number and the sum of the digits. How do I improve this?

For example:

Input: 3456
Output:
3 4 5 6
Sum of all digits: 19

Input: 1234
Output:
1 2 3 4
Sum of all digits: 10

#include <iostream>
#include <iomanip>

using namespace std;
int main()
{
    int base;
    int first;
    int second;
    int third;
    int fourth;
    int sum;

    cout << " Please enter a whole number." << endl;
    cin >> base ;

    first = base / 1000;
    second = base / 100 % 10;
    third =  base / 10 % 10;
    fourth = base % 10;

    sum = first + second + third + fourth;

    cout << first << " " << second << " " << third<<  " " << fourth <<  " " << endl;
    cout << "Sum of the digits: " << sum << endl;

    return 0;
}

Please stop using using namespace std. Read
this
Consider using a std::vector to hold the digits of a number. Here
it seems as though you can assume the user is inputting a number
with four digits. But that might not always be the case. A vector is
dynamic.
 This is...uh, well:

first = base / 1000;
second = base / 100 % 10;
third =  base / 10 % 10;
fourth = base % 10;

Consider using a while loop to get all digits of the number.

while(number)
{
    digits.push_back(number%10);
    number /= 10;
}

Note the digits will be in reverse. You can use std::reverse() to
reverse the digits such that the first element in the vector is the
first digit of the number.
Use std::accumulate() to obtain the sum of all digits. You don't
have to do this in the while loop.

Here is everything together. Ideally, you would want to put the while loop and summing operation in another function.

#include <iostream>
#include <vector>
#include <algorithm>

using std::cout;
using std::cin;
using std::endl;
using std::vector;

int main()
{
    int number, sum;
    vector<int> digits;

    cout << "Enter a whole number: ";
    cin >> number;

    while(number)
    {
        digits.push_back(number%10);
        number /= 10;
    }

    std::reverse(digits.begin(),digits.end());
    sum = std::accumulate(digits.begin(),digits.end(),0);

    //C++11 range-based for loop
    for(auto i : digits)
        cout << i << " ";

    cout << "\nSum of the digits: ";
    cout << sum << endl;

}


Apache2 mod_rewrite directive The website I'm working on is essentially project-management software. Each project has a code (alphanumeric lowercase, with hyphens). The URL must include the codes and treat them as directories.

website.com/p/<code>/<page> => website.com/p/<page>?code=<code>

Each project will have files associated with it. All file downloads are handled through a single script called 'files.php'. The file name itself must be in the URL. Files.php can also index all the files, only when $_GET['file'] is empty.

website.com/p/<code>/files/<filename> => files.php?code=<code>&file=<filename>

Another requirement is that all project codes must be treated as directories. And the files must also serve as a directory. Attempting to otherwise will cause redirection.

website.com/p/<code> (redirects to) website.com/p/<code>/
website.com/p/<code>/files (redirects to) website.com/<code>/files/

Below is my solution, however it feels sloppy. Any advice?

<Directory ${DOC_ROOT}>
        RewriteEngine On
        RewriteBase /
        RewriteCond  %{QUERY_STRING}  !code
        RewriteRule ^p/([a-z0-9\-]+)/files$ p/$1/files/ [R,L]
        RewriteCond  %{QUERY_STRING}  !code
        RewriteRule ^p/([a-z0-9\-]+)/files/(.*)?$ p/files.php?code=$1&name=$2&%{QUERY_STRING} [L]

        RewriteCond  %{QUERY_STRING}  !code
        RewriteRule ^p/([a-z0-9\-]+)$ p/$1/ [R,L]
        RewriteCond  %{QUERY_STRING}  !code
        RewriteRule ^p/([a-z0-9\-]+)/(.*)$ p/$2?code=$1&%{QUERY_STRING} [L]
</Directory>

It's an Apache config file. It isn't going to look pretty. :)

That being said, I don't understand why you have RewriteCond specified for any of these rules. What is that condition preventing? The rewritten URL's are not causing redirect, so it is not clear to me when you would ever run into the condition of having "code" as a query string parameter.

These conditions really don't make sense for case when you are adding trailing slashes, and if you truly are getting query strings in these trailing slash use cases, you are effectively stripping the query string out of the redirect as you are not using QSA flag.

You should consider using R=301 flag instead of just R for these trailing slash redirect use cases in order to signal to the client that this is permanent location for the resource that is to be used.

I personally find it easier to read through rewrite logic and have that logic to be less error prone if all redirects are handled first in the file, followed by more complex rewrite logic.

Finally, if your app keeps growing in complexity to where you have more of these redirect rules being added, you might consider simply moving to more of a front controller model where you evaluate the URI in PHP and route within your application accordingly.


Keeping Linq's readability (e.g. over goto statements) without sacrificing performance I'm doing Project Euler problem 5 as a kata, focussing on TDD and code readability. The challenge is:

  What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

For reference, a "less-readable" solution that reeks of goto was something along these lines:

public int Solution_GotoVersion()
{
    int solution = 1;
    outer: while (solution++ < int.MaxValue)
    {
        for (int j = 1; j <= 20; j++)
        {
            if (solution % j != 0)
                goto outer;
        }
        return solution;
    }
    throw new Exception("Solution not found");
}

This takes about 4 seconds to run, relative to the other solutions.

Of course I was trying to avoid such code (in light of readability, for one), I wrote it specifically to compare it to my first solution, that uses an alternative along the lines of this SO answer. So, focussing on readability, here's something a little nicer:

public int Solution_LinqVersion()
{
    var dividers = Enumerable.Range(1, 20);

    int solution = Enumerable
                    .Range(1, int.MaxValue)
                    .FirstOrDefault(candidate
                      => dividers.All(divider => candidate.IsDivisibleBy(divider)));

    return solution;
}

This performs much worse, taking about 25 seconds, relatively. I don't mind loosing some performance when traded for readability, but a factor 8 is a bit much.

I've tried an intermediate solution, like this:

public int Solution_LittleOfBothWorlds()
{
    var dividers = Enumerable.Range(1, 20).ToArray();
    int solution = 1;
    while (solution++ < int.MaxValue)
    {
        if (dividers.All(divider => solution % divider == 0))
            return solution;
    }
    throw new Exception("Solution not found");
}

This is in the middle, performance-wise, taking about 14 seconds, relatively.

What can I do to keep relative performance close to the first example, with code that utilizes Linq's readability?

Note: I'm aware there are of  many other optimizations possible, amongst others in the algorithm itself. In this case however, I'm specifically interested in the performance of Linq queries relative to more "oldskool" constructs, and how to keep Linq's nice syntax without giving up too much performance (if possible).

Focus on optimizing the innermost loop. That's where the performance is lost. In particular try to minimize the number of indirect calls (virtual method, interface and delegate calls).

Your LINQ code has ~60 of those. My variant 1 reduces them to ~20, and my variant 2 to only a handful.

Variant 1:

Very close to linq, but using Array.TrueForAll. About twice as fast are pure linq:

public int Solution_ArrayLinqVersion()
{
    var dividers = Enumerable.Range(1, 20).ToArray();

    int solution = Enumerable
                    .Range(1, int.MaxValue)
                    .First(candidate
                      => Array.TrueForAll(dividers, divider => candidate%divider==0));

    return solution;
}

Variant 2:

Use a loop for the division testing(inner loop) and linq for the outer loop. Almost as fast as no LINQ:

int LinqOutLoopInner()
{
    return Enumerable
        .Range(1, int.MaxValue)
        .First(IsDivisibleBy1to20);
}

static bool IsDivisibleBy1to20(int candidate)
{
    for (int j = 1; j <= 20; j++)
    {
        if (candidate % j != 0)
            return false;
    }
    return true;
}


PHP form validation for name, email, and Twitter handle Given the following input values from a form, create a method or
  methods to validate the input based on their requirements. Throw an
  exception if any data is invalid.

  1.1 name: is required
  1.2 must be first name AND last name

  2.1 email is required
  2.2 is a valid email address

  3.1 twitter is optional
  3.2 is a Twitter handle

  The input data will be in the following format:

$input = [
    'name' => '...',
    'email' => '...',
    'twitter' => '...'
];

This is the code snippet I worked on. How can I improve my code?  I am not allowed to include or use any other files.

<!DOCTYPE html>

    <title>PHP Form Validation</title>

<?php

$name = '';
$email = '';
$twitter = '';
$nameError ="";
$emailError ="";
$twitterError ="";

class dataException extends Exception
{
      public function errorMessage()
      {
        $message = $this->getMessage();
        return $message;
      }
}

try
{
    if(isset($_POST['submit']))
    {
        if (empty($_POST["name"]) && empty($_POST["email"]))
        {
            throw new dataException('Name and Email are required');
        }
        if (empty($_POST["name"]))
        {
            throw new dataException('Name is required');
        }
        else
        {
            $name = test_input($_POST["name"]);
            // check name only contains letters and whitespace
            if (!preg_match("/^[a-zA-Z ]*$/",$name))
            {
                throw new dataException('Only letters and white space allowed');
            }
            else
            {
                if(str_word_count($name)!=2)
                {
                     throw new dataException('FirstName and LastName is required');
                }
            }
        }
        if (empty($_POST["email"]))
        {
            throw new dataException('Email is required');
        }
        else
        {
            $email = test_input($_POST["email"]);
            // check if e-mail address syntax is valid or not
            if (!preg_match("/([\w\-]+\@[\w\-]+\.[\w\-]+)/",$email))
            {
                throw new dataException('Email is invalid');
            }
        }
        //checked vs internet.
        $twitter = test_input($_POST['twitter']);
        if (!isset($_POST['twitter']) || $_POST['twitter'] === '')
        {
            $twitter = '';
        }
        else
        {
            if (!preg_match('/^(\@)?[A-Za-z0-9_]+$/', $twitter))
            {
                throw new dataException('Invalid twitter handle');
            }
        }
    }
}
catch (dataException $e)
{
    echo 'Caught exception: ',  $e->getMessage(), "\n";
}

function test_input($data)
{
    $data = trim($data);
    $data = stripslashes($data);
    $data = htmlspecialchars($data);
    return $data;
}
//php code ends here
?>

<form action="" method="post">
Name (FirstName and LastName):
<input type="text" name="name" value="<?php echo htmlspecialchars($name);?>">
* <?php echo $nameError;?>

E-mail:
<input type="text" name="email" value="<?php echo htmlspecialchars($email);?>">
* <?php echo $emailError;?>

Twitter:
<input type="text" name="twitter" value="<?php echo htmlspecialchars($twitter);?>">
 <?php echo $twitterError;?>

<input type="submit" name="submit" type="submit" value="Submit">
</form>

Here are just few things I noticed.

Class names
I always try and use uppercase first letters, so DataException rather than dataException.

Exception
The errorMessage() method is redundant, as it just does the same as getMessage(), and it's also not used anywhere.

Function name
test_input doesn't really describe what the function does, as it doesn't actually test, so I renamed it clean_input, which better describes it.

Possible bug
The line if(str_word_count($name) != 2){ would give the error message FirstName and LastName is required if the user enter Homer J Simpson, so I change to see if the count is lower than 2, anything else is fine.

Unused variables
The $*Error variables are echoed, but never have a value assigned to them other than the empty string, so removed references to those.

Checking for data
This maybe more personal preference than anything, but when checking for POST data, I use if(!empty($_POST)).

<!DOCTYPE html>

    <title>PHP Form Validation</title>

    <?php
    $name            = '';
    $email           = '';
    $twitter         = '';

    class DataException extends Exception {}

    try {
        if(!empty($_POST)){
            if(empty($_POST["name"]) && empty($_POST["email"])){
                throw new DataException('Name and Email are required');
            }
            if(empty($_POST["name"])){
                throw new DataException('Name is required');
            } else {
                $name = clean_input($_POST["name"]);
                // check name only contains letters and whitespace
                if(!preg_match("/^[a-zA-Z ]*$/", $name)){
                    throw new DataException('Only letters and white space allowed');
                } else {
                    if(str_word_count($name) < 2){
                        throw new DataException('FirstName and LastName is required');
                    }
                }
            }
            if(empty($_POST["email"])){
                throw new DataException('Email is required');
            } else {
                $email = clean_input($_POST["email"]);
                // check if e-mail address syntax is valid or not
                if(!preg_match("/([\w\-]+\@[\w\-]+\.[\w\-]+)/", $email)){
                    throw new DataException('Email is invalid');
                }
            }
            //checked vs internet.
            $twitter = clean_input($_POST['twitter']);
            if(!isset($_POST['twitter']) || $_POST['twitter'] === ''){
                $twitter = '';
            } else {
                if(!preg_match('/^(\@)?[A-Za-z0-9_]+$/', $twitter)){
                    throw new DataException('Invalid twitter handle');
                }
            }
        }
    } catch(DataException $e){
        echo 'Caught exception: ', $e->getMessage(), "\n";
    }

    function clean_input($data){
        $data    = trim($data);
        $data    = stripslashes($data);
        $data    = htmlspecialchars($data);
        return $data;
    }

    //php code ends here
    ?>

    <form action="" method="post">
        Name (FirstName and LastName):
        <input type="text" name="name" value="<?php echo htmlspecialchars($name); ?>">
        *

        E-mail:
        <input type="text" name="email" value="<?php echo htmlspecialchars($email); ?>">

        Twitter:
        <input type="text" name="twitter" value="<?php echo htmlspecialchars($twitter); ?>">

        <input type="submit" name="submit" type="submit" value="Submit">
    </form>


List-flattening function in erlang feels too wordy I wrote a tail-recursive list-flattening function, but I'm not too happy with it.

a) Is tail-recursion necessary here, or will the function be optimized without it?
b) It's ugly how many clauses there are, and that I have to call out to flatten in the expression of the first clause of flatten_acc

So, how could I do better?

(To be clear, this is a function that completely flattens the list -- e.g. flatten([foo, [[[bar]], [baz]]]) == [foo, bar, baz])

flatten(List) ->
    lists:reverse(flatten_acc(List, [])).

flatten_acc([[[H|T0]|T1]|T2], Acc) ->
    flatten_acc([flatten([[H|T0]|T1])|T2], Acc);
flatten_acc([[H|T0]|T1], Acc) ->
    flatten_acc([T0|T1], [H|Acc]);
flatten_acc([[]|T], Acc) ->
    flatten_acc(T, Acc);
flatten_acc([H|T], Acc) ->
    flatten_acc(T, [H|Acc]);
flatten_acc([], Acc) ->
    Acc.

Thanks!

Little smaller than yours.

% flatten a list
my_flatten([])->
    [] ;
my_flatten([[]|T])->
    my_flatten(T);
my_flatten([[H|T]|T2])->
    my_flatten([H|[T|T2]]);
my_flatten([H|T])->
    [H|my_flatten(T)].

http://erlang99.wordpress.com/2009/01/19/p07-flatten-a-nested-list-structure/

my_flatten_rec(L)->
    lists:reverse(my_flatten_rec([],L)).
my_flatten_rec(Acc,[[]|T])->
    my_flatten_rec(Acc,T);
my_flatten_rec(Acc,[[H|T]|T2])->
    my_flatten_rec(Acc,[H|[T|T2]]);
my_flatten_rec(Acc,[H|T])->
    my_flatten_rec([H|Acc],T);
my_flatten_rec(Acc,[])->
    Acc.


Scraper for words from Wiktionary I wrote this code in Java using the Jaunt library. The program scrapes all words from Wiktionary from category "English_uncountable_nouns". And after save
each world to text file.
I am not sure that exceptions are handled in best way, so I'm looking for tips on how to make this code easier to understand and more stable. I removed some of the redundancies but it is still strange.

package wiksurfer;
import com.jaunt.*;
import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;
import java.util.logging.Level;
import java.util.logging.Logger;

public class Wiksurfer {

    public static void main(String[] args) {
        surfPages();
    }

    public static void surfPages() {
        int i = 0;//counter for number of processed pages
        UserAgent userAgent = new UserAgent();
        userAgent.settings.autoSaveAsHTML = true;  //change settings to autosave last visited page.
        Elements elements;
        String wikiword;

        try {
            PrintWriter pw = new PrintWriter(new FileWriter("wkwords.txt"));
            try {
                String href = "http://en.wiktionary.org/wiki/Category:English_uncountable_nouns";

                for (i = 0;; i++) {
                    userAgent.visit(href);
                    href = userAgent.doc.getHyperlink("next page").getHref().replaceAll("amp;", "");
                    //Jaunt has now a strange bug on processing strings, so the fragment replaceAll("amp;", "")
                    //is devoted to fix this issue
                    System.out.println("next page:" + href);

                    elements = userAgent.doc.findEvery("<div class=mw-category-group>").findEach("<li>").findEach("<a>");
                    //select all links which contan words we need
                    for (Element el : elements) {           //iterate through Results
                        wikiword = el.getText().replaceAll("amp;", "");
                        if (isGoodWord(wikiword)) {
                            pw.println(wikiword);
                        }
                    }
                }

            } catch (JauntException e) {
                System.err.println(e);
            } finally {
                pw.close();
            }
        } catch (IOException ex) {
            Logger.getLogger(Wiksurfer.class.getName()).log(Level.SEVERE, null, ex);
        }
    }

    public static boolean isGoodWord(String s) {
        char c;
        if (s.split(" +").length > 1) {
            return false;
        }
        c = s.charAt(0);
        return ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z'));
    }
}

First, you could wrap the PrintWriter pw into a try-with-resources statement.

Without knowing the library, it is unclear how the loop for (i = 0;; i++) can be left (only with an exception). Does some method throw a JauntException ?
If so, where? JauntException also seems to be a quite general exception. Maybe a more specific one can be used. Also if the API provides such methods, it might be a good idea to test if more results are available (like hasNext() in Iterator) (instead of relying on exceptions for control flow).


Integer seconds to formated string mm:ss I have a String like "Time left (XX)" or "You need stay here for (XX)" and int as second. I use those two variables to format label like "string (mm:ss)" example "Time left (59:23)".

private String timeDescription(String pDescription,int pTime) {

    final String preformatedTime = secondsToString(pTime);
    final String timeForReturn = putTimeInXX(pDescription,preformatedTime);
    return timeForReturn;
}

private String secondsToString(int pTime) {
    final int min = pTime/60;
    final int sec = pTime-(min*60);

    final String strMin = placeZeroIfNeede(min);
    final String strSec = placeZeroIfNeede(sec);
    return String.format("%s:%s",strMin,strSec);
}

private String placeZeroIfNeede(int number) {
    return (number >=10)? Integer.toString(number):String.format("0%s",Integer.toString(number));
}

private String putTimeInXX(String pDescription,String pTime) {
    String[] apartDescription = pDescription.split("XX");

    StringBuilder descriptionForReturn = new StringBuilder();
    for (int i = 0; i < apartDescription.length; i++) {
        descriptionForReturn.append(apartDescription[i]);
        if (i == 0) {
            descriptionForReturn.append(pTime);
        }
    }
    return descriptionForReturn.toString();
}

Your code looks functional, and, if this was for early versions of Java, it would be quite typical and standard.

The String.format process can do so much more than what you are using it for, though.

Your function:

private String secondsToString(int pTime) {
    final int min = pTime/60;
    final int sec = pTime-(min*60);

    final String strMin = placeZeroIfNeede(min);
    final String strSec = placeZeroIfNeede(sec);
    return String.format("%s:%s",strMin,strSec);
}

can be simplified drastically by using more of the String.format functionality:

private String secondsToString(int pTime) {
    return String.format("%02d:%02d", pTime / 60, pTime % 60);
}

The %02d means: "Format as a decimal number using at least 2 digits, and pad with 0 if less than 2 digits"

Note, that using the modulo 60 pTime % 60 gets the remainder of seconds when dividing by 60. This is  short hand for pTime - (ptime / 60) * 60


Print an ASCII diamond This takes a width specified by user and prints a diamond of that width.  It uses only three for loops, but could I reduce that further? Is there a more elegant solution?

public class Diamond {

    static boolean cont = true;

    public static void main (String[] args) {
        Scanner input = new Scanner(System.in);
        while (cont) {
            System.out.print("Width: ");
            int width = input.nextInt();
            int lines = width;
            System.out.println();
            for (int line = 0; line < lines; line++) {
                for (int spaces = 0; spaces < Math.abs(line - (lines / 2)); spaces++) {
                    System.out.print(" ");
                }
                for (int marks = 0; marks < width - 2 * (Math.abs(line - (lines / 2))); marks++) {
                    System.out.print("x");
                }
                System.out.println();
            }
            System.out.println();
        }
    }
}

There are a few things that we can do to clean this up.

This is something I would extract to its own method.  Handle getting the user input in the main() method, and then pass that on to the drawDiamond() method.
Your for loops are divided into iterating over lines, spaces, and
marks.  We can simplify that down to just rows and columns where we can iterate over each individual unit at a time.  This will also eliminate one of your System.out.println()s in the final method.
We can simplify down the math of where to print a piece of the diamond.

if ((column == Math.abs(row - half)) || (column == (row + half)) || (column == (sqr - row + half - 1)))

Final Method:

void drawDiamond(int sqr)
{
    int half = sqr/2;
    for (int row=0; row<sqr; row++)
    {
        for (int column=0; column<sqr; column++)
        {
            if ((column == Math.abs(row - half)) || (column == (row + half)) || (column == (sqr - row + half - 1)))
            {
                System.out.print("*");
            }
            else System.out.print(" ");
        }
        System.out.println();
    }
}


DateTime internal representation to FILETIME Here is a routine I put together to convert SQL Server's internal DateTime representation (BINARY(8)) to a FILETIME. I know that the SQL DateTime (as in GetDate()) is not precise and will vary from the SystemTimeAsFileTime. But it should be different by milliseconds, but by seconds, right?

In any case, here is my routine and I was wondering if anyone saw any flaws in it:

int SQLDateTimeToFileTime(PULARGE_INTEGER t_sql, PULARGE_INTEGER t_ft)
{
    ULONGLONG days = t_sql->LowPart;
    ULONGLONG ticks = t_sql->HighPart;
    days += (ULONGLONG) 109207;         // 01/01/1900 - 01/01/1601
    days *= (ULONGLONG) 864000000000;   // (FT_SECONDS_PER_DAY * FT_SECOND);
    ticks *= (ULONGLONG) 33333333333;   // MS per tick (3.33...) * 10000);
    ticks /= (ULONGLONG) 1000000;       // to avoid floating point operation
    days += ticks;
    t_ft->QuadPart = days;
    return(0);
}

This function always returns 0, but you haven't stated why that's important for this use (if it's intended).  Since it doesn't also return 1 or another non-0 value for failure, I assume that it's not returning error codes.  Otherwise, it should return void, and the return 0 should then be removed.
Since this is also tagged with c++, I'll also provide the C++ equivalent of the constants given in @chux's answer:

constexpr unsigned long long FT_SECONDS_PER_DAY = 24ULL * 60 * 60;
constexpr unsigned long long FT_SECOND = 10000000ULL;

(constexpr has them evaluated at compile time.)

Or for brevity, they can use auto instead:

constexpr auto FT_SECONDS_PER_DAY = 24ULL * 60 * 60;
constexpr auto FT_SECOND = 10000000ULL;


Data model code running too slowly The model takes a long time to run. I have tried to vectorise it as far as I can, but it's still taking about a day to compute. I'm wondering if it's possible to speed it up.

Maybe the problem isn't even with the code, but with the model being improper/bad priors/...? Or maybe the data set is simply very large and supposed to take hours to compute. Either way, for now, I dread adding another predictor.

# data is in wide format, one row with one outcome and a range of predictors

set_cppo(mode = "fast")

code ="                     // the model itself
data        {
       int<lower=1> nrow;   // 72000 (3 items per W*N*R)
       int W;               // n words; 24
       int N;               // n subjects; 100
       int R;               // n raters; 10
       int<lower=1,upper=nrow> ind[nrow]; // this vector is used to index fields per row
       int<lower=0,upper=1> responses[nrow];  // outcomes
       int subjects[nrow];
       int words[nrow];
       int sex[nrow];
       int languages[nrow];
       int raters[nrow];
}

parameters  {
real intercept;               // "fixed"
real beta_language;
real beta_sex;
real interaction_s_l;
real beta_rater[R];           // "random"
real beta_word[W];
real beta_subject[N];
}

model       {
real theta[nrow];

 beta_word ~ normal(0,100);         // priors
 beta_subject ~ normal(0,100);
 beta_language ~ normal(0,100);
 beta_sex ~ normal(0,100);
 interaction_s_l ~ normal(0,100);

for (i in 1:nrow){                    // for each row, construct theta
theta[i] <- intercept
          + beta_sex * sex[ind[i]]
          + beta_language * languages[ind[i]]
          + interaction_s_l * languages[ind[i]] * sex[ind[i]]
          + beta_subject[subjects[ind[i]]]
          + beta_rater[raters[ind[i]]];}

responses ~ bernoulli_logit(theta);   // sample
}

generated quantities {               // get odds ratios
real odds_intercept;
real odds_sex;
real odds_language;
real odds_interaction;

odds_intercept <- exp(intercept);
odds_sex <- exp(beta_sex);
odds_language <- exp(beta_language);
odds_interaction <- exp(interaction_s_l);
}
"

fit <- stan(model_code = code, data = dat,     // run the model
            iter = 4000, chains = 3)

You made a typo. The line

responses ~ bernoulli_logit(theta);

should be outside the for loop in the model block.


Modern OpenGL shader wrapper v2 This is a fixed up version (following RAII principles) of my previous reviewal. I have moved the construction of the object entirely to the constructor to prevent any bad object states.

The only thing I am slightly concerned about is if an exception is thrown, the destructor will not be called thus causing shader objects to leak (as glDeleteShader won't be called).

Maybe wrapping the shader handle ID (shader_id) in some sort of POD-wrapper could fix this?

compilation_error.h

#pragma once

#include <stdexcept>
#include <string>

class CompilationError : public std::runtime_error {
public:
    CompilationError(const std::string &message, std::string info_log);

    const std::string &get_info_log() const;

private:
    const std::string info_log;
};

compilation_error.cpp

#include <render/shader/compilation_error.h>

CompilationError::CompilationError(const std::string &message, std::string info_log) :
    runtime_error(message), info_log(std::move(info_log)) {}

const std::string &CompilationError::get_info_log() const {
    return info_log;
}

shader.h

#pragma once

#include <engine.h>
#include <string>

class Shader {
public:
    Shader(GLenum type, const std::string &path);

    ~Shader();

private:
    const GLuint shader_id;
};

shader.cpp

#include <render/shader/shader.h>
#include <render/shader/compilation_error.h>
#include <fstream>

Shader::Shader(const GLenum type, const std::string &path) :
    shader_id(glCreateShader(type)) {
    if (!shader_id) {
        throw std::runtime_error("Unable to create shader");
    }

    std::ifstream input_stream(path, std::ifstream::in | std::ifstream::ate);

    if (!input_stream) {
        glDeleteShader(shader_id);

        throw std::ifstream::failure("Unable to open shader file");
    }

    const long source_length = input_stream.tellg();
    std::vector<char> source((unsigned long) source_length);

    input_stream.seekg(0);
    input_stream.read(source.data(), source_length);

    const GLchar *sources[] = {source.data()};
    const GLint lengths[] = {(GLint) source_length};

    glShaderSource(shader_id, 1, sources, lengths);
    glCompileShader(shader_id);

    GLint compile_status;
    glGetShaderiv(shader_id, GL_COMPILE_STATUS, &compile_status);

    if (!compile_status) {
        GLint log_length;
        glGetShaderiv(shader_id, GL_INFO_LOG_LENGTH, &log_length);

        std::vector<GLchar> log_output((unsigned long) log_length);
        glGetShaderInfoLog(shader_id, log_length, nullptr, log_output.data());

        std::string info_log(log_output.data());

        if (!info_log.empty() && info_log.back() == '\n') {
            info_log.pop_back();
        }

        glDeleteShader(shader_id);

        throw CompilationError("Unable to compile shader", info_log);
    }
}

Shader::~Shader() {
    glDeleteShader(shader_id);
}

This looks a lot better! Good job!

Nitty-gritty stuff:

better cleanup in the constructor

You hava a lot of the following idiom in the constructor:

if(condition) {
  glDeleteShader(shader_id);
  throw something;
}

This is a little fragile, since should you make the class more complex, that's a lot of places to refactor. The following would be better:

try {
  //most of the constructor in here
  ...
  if(condition) throw something;
  ...
}
catch(...) {
  glDeleteShader(shader_id);
  throw;
}

ifstream can throw its own exceptions if you tell it to

ifstream input_stream;
input_stream.exceptions( std::ios::failbit );
input_stream.open(path, std::ifstream::in | std::ifstream::ate);

Use iterators when converting a vector to a string

std::string info_log(log_output.data());
vs
std::string info_log(log_output.begin(), log_output.end());

The second one is better because it works whether there is a null terminator or not. It also removes a strlen() call from the code, which is a O(N) operation.

That's actually all I've got. Good job once again.


Checking for Internet connectivity on Android I wrote this code to check an internet connection and was wondering if this is the best way to do this check.

public static boolean isNetworkAvailable(Context context) {
    boolean status = false;
    try {
        ConnectivityManager cm = (ConnectivityManager) context
                .getSystemService(Context.CONNECTIVITY_SERVICE);
        NetworkInfo netInfo = cm.getNetworkInfo(0);

        if (netInfo != null
                && netInfo.getState() == NetworkInfo.State.CONNECTED) {
            status = true;
        } else {
            netInfo = cm.getNetworkInfo(1);
            if (netInfo != null
                    && netInfo.getState() == NetworkInfo.State.CONNECTED)
                status = true;
        }
    } catch (Exception e) {
        e.printStackTrace();
        return false;
    }
    return status;
}

I am not familiar enough with Java to know what inside your try block could throw an exception or what sort of exceptions you might expect, however, simply printing the stack trace surely can't really be all that helpful.

First of all, any end user isn't going to see this information.  Second of all, no developer will see this testing outside of the development environment.

Again, I'm not that familiar with Java, so I don't know what options are available, but you need to do one of a few things.

Show the exception to the user (or some information about it anyway).  This is probably completely unnecessary.  The network connection is either available or not, right?
Rethrow the exception and let the caller handle it.  It can be useful to know there was an exception and we should retry the network connection later.
Log the exception in a way that the developer can access even when it happens in production.  Error logs should be uploaded somewhere.
Just do nothing, and don't waste execution time printing the stack trace.  If you chose this option, you probably want to find a way to leave the print stack trace in for development environments, but for production builds, you shouldn't be executing this code.

Now.... our code also has some magic numbers in it.  I realize this isn't entirely your fault.  I've visited the official documentation.  I see, that somehow, this method simply takes an int and the documentation doesn't seem to clarify what the legitimate values are very well... but they are all defined as constants:

ConnectivityManager.TYPE_BLUETOOTH
ConnectivityManager.TYPE_DUMMY
ConnectivityManager.TYPE_ETHERNET
ConnectivityManager.TYPE_MOBILE
ConnectivityManager.TYPE_MOBILE_DUN
ConnectivityManager.TYPE_MOBILE_HIPRI
ConnectivityManager.TYPE_MOBILE_MMS
ConnectivityManager.TYPE_MOBILE_SUPL
ConnectivityManager.TYPE_VPN
ConnectivityManager.TYPE_WIFI
ConnectivityManager.TYPE_WIMAX

These are the constants we should be passing in when checking for a connection.  Using these constants makes things a bit more readable.  But moreover, I might modify our method to be more flexible.  You're only checking for WIFI or MOBILE (I don't know enough about Android to know if this is excluding anything important).  But we can write our function in such a way that we can check for whatever types the user needs.

public static boolean isNetworkAvailable(Context context, int[] networkTypes) {
    try {
        ConnectivityManager cm = (ConnectivityManager)context.getSystemService(Context.CONNECTIVITY_SERVICE);
        for (int networkType : networkTypes) {
            NetworkInfo netInfo = cm.getNetworkInfo(networkType);
            if (netInfo != null && netInfo.getState() == NetworkInfo.State.CONNECTED) {
                return true;
            }
        }
    } catch (Exception e) {
        return false;
    }
    return false;
}

Now our method is duplicating less code and its flexible enough to check for what ever types of networks the user happens to specify.  If we want a method that doesn't require passing in networks and still does what your original implementation does, we can just write a wrapper method that passes mobile/wifi in to this method.


Connected component analysis I finished a program to do connected component analysis using union - find algorithm. Is it true that the complexity of the code is \$O(N logN)\$, where \$N\$ is the total number of pixels (512x512 by example, not 512).

Is there any way to improve the performance?

import cv2
import numpy as np
import random

class QuickUnionUF:

    def __init__(self, N):
        self.id = range(N)
        self.sz = [0] * N

    @classmethod
    def fromimage(self, im):
        self.id = im
        self.sz = [0] * len(im)

    def root(self, i):
        while (i != self.id[i]):
            i = self.id[i]
        return i

    def getresult(self):
        result = [self.root(i) for i in self.id]
        return result

    def connected(self, p, q):
        return self.root(p) == self.root(q)

    def union(self, p, q):
        i = self.root(p)
        j = self.root(q)

        if (i == j):
            return
        if (self.sz[i] < self.sz[j]):
            self.id[i] = j
            self.sz[j] += self.sz[i]
        else:
            self.id[j] = i
            self.sz[j] += self.sz[i]

def bwlabel(im):

    M, N = im.shape[:2]
    qf = QuickUnionUF(M * N)
    for i in range(M - 1):
        for j in range(N - 1):
            if (im[i][j] == im[i][j+1]):
                qf.union(i * N + j, i * N + j + 1)
            if (im[i + 1][j] == im[i][j]):
                qf.union(i * N + j, (i + 1) * N + j)

    mask = np.reshape(np.array(qf.getresult()), (M, N))
    values = np.unique(mask).tolist()

    random.seed()
    colors = [(random.randint(0,255), random.randint(0,255), random.randint(0,255)) for k in range(len(values))]

    out = np.zeros((M, N, 3))
    for i in range(M):
        for j in range(N):
            label = values.index(mask[i][j])
            out[i,j] = colors[label]

    return out

im = cv2.imread("bw.jpg",cv2.IMREAD_GRAYSCALE)
out = bwlabel(im > 100)
cv2.imwrite("result.png", out)

Is it true that the complexity of the code is O(NlogN), where N is the total number of pixels (512x512 by example, not 512).

You main loop in bwlabelis:

for i in range(M - 1):
    for j in range(N - 1):
        if (im[i][j] == im[i][j+1]):
            qf.union(i * N + j, i * N + j + 1)
        if (im[i + 1][j] == im[i][j]):
            qf.union(i * N + j, (i + 1) * N + j)

So you are calling qf.union \$O(N*M) = O(n)\$ times. There you call self.root twice:

def root(self, i):
    while (i != self.id[i]):
        i = self.id[i]
    return i

What's the worst case running time of this? At most \$O(n)\$, since self.id has \$n\$ values and there are no cycles. However, since you are comparing ranks in union:

    if (self.sz[i] < self.sz[j]):
        self.id[i] = j
        self.sz[j] += self.sz[i]
    else:
        self.id[j] = i
        self.sz[j] += self.sz[i]

You will have \$O(log\ n)\$ paths to root, so the total running time is \$O(n\ log\ n)\$.

  Is there any way to improve the performance?

Yes, path compression. You descended down two paths from p and q to i and j and it took \$O(log\ n)\$. At each step you can point a node to its root, instead of just the next element. You can do this recursively like so:

def root(self, i):
    if i == self.id[i]:
        return i
    self.id[i] = self.root(self.id[i])
    return self.id[i]

Recursion adds its own cost, though, so whether it's faster in practice would require testing. An alternative would be to add a second iterative compression function:

def compress(self, i, r):
    while i != r:
        i, self.id[i] = self.id[i], r

Now you are walking the path twice, which again may or may not be faster than recursion. You should only need to call this for one of the paths, though.


VbScript/ASP Classic good OOP Pattern I have to create a website in Classic ASP, and I really want to do a clean design so I will try to put everything in Classes and Models to emulate the ViewModel Pattern.

So can you tell me if this approach of creating objects is good because I will use it everywhere

Class User
    Public Name
    Public Adress
    Public Title
    Public Text
    Public Rights 'ArrayList of Right objects

    Private Sub Class_Initialize()
       Initialize
    End Sub

    Private Sub Class_Terminate()
       Dispose
    End Sub

   Private Sub Initialize()
       Name    = ""
       Adress  = ""
       Title   = ""
       Text    = ""
       Set Rights = Server.CreateObject("System.Collections.ArrayList") ' Arraylist .net
   End Sub

   Private Sub Dispose()
   End Sub

End Class

Class Right

    Public Name

    Private Sub Class_Initialize()
        Initialize
    End Sub

   Private Sub Class_Terminate()
       Dispose
   End Sub

   Private Sub Initialize()
       Name    = ""
   End Sub

   Private Sub Dispose()
   End Sub
End Class

And then I do this for instantiating objects :

Dim myUser
Set myUser = New User

'Do some work

Set myUser = nothing 'Dispose the object

Any idea, suggestion or correction is welcome.

I am not a big VB person, but I would think that if you give your class a Dispose method you should use it rather than setting it to nothing, I know this is kind of the same thing, but if you aren't going to use the Method don't create the method, it's extra stuff.  if you are going to use it in the future, comment to that effect.

'This Method is for future development
Private Sub Dispose()
End Sub

OR

'TODO: Implement!
Private Sub Dispose()
End Sub

my comment syntax is probably off a little bit.


Powershell to quickly ping a number of machines I came up with the below code to improve the peformance of pinging a large number of machines.
At present it's fairly basic, but thought I should see what people thought before proceeding further.

NB: This is my first time playing with workflows, so I've probably committed a few faux-pas there.

cls
workflow Test-ConnectionQuickly {
    [cmdletbinding()]
    param(
        [Parameter(Mandatory = $true)]
        [string[]]$computers
    )
    $computerGroups = InlineScript {
        #group computers to throttle the number of threads
        [psobject]$itemCounter = [pscustomobject]@{itemNo=0;groupSize=10} #alter groupSize per your preference
        $itemCounter | Add-Member -MemberType ScriptMethod -Name 'groupNo' -Value {
            [math]::Floor($this.itemNo++ / $this.groupSize)
        }
        write-output $using:computers | Group-Object -Property {$itemCounter.groupNo()}
    }
    ForEach -parallel ($computerGroup in $computerGroups) {
        $computerGroup | select -ExpandProperty group | %{
            $pingable = (Test-Connection $_ -Count 2 -Quiet -ErrorAction SilentlyContinue)
            write-output (new-object -type psobject -property @{Name=$_;Online=$pingable})
        }
    }
}

[string[]]$computers = (1..100 | %{("Server{0:000}" -f $_)}) #here's where we'd read in the computere from file
Test-ConnectionQuickly $computers | select Name, Online #here's where we'd pipe the output to file

Design Notes:

The InlineScript is used to group servers into small sets (of 10; an arbitrary size) in the hope of balancing serial's performance for small sets against parallel's performance for large sets of computers.
The $itemCounter variable's an attempt at making grouping into defined set sizes simpler to read
-count 2 is specified on the test-connection so that we have some tolerence for network glitches without too much affect on performance.
-Quiet is specified because we don't need output / and not having output will help performance.

Want to start of with I am new to workflows as well. For the most part your code is solid. You should see a couple of suggestions that would actually improve performance especially if you are running this against 100's of computers.

Grouping logic

This is more of a compliment. I thought, from what I read, that -ThrottleLimit would be a simple answer to replace your grouping logic. Likely from my inexperience but most of my testing shows that what you have there outperforms some simple tests that use just -ThrottleLimit. I know that it does have its place but here I think what you have is actually better.

Use a parameter for grouping/ throttling

You have a necessary property for $computers but I think it would make just as much sense to have a parameter in the workflow for GroupSize as well.

Use of ScriptMethod

While this is a neat way to get the computers group together I think it is overkill for a simple array of computers. You can avoid group-object all together really since you already know how to group the computers. Instead consider just using a simple foreach-object loop that extracts the chunks of computers each as there own array. In practice there is no performance gain here. It is easier to understand if it was simple array splitting.

Many different ways of splitting the array into smaller chunks as well. I was trying to avoid a simple counter that reset when it reached the $groupsize

As an aside I'm am disappointed that you still need Add-Member if you want to make something other than a note property easily. I read about another approach here but did not think it was useful enough to suggest in your case.

New-Object and [pscustomobject]

You use both approaches for creating object. Might as well just stick to one. When I changed your final object creation to just a [pscustomobject] cast on the hashtable it actually improved performance!

Testing the connection twice

I like the idea of hardening the test. It is possible that a computer might not respond to the first ping and could be recorded as down when it is indeed not. So I am fine with doing the test twice.

However when you are testing a computer that is up you would ping it twice. Would be better to do two separate tests and do it in such a way that the first, if successful, will prevent checking again as it would be redundant.

Consider the following very simple If statement.

if(1 -eq 1 -or 1/0 ){"Yo"}

At first glance you should see the divide by 0 for the second condition and expect failure. That will never be checked since 1 -eq 1 will always be true. Since the first condition is true then PowerShell will ignore the right hand condition as it would not change the outcome. This concept is covered under about_logical_operators in the last paragraph.

So I propose something like this:

$pingable = (Test-Connection $_ -Count 1 -Quiet -ErrorAction SilentlyContinue) -or
                        (Test-Connection $_ -Count 1 -Quiet -ErrorAction SilentlyContinue)

Same command is called twice but only if the first one fails. So it is still hardened but should be efficient as well. I suppose you could splat the commands since they are the same but I was trying to keep extra logic out of the loop for performance.

You by no means need to use this code but I implore you to try both yours against mine to see if there is at least a performance improvement.

workflow Test-ConnectionQuickly {
    [cmdletbinding()]
    param(
        [Parameter(Mandatory = $true)]
        [string[]]$computers,
        [Parameter(Mandatory = $false)]
        [int]$GroupSize = 10
    )
    $computerGroups = InlineScript {
        #group computers to throttle the number of threads
        $numberofGroups = [math]::Ceiling($using:Computers.Count / $using:groupSize)
        1..$numberofGroups | ForEach-Object{
            # Arrays are zero based which is why you will see -1
            ,($using:computers | Select -Index ((($_ - 1) * $using:GroupSize)..($_ * $using:GroupSize - 1)))
            # Use the unary operator to ensure an array of computers is returned and not unrolled.
    }

    }
    ForEach -parallel ($computerGroup in $computerGroups) {
        $computerGroup | ForEach-Object{
            $pingable = (Test-Connection $_ -Count 1 -Quiet -ErrorAction SilentlyContinue) -or
                        (Test-Connection $_ -Count 1 -Quiet -ErrorAction SilentlyContinue)
            [pscustomobject]@{Name=$_;Online=$pingable}
        }
    }
}

The only points I would recommend, if nothing else, is [pscustomobject]@{Name=$_;Online=$pingable} and how I broke up the ping test into 2 separate tests.

Most of my testing was done with a $GroupSize of 5 and 80 computers.

Measure-Command -Expression {Test-ConnectionQuickly $computers -GroupSize 5}

You definitely need to experiment with $GroupSize to find that sweet spot.


A simple command-line calculator I created a simple command line calculator, and then after some reading I made a few tweaks and then remade it with a friends idea to make it more simpler. I'm wondering if this can be more optimized or if any part of the code can be done "better"?

I recently started learning Java and wanted for this first project of mine to end up the "best possible" written, so I can start learning on what is a "good code".

One of the things that was changed over time was the use of the Scanner.

Firstly I had it create a new one on each iteration of kalkulator(); but that was obviously a unnecessary waste.
Then I had a static scanner static Scanner scanMe = new Scanner(System.in); but somewhere I read It was better to pass it as an argument, because something that doesn't belong anywhere doesn't have place in a good code unless it is really necessary.

Here is what I ended up with so far:

"Main" Method:

public class SimpleCalculator {

public static void main(String[] args) {

    Scanner scanMe = new Scanner(System.in);

    kalkulator(scanMe);
}

"Helper" Methods:

static double scanDouble(Scanner scan){
    while (!scan.hasNextDouble()){

        System.out.println("Invalid number! Please try again.");
        scan.nextLine();
    }

    return scan.nextDouble();
}

static String scanOperator(Scanner scan){
    String In = scan.next();

    while (!(In.equals("+") || In.equals("-") || In.equals("*") || In.equals("/") || In.equals("end"))) {

        System.out.println("Invalid operator! Please select either: +,-,*,/");
        In = scan.next();
    }

    return In;
}

The Main Method:

    public static void kalkulator(Scanner scan) {

    double prviB = 0, drugiB = 0, rezultat = 0;
    String operator;

    System.out.println("Enter the 1st number: ");
    prviB = scanDouble(scan);

    System.out.println("Enter the 2nd number: ");
    drugiB = scanDouble(scan);

    System.out.println("Select an operator: (+,-,*,/) , or type 'end' for termination: ");
    operator = scanOperator(scan);

    switch (operator) {
    case "+":
        rezultat = prviB + drugiB;
        break;

    case "-":
        rezultat = prviB - drugiB;
        break;

    case "*":
        rezultat = prviB * drugiB;
        break;

    case "/":
        rezultat = prviB / drugiB;
        break;

    case "end":
        System.out.println("Terminated.");
        scan.close();
        System.exit(0);
        break;

    default:
        break;
    }

    System.out.println("Result: " + rezultat);
    System.out.println("");

    kalkulator(scan);
}

Nice improvement is to use Enum with behaviour. This is an example from Effective Java book :

public enum Operation {
    PLUS("+") {
        double apply(double x, double y) {
            return x + y;
        }
    },
    MINUS("-") {
        double apply(double x, double y) {
            return x - y;
        }
    },
    TIMES("*") {
        double apply(double x, double y) {
            return x * y;
        }
    },
    DIVIDE("/") {
        double apply(double x, double y) {
            return x / y;
        }
    };
    private final String symbol;

    Operation(String symbol) {
        this.symbol = symbol;
    }

    @Override
    public String toString() {
        return symbol;
    }

    abstract double apply(double x, double y);
}


Making the basic accumulator in Real World OCaml use Async I was going through Real World OCaml and wanted to make the program in "A Complete Program" use Async. It works now, but I'm wondering how idiomatic it is, and if I'm doing anything weird?

open Core.Std
open Async.Std

let read_and_accumulate () =
  Reader.lines (Lazy.force Reader.stdin)
  |> Pipe.filter ~f:(Fn.non String.is_empty)
  |> Pipe.map ~f:Float.of_string
  |> Pipe.fold ~init:0. ~f:(fun x y -> return (x +. y))

let () =
  ignore (read_and_accumulate ()
          >>| printf "Result is %f\n"
          >>| fun () -> exit 0);
  ignore (Scheduler.go ())

Also, I feel like I should be able to Fn.compose return (+.) for the Pipe.fold line but it doesn't work. Why not?

It is totally idiomatic and pretty code. My personal style is to put the pipe operators to the end, as they are, at their essence, just functional semicolons, e.g.,

let read_and_accumulate () =
  Reader.lines (Lazy.force Reader.stdin)  |>
  Pipe.filter ~f:(Fn.non String.is_empty) |>
  Pipe.map ~f:Float.of_string |>
  Pipe.fold ~init:0. ~f:(fun x y -> return (x +. y))

With such approach every line starts with a word, not the punctuation that helps the readability.

What concerning the compose function, it didn't work for you because (+.) is binary (in fact it is not binary, but it is a function that returns a function). So, when you compose a return function that has type 'a -> 'b, with some function f : 'c -> 'c -> 'c, then you actually has a type ('c -> 'c) -> 'b, where ('c -> 'c) is the return type of function f. Note, the parenthesis. The ('c -> 'c) -> 'b is not equivalent to 'c -> 'c -> 'b. The first type describes a function, that takes a function and returns a value (i.e., apply-style function), and the latter type describes a function that take a value, and returns a function, that takes another value and computes the result (the curried binary function).


Skipping whitespaces when reading file In reading a file, I found blank lines (even at the bottom) crash the program. To fix this, I've added the following if statement:

string line;
while(getline(inFile, line))
{
    if(line.length() < 1 || (line.length() == 1 && isspace(line.at(0))))//this was added
        continue;
    string result = process(line);
    aStruct.field1 = result;
    //...rest of code
}

For what it's worth, it can be assumed there isn't going to be a bizarre mix of white space making an entire line. For example, one line isn't going to be six spaces (maybe one space, by mistake). This solution isn't exactly elegant.

The at() method validates that the input is in the correct range. Since you have already done the validation there is no point in using this method you may as well use operator[]

Additionally there is a call to check for empty strings empty() so prfer to use that rather than test the length against zero.

if (line.empty() || (line.length() == 1 && isspace(line[0])))


Multiple SQL statements in a single transaction Using the following query I'm doing transactions with two tables

BEGIN TRANSACTION
UPDATE SalaryTrans SET carried_forward_amount = @carriedForwardAmount, net_wage=@netWage, processed_date = @processedDate WHERE employee_id=@employeeID AND reference = @reference
DELETE FROM CarriedForward WHERE employee_id = @employeeID
INSERT INTO CarriedForward (employee_id, carried_forward_amount) VALUES(@employeeID, @carriedForwardAmount)
COMMIT;

First SalaryTrans table is updated. Then all matching records in CarriedForward table are deleted and new details are entered.

I just want to know whether this approach is acceptable or there are better ways to write this.

In terms of the raw/basic functionality, what you have is fine. The transactional logic is good.

Readability is the only concern I have, and would rewrite your code as (note, there are some spaces I added around some = conditions):

BEGIN TRANSACTION

    UPDATE SalaryTrans
    SET carried_forward_amount = @carriedForwardAmount,
        net_wage = @netWage,
        processed_date = @processedDate
    WHERE employee_id = @employeeID
      AND reference = @reference

    DELETE FROM CarriedForward
    WHERE employee_id = @employeeID

    INSERT INTO CarriedForward (employee_id, carried_forward_amount)
    VALUES(@employeeID, @carriedForwardAmount)

COMMIT;

I assume the business logic is correct. It is unusual to have a table in a database where deletes happen. I am more accustomed to having some sort of history for the data, and the delete is a logical thing, not a real delete. This is normally for audit/reporting purposes.


Ask for stream of character input and print number of uppercase/lowercase characters Does this code look okay?

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

int case_letters();

int main(void)

{
    case_letters();
    return 0;
}

int case_letters()

{
    int ch;
    int numOfUpper = 0;
    int numOfLower = 0;

    printf("please enter a some characters, and ctrl + d to see result\n");

    while ((ch = getchar()) != EOF)
    {
        if (isdigit(ch))
        {
            printf("please enter a valid character\n");
            continue;
        }

        else if (isupper(ch))
        {
            numOfUpper++;
        }

        else if (islower(ch))
        {
            numOfLower++;
        }
    }

    return  printf("There are %d Uppercase letters, and %d Lowercase letters\n", numOfUpper, numOfLower);
}

It looks unusual to have newlines between the function statement and body since they're associated with each other.  This just adds extra lines and makes the code a little harder to read.
If case_letters() is just supposed to print something at the end and not return a value, it should return void (no value), not int.  main() isn't doing anything with this function after calling it.
Unformatted outputs should use puts() instead of printf().
As @Rhs has mentioned, your program doesn't handle special characters.  Based on this from @Loki Astari, there are additional types of characters and with accompanying testing functions.
According to this answer, CTRLD only works on Mac OS and Linux.  For Windows, it's CTRLZ.  All of this should be stated in the output in case the user is running this on Windows.
Consider making this more modular for increased maintainability:

keep letter counters for final output (in main())
collect each character from user
determine if the character is a letter
increment either counter if a letter is entered

Here's what I came up with:

#include <ctype.h>
#include <stdbool.h> // add this library
#include <stdio.h>

bool valid_char(char ch)
{
    if (isdigit(ch)) return false;

    // include additional non-letter checks...

    // if everything checks out:
    return true;
}

void update_counters(char ch, int* numOfUpper, int* numOfLower)
{
    if (isupper(ch))
    {
        (*numOfUpper)++;
    }
    else if (islower(ch))
    {
        (*numOfLower)++;
    }
}

void get_letters(int* numOfUpper, int* numOfLower)
{
    puts("Please enter some characters\n");
    puts("Press CTRL+D on Mac OS/Linux or CTRL+Z on Windows to see the results\n");
    char ch;

    while ((ch = getchar()) != EOF)
    {
        while (!valid_char(ch))
        {
            puts("Character must be a letter\n");
            ch = getchar();
        }

        update_counters(ch, numOfUpper, numOfLower);
    }
}

int main(void)
{
    int numOfUpper = 0;
    int numOfLower = 0;

    int* upperPtr = &numOfUpper;
    int* lowerPtr = &numOfLower;

    get_letters(upperPtr, lowerPtr);

    printf("There are %d uppercase letters and %d lowercase letters\n", numOfUpper, numOfLower);
}


The parking machine implementation I am interesting in the topic of finite automata, so I started over by implementing a parking machine from this video: Computers Without Memory - Computerphile to understand conceptions. Didn't do much of an error checking - have made a minimal working version.

Short description of the task, but for better understanding watch the video:

You have the parking machine which is implemented by the finite automaton.
The parking lot costs 25 pence. The machine takes only 5, 10, 20 pence coins. Thus, any combination of these coins can be used to pay for parking.
After a sum of coins has reached 25 pence, the ticket is issued.
The machine gobble up all extra money above the 25 pence, like 20 + 20 pence = the ticket only, no change are given.

Questions:

What do you think about my approach?
Is it optimal, extendable, suitable for real world applications or this technique has limiting factors and an another should be used?
How would you implement this machine? It will be good to see another solutions.

It is possible to do this by such way: Explicit state transition table, may be I will do this also. My target is to implement something more serious like a regex engine in the future, but I should read some books/articles before.

#!/usr/bin/python3

def get_coin(current_sum, *functions):
    print(f"Current sum = {current_sum} pence")
    coin = int(input("Insert coin\n"))
    if coin == 5:
        functions[0]()
    elif coin == 10:
        functions[1]()
    elif coin == 20:
        functions[2]()
    else:
        error(current_sum, functions)

def error(current_sum, functions):
    print("Error: only 5, 10, 20 pence coins are allowed\n")
    get_coin(current_sum, *functions)

def zero_pence():
    get_coin("0", five_pence, ten_pence, twenty_pence)

def five_pence():
    get_coin("5", ten_pence, fifteen_pence, twenty_five_pence)

def ten_pence():
    get_coin("10", fifteen_pence, twenty_pence, give_ticket)

def fifteen_pence():
    get_coin("15", twenty_pence, twenty_five_pence, give_ticket)

def twenty_pence():
    get_coin("20", twenty_five_pence, give_ticket, give_ticket)

def twenty_five_pence():
    input("Current sum = 25 pence, press the 'return' button to pay")
    give_ticket()

def give_ticket():
    print("""\nTake your ticket:

Date: 1 November 2019
Start time: \t20:21
End time: \t22:21\n
""")

def parking_machine():
    prompt = """\n\tThe parking machine.

Information:
1. The machine takes 5, 10, 20 coins.
2. The machine doesn't give a change.
3. The parking costs 25 pence.

Press 's' button to start inserting of coins.

"""
    prompt = '#' * 80 + prompt + '#' * 80 + '\n'

    while True:
        button = input(prompt)
        if button == 's':
            zero_pence()

parking_machine()

Not a finite state machine

The state machine in the posted code has infinite states.
This is caused by the recursive calls of get_coin and error.
Consider for example these states:

zero-pence with nothing entered yet
zero-pence, after zero-pence and an invalid input
zero-pence, after zero-pence and an invalid input twice
...

And so on. You can see these are different states,
because the content of the stack is different.

There are theoretically infinite zero-pence, five-pence, etc, states.
Practically there are finite, because after enough invalid inputs,
the stack will eventually overflow.

There should be precisely one zero-pence, five-pence, etc, states.

This is easy to fix by replacing the error function with a loop inside get_coin.

There are too many states

Even after making the states finite,
there will still be more states than required by the exercise.

Why, again, because of the stack.
For example, there are two ways to reach 10 pence:

0 -> 5 -> 5
0 -> 10

At this point, the machine should be in the 10 pence state,
but these two states are not the same,
because the stack stores two different histories.

If you want truly identical ten-pence, fifteen-pence, etc, states,
you have to eliminate history, you have to eliminate stack,
you have to eliminate function calls.

This is easy to fix by implementing state transitions in a loop,
instead of through function calls.

Don't use varargs if you don't need it

The get_coin function takes a variable number of functions as arguments.
But in fact it will only ever use 3,
and in fact it's only ever called with 3.
Therefore, it would be more natural to give those parameters dedicated, descriptive names.

Why convert input to int?

The get_coin function converts input to int,
but doesn't actually perform any numeric operations with it.

Use a more portable shebang

Not all systems have Python 3 installed in /usr/bin/python3.
A more robust, portable shebang would be:

#!/usr/bin/env python3


Calculating sum of primes using the CPU and GPU This is a little baffling to me as to why the CUDA code runs about twice as slow as the CPU version. I am just counting all the primes from 0 to (512 * 512 * 512). The CPU version executed in about 97 seconds whereas the GPU version took 182 seconds.

CPU: Intel Core i7 @ 4 GHz
GPU: NVIDIA GTX 960

Any ideas why?

#include <cuda.h>
#include <iostream>
#include <cstdint>
#include <stdio.h>
#include <ctime>
#include <vector>
#include <cstdlib>
#include <climits>

using namespace std;

__host__ __device__  bool is_prime(uint32_t n)
{
    if(n == 2)
        return true;
    if(n % 2 == 0)
        return false;
    uint32_t sr = sqrtf(n);

    for(uint32_t i = 3; i <= sr; i += 2)
        if(n % i == 0)
            return false;
    return true;
}

__global__ void prime_sum(unsigned int* count)
{
    uint32_t n = (blockIdx.y * gridDim.y + blockIdx.x) * blockDim.x + threadIdx.x;
    if(is_prime(n))
        atomicAdd(count, 1);
}

CPU version

int main()
{
    time_t start = time(0);
    int pcount = 0;
    for(uint32_t i = 0; i < (512 * 512 * 512); i++)
    {
        if(is_prime(i)) pcount++;
    }
    start = time(0) - start;
    std::cout << pcount << "\t" << start << std::endl;

    return 0;
}

CUDA version

int main()
{
    time_t start = time(0);
    unsigned int* sum_d;
    cudaMalloc(&sum_d, sizeof(unsigned int));
    cudaMemset(sum_d, 0, sizeof(unsigned int));

    prime_sum<<< dim3(512, 512), 512 >>>(sum_d);

    unsigned int sum = 0;
    cudaMemcpy(&sum, sum_d, sizeof(unsigned int), cudaMemcpyDeviceToHost);
    start = time(0) - start;
    std::cout << sum << "\t" << start << std::endl;
    cudaFree(sum_d);

    return 0;
}

Try not to use using namespace std.
This is lacking in curly braces and could be bad for maintenance:

if(n == 2)
    return true;
if(n % 2 == 0)
    return false;
uint32_t sr = sqrtf(n);

for(uint32_t i = 3; i <= sr; i += 2)
    if(n % i == 0)
        return false;
return true;

It's a good idea to still use them for single-line statements, and you should especially use them with for loops.  Some extra whitespace in places could be useful as well.

Here's what it should look like:

if (n == 2)
{
    return true;
}
if (n % 2 == 0)
{
    return false;
}

uint32_t sr = sqrtf(n);

for (uint32_t i = 3; i <= sr; i += 2)
{
    if (n % i == 0)
    {
        return false;
    }
}

return true;

Consider finding an alternative to sqrtf() as it can be slow.  It may not make a huge difference in overall runtime, though.  Fortunately, it appears to be thread-safe.
One source of slower performance may be the if statement in prime_sum().  Doing so may cause thread divergence, if threads end up executing different instructions.  You'll have to analyze your code to determine this for sure.
This line seems confusing:

start = time(0) - start;

It appears that the lvalue should instead be named end since you already calculate a starting time beforehand.


DestroyWindow() non-existant windows What I am working on has sets of window controls. Controls are constantly being either destroyed, or created.

void ChangeControl() {

// remove all controls, existing, or not
hwnd = GetDlgItem(hWnd, IDC_BTN_FOOA);
DestroyWindow(hwnd);
hwnd = GetDlgItem(hWnd, IDC_BTN_FOOB);
DestroyWindow(hwnd);
hwnd = GetDlgItem(hWnd, IDC_BTN_FOOC);

// add the control wanted
hwndButton = CreateWindow("BUTTON", "Start", WS_VISIBLE|WS_CHILD|BS_OWNERDRAW|BS_PUSHBUTTON,
        left, top, BUTTON_WIDTH, BUTTON_HEIGHT, hWnd, (HMENU)IDC_BTN_FOOB, GetModuleHandle(NULL), NULL);
}

Test case:

DWORD errorResult;
testHwnd = GetDlgItem(hWnd, IDC_BTN_START);
if (testHwnd) {
    DestroyWindow(hwnd);
}
else {
    errorResult = GetLastError();
}

I am wondering if this method is safe, or if I should be checking if a control exists before calling DestroyWindow().

If the control doesn't exist then GetDlgItem returns NULL. Passing NULL to DestroyWindow should be "save" in the sense that it won't crash - it will return FALSE and GetLastError will be returning an "Invalid Handle" error (or something like that) (can't find any docs for it but that's the behaviour I've seen in the past).

A remark to your code:

hwnd = GetDlgItem(hWnd, IDC_BTN_FOOA);
DestroyWindow(hwnd);

The use of two different variables which only differ in the capitalization of a single letter is ugly and error prone and you should use a different name. One convention I tend to follow if I select objects which are to be destroyed is to call them doomed. So maybe something like this:

doomedHandle = GetDlgItem(hWnd, IDC_BTN_FOOA);
DestroyWindow(doomedHandle);


Simplifying a LINQ expression that finds the largest date smaller than another date I have a LINQ 2 SQL query that needs to run a subquery in the where clause. The subquery just returns the largest date (from a date column) that is less than or equal to another date (an input to the function).

I originally had it like this (not this in the where clause of an outer query):

skew.CalibrationDate.Date == (from skew2 in db.Skew
                              where skew2.CalibrationDate.Date <= date.Date
                              select skew2.CalibrationDate).Max()

And then simplified it to:

skew.CalibrationDate.Date == db.Skew.Select(s => s.CalibrationDate.Date)
                                    .Where(s => s <= date.Date)
                                    .Max()

(note that the db.Skew table/object has multiple columns/properties but I'm only interested in the CalibrationDate for this clause and also that date is an input to the function that this is in, a regular DateTime variable).

Is it necessary for me to have all three of the Select(), Where() and Max() calls or is there some redundancy here? Could I put a lambda expression in the Max() call to get rid of one of the others?

Is this what you are looking for?

skew.CalibrationDate.Date == db.Skew
    .Where(s => s.CalibrationDate.Date <= date.Date)
    .Max(s => s.CalibrationDate.Date)

Max will return the maximum value itself, not the object with the maximum date.


Database model for a hotel I have to create a Hotel database according to these requirements:

information about guests should be found, information about rooms divided into categories, reservations (ordered by a type of a room and assigned for a specific type of a room).
the following functionality should be provided: make a reservation by assigning a free room with the guests expectations - if you cannot meet guest's expectations, you have to save booking order to be settled by authorized employees
check how many times given person was a guest in last 2 years
which guests came to the hotel, accordingly to the reservation
free the room and write the bill

My code:

CREATE TABLE client_reservation (
    client_id        INTEGER NOT NULL,
    reservation_id   INTEGER NOT NULL
);

ALTER TABLE client_reservation ADD CONSTRAINT client_reservation_pk PRIMARY KEY ( client_id,
reservation_id );

CREATE TABLE clients (
    client_id      INTEGER NOT NULL,
    first_name     VARCHAR2(50) NOT NULL,
    last_name      VARCHAR2(100) NOT NULL,
    street         VARCHAR2(50),
    house_number   VARCHAR2(6),
    town           VARCHAR2(100),
    postal_code    VARCHAR2(6),
    telephone      VARCHAR2(15) NOT NULL
);

ALTER TABLE clients ADD CONSTRAINT clients_pk PRIMARY KEY ( client_id );

CREATE TABLE reservations (
    reservation_id        INTEGER NOT NULL,
    date_of_purchase      DATE,
    beggining             DATE,
    ending                DATE,
    status                VARCHAR2(10) NOT NULL,
    date_of_reservation   DATE NOT NULL
);

ALTER TABLE reservations ADD CONSTRAINT reservation_pk PRIMARY KEY ( reservation_id );

CREATE TABLE resevation_rooms (
    reservation_id   INTEGER NOT NULL,
    room_id          INTEGER NOT NULL
);

ALTER TABLE resevation_rooms ADD CONSTRAINT resevation_rooms_pk PRIMARY KEY ( reservation_id,
room_id );

CREATE TABLE room_category (
    category_id   INTEGER NOT NULL,
    name          VARCHAR2(50) NOT NULL,
    description   VARCHAR2(200)
);

ALTER TABLE room_category ADD CONSTRAINT room_category_pk PRIMARY KEY ( category_id );

CREATE TABLE rooms (
    room_id         INTEGER NOT NULL,
    capacity        INTEGER NOT NULL,
    price_per_day   NUMBER(10) NOT NULL,
    room_category   INTEGER NOT NULL
);

ALTER TABLE rooms ADD CONSTRAINT room_pk PRIMARY KEY ( room_id );

ALTER TABLE client_reservation
    ADD CONSTRAINT client_fk FOREIGN KEY ( client_id )
        REFERENCES clients ( client_id );

ALTER TABLE client_reservation
    ADD CONSTRAINT reservation_fk FOREIGN KEY ( reservation_id )
        REFERENCES reservations ( reservation_id );

ALTER TABLE resevation_rooms
    ADD CONSTRAINT reservation_rooms_fk FOREIGN KEY ( reservation_id )
        REFERENCES reservations ( reservation_id );

ALTER TABLE rooms
    ADD CONSTRAINT room_category_fk FOREIGN KEY ( room_category )
        REFERENCES room_category ( category_id );

ALTER TABLE resevation_rooms
    ADD CONSTRAINT room_reservation_fk FOREIGN KEY ( room_id )
        REFERENCES rooms ( room_id );

Model:

Thanks for every suggestions, cause I'm not so sure if I meet requirements and put right relations (identifying or nonidentifying)

I think, you don't need tables client_reservation and reservation_rooms. You have tables clients and rooms, and then, to make a reservation, you need only one table - reservation (I prefer to give table names in singular form). This table will reference to a client and to a room, and the reservation period (start date/end date). Also, you forgot about the actual state of the room - it could be just booked or busy. I would propose to create a new table - status, and reference it from reservation. The table status could contain the following values:

booked - when a client booked the room
busy - when a client came to the hotel
canceled - when a client didn't come at time and room became available for other guests
unavailable - for example, when a room needs a renovation and/or repair of equipment
etc.

Also, a billing is mentioned in the task. It is a very wide area, to fully support it, you may need a lot of tables to store the data about billing plans (a room can have different prices depending on a season, a category, available equipment, a client type, a view from the window, the number of the floor, etc.).


Attempt to reconnect, with timeout I have a situation in which a program, communicating over the network, can timeout or otherwise fail to establish a connection through no fault of my application or its user. If and when this happens, an exception is thrown. The solution is obvious; simply try again a couple more times, and if multiple tries don't work then this is really a problem.

I've accomplished this by wrapping the original try-catch in a while loop with a retry counter, producing the following basic pattern:

var retryCount = 0;
while(true)
{
   try
   {
      AttemptToConnect();
      break;
   }
   catch(TimeoutException tex)
   {
      if(++retryCount < 3) continue;

      throw; //or handle error and break/return
   }
}

Do any of you see any problems? The biggest smell, prompting me to ask in the first place, is the conditionless loop, which would be infinite except that the "happy path" breaks out of the loop manually, while the terminating error will throw out. This has been called out before as a potential bug generator, if it's ever modified in the future (for instance to catch a second type of exception, or to do something else that could throw an exception).

Yeah, I'd probably redo it with an actual condition. That way, intent looks clear and is not dependent on internal code to break, continue, etc.

const int NumberOfRetries = 3;
var retryCount = NumberOfRetries;
var success = false;
while(!success && retryCount > 0)
{
   try
   {
      AttemptToConnect();
      success = true;
   }
   catch(TimeoutException tex)
   {
      retryCount--;

      if (retryCount == 0)
      {
          throw; //or handle error and break/return
      }
   }
}


use trie data structure to solve the problem of clustering anagrams Given an array of words, print all anagrams together. For example, if
  the given array is {“cat”, “dog”, “tac”, “god”, “act”}, then output
  may be “cat tac act dog god”.

The following is my c++ code. I use raw pointer to implement varying number of children
for practice. Actually, I guess maybe it's better to adopt STL containers.

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

struct Index_node_base
{
    Index_node_base* next;
    Index_node_base():next(NULL){}
};

struct Index_node : public Index_node_base
{
    int index;
    Index_node(int i):index(i), Index_node_base(){}
};

void insert_index_node(Index_node** head, int i)
{
    Index_node* node = new Index_node(i);
    node->next = (*head);
    *head = node;
}

struct Trie_node_base
{
    typedef Trie_node_base* Base_ptr;

    Base_ptr* child;
    int num_of_child;

    Trie_node_base(int n)
    {
        child = new Base_ptr[n];
        for(int i=0; i < n; ++i)
            child[i] = NULL;
        num_of_child = n;
    }

    ~Trie_node_base()
    {
        delete [] child;
    }
};

struct Trie_node : public Trie_node_base
{
    bool is_end;
    Index_node* head;
    Trie_node(int n):Trie_node_base(n), is_end(false), head(NULL){}
};

class Trie
{
public:
    Trie(int n=26)
    {
        root = new Trie_node(n);
    }
    ~Trie()
    {
        destroy(root);
    }

    void insert_trie_node(const char* w, int ind);
    Trie_node* get_root_node();

private:
    Trie_node* root;
    void destroy(Trie_node* root);
};

Trie_node* Trie::get_root_node()
{
    return root;
}

void Trie::insert_trie_node(const char* w, int ind)
{
    Trie_node* r = root;
    while(*w)
    {
        int i = *w - 'a';
        if(r->child[i] == NULL)
        {
            r->child[i] = new Trie_node(r->num_of_child);
        }
        r = (Trie_node*)r->child[i];
        ++w;
    }

    if(r->is_end)
    {
        insert_index_node(&(r->head), ind);
    }
    else
    {
        r->is_end = true;
        r->head = new Index_node(ind);
    }
}

void traversal_trie(const char* word_arr[], Trie_node* r)
{
    size_t i;

    if(r->is_end)
    {
        Index_node* h = r->head;
        while(h != NULL)
        {
            printf("%s\n", word_arr[h->index]);
            h = (Index_node*)h->next;
        }
    }

    for(i = 0; i < r->num_of_child; ++i)
    {
        if(r->child[i] != NULL)
            traversal_trie(word_arr, (Trie_node*)r->child[i]);
    }
}

void Trie::destroy(Trie_node* root)
{
    size_t i;

    for(i = 0; i < root->num_of_child; ++i)
    {
        if(root->child[i] != NULL)
            destroy((Trie_node*)root->child[i]);
    }

    if(root->is_end)
    {
        Index_node* h = root->head;
        Index_node* tmp = NULL;

        while(h != NULL)
        {
            tmp = (Index_node*)h->next;
            delete h;
            h = tmp;
        }
    }

    delete root;
}

static int comp_char(const void* x, const void* y)
{
    const char* c1 = (const char*)x;
    const char* c2 = (const char*)y;

    return *(c1) - *(c2);
}

static void cluster_anagrams(const char* word_arr[], size_t size)
{
    Trie* trie = new Trie(26);
    size_t i;
    char* buffer;

    for(i = 0; i < size; ++i)
    {
        int len = strlen(word_arr[i]);
        buffer = new char [len+1];
        memcpy(buffer, word_arr[i], len+1);
        qsort(buffer, strlen(buffer), 1, comp_char);
        trie->insert_trie_node(buffer, i);
        delete buffer;
    }

    cout << "Debug!" << endl;
    traversal_trie(word_arr, trie->get_root_node());
}

int main()
{
    const char* word_arr[] = {"cat", "dog", "tac", "god", "act", "gdo"};
    size_t size = sizeof(word_arr) / sizeof(word_arr[0]);
    cluster_anagrams(word_arr, size);

    system("pause");
    return 0;
}

#include <cstring>

Avoid including C headers when programming C++.

struct Index_node_base
{
    Index_node_base* next;
    Index_node_base():next(NULL){}
};

struct Index_node : public Index_node_base
{
    int index;
    Index_node(int i):index(i), Index_node_base(){}
};

void insert_index_node(Index_node** head, int i)
{
     Index_node* node = new Index_node(i);
     node->next = (*head);
     *head = node;
}

This is overkill as you are just re-implementing a list of numbers / characters.

const char*

This is C code, consider using the C++ std::string instead.

const char* word_arr[]

For example, this one should be a std::list<std::string> (a.k.a. list<string>)

 printf("%s\n", word_arr[h->index]);

Again C, use the C++ iostreams instead. This should be like cout << ... << endl;

while(*w)
{
    int i = *w - 'a';
    if(r->child[i] == NULL)
    {
        r->child[i] = new Trie_node(r->num_of_child);
    }
    r = (Trie_node*)r->child[i];
    ++w;
}

You can use iterators and STL algorithms to do this in one line.

for(i = 0; i < r->num_of_child; ++i)
{
    if(r->child[i] != NULL)
        traversal_trie(word_arr, (Trie_node*)r->child[i]);
}

This would also be one line using for_each on iterators and passing traversal_trie.

void Trie::destroy(Trie_node* root)

You won't need destory anymore as a result; also, there's a chance you might not need pointers.

static int comp_char(const void* x, const void* y)
{
    const char* c1 = (const char*)x;
    const char* c2 = (const char*)y;

    return *(c1) - *(c2);
}

This is messsy, you are doing useless C style voids and casts and not using C++ overloading. This body would have been something along the line of just the line return c1 < c2;

for(i = 0; i < size; ++i)
{
    int len = strlen(word_arr[i]);
    buffer = new char [len+1];
    memcpy(buffer, word_arr[i], len+1);
    qsort(buffer, strlen(buffer), 1, comp_char);
    trie->insert_trie_node(buffer, i);
    delete buffer;
}

memcpy? Not necessary, std::string allows you to copy the string and not mess with memory.


Are the words isomorph? Are the words isomorphs? (Code-Golf)

This is my non-golfed, readable and linear (quasi-linear?) in complexity take of the above problem. For completeness I include the description:

  Two words are isomorphs if they have the same pattern of letter
  repetitions. For example, both ESTATE and DUELED have pattern abcdca

ESTATE
DUELED
abcdca

  because letters 1 and 6 are the same, letters 3 and 5 are the same,
  and nothing further. This also means the words are related by a
  substitution cipher, here with the matching E <-> D, S <-> U, T <-> E, A <-> L.

  Write code that takes two words and checks whether they are isomorphs.

As always tests are included for easier understanding and modification.

def repetition_pattern(text):
    """
    Same letters get same numbers, small numbers are used first.
    Note: two-digits or higher numbers may be used if the the text is too long.

    >>> repetition_pattern('estate')
    '012320'
    >>> repetition_pattern('dueled')
    '012320'
    >>> repetition_pattern('longer example')
    '012345647891004'

    #    ^  ^      ^  4 stands for 'e' because 'e' is at 4-th position.
    #           ^^ Note the use of 10 after 9.
    """
    for index, unique_letter in enumerate(sorted(set(text), key=text.index)):
        text = text.replace(unique_letter, str(index))
    return text

def are_isomorph(word_1, word_2):
    """
    Have the words (or string of arbitrary characters)
    the same the same `repetition_pattern` of letter repetitions?

    All the words with all different letters are trivially isomorphs to each other.

    >>> are_isomorph('estate', 'dueled')
    True
    >>> are_isomorph('estate'*10**4, 'dueled'*10**4)
    True
    >>> are_isomorph('foo', 'bar')
    False
    """
    return repetition_pattern(word_1) == repetition_pattern(word_2)

for index, unique_letter in enumerate(sorted(set(text), key=text.index)):
    text = text.replace(unique_letter, str(index))
return text

You don't need to go through the set and sort, you can do:

for index, letter in enumerate(text):
    text = text.replace(letter, str(index))
return text

By the time it gets to the last "e" in "estate" and tries to replace it with "5", it will already have replaced it with "0" earlier on, and there won't be any "e"s left to replace. And it's not changing something while iterating over it, because strings are immutable.

More golf-ish and less readable is:

return ''.join(str(text.index(letter)) for letter in text)

Edit: Note for posterity: the above two methods have the same bug Gareth Rees identified, comparing 'decarbonist' and 'decarbonized', where one ends by adding number 10 and the other is a character longer and ends by adding numbers 1, 0 and they compare equally.End Edit

But @jonsharpe's comment asks if you need it to return a string; if you don't, then it could be a list of numbers, which is still very readable:

return [text.index(letter) for letter in text]

Minor things:

are_isomorph is a mix of plural/singular naming. Maybe isomorphic(a, b)?
Your code doesn't take uppercase/lowercase into account - should it?
This approach will break without warning for an input with numbers in it, e.g.

repetition_pattern('abcdefghi1') is 0923456789. "b" changes to 1, then changes to 9 because the "1" at the end catches it, then it appears the two were originally the same character. (It might be outside the scope of the question and not a problem).


Lindenmayer System String Generator in F# Lindenmayer Systems or L-systems are fractals that can be constructed by applying replacement rules to a string for a certain amount of iterations. For example:

Axiom (start): "FX"
Rules:
  X -> "X+YF+"
  Y -> "-FX-Y"

Iteration 0: FX
Iteration 1: FX+YF+
Iteration 2: FX+YF++-FX-YF+
Iteration 3: FX+YF++-FX-YF++-FX+YF+--FX-YF+

 F means draw forward
 + means turn clockwise 90 degrees
 - means turn counter-clockwise 90 degrees
 Everything else is ignored

So each can be simplified to:
 0: F
 1: F+F+
 2: F+F+F-F+
 3: F+F+F-F+F+F-F-F+

The goal of my program is to print out those simplified strings given the axiom, rules, and iteration

module LSystem

let getLSystem axiom rules i =
    let rec find i =
        let replace (str:string)=
            str
            |> List.ofSeq
            |> List.map (fun x ->
                match List.tryFind (fun (c,_) -> c = x) rules with
                | Some (_,r) -> r
                | None -> x.ToString() )
            |> List.reduce (+)
        match i with
        | 0 -> axiom
        | _ -> i-1 |> find |> replace
    let simplify (s:string) = s.Replace("X","").Replace("Y","").Replace("+++","-").Replace("---","+").Replace("+-","").Replace("-+","")
    i |> find |> simplify

And something to run it:

open LSystem

let getDragonCurve =
    let axiom = "FX"
    let rules  = [
        ('X',"X+YF+");
        ('Y',"-FX-Y")]
    getLSystem axiom rules

[for i in 0..5 -> getDragonCurve i]
|> List.iter (fun x -> printf "%s\n" x)

System.Console.ReadKey() |> ignore

I'm specifically concerned about the simplify function, as using straight up C# methods is often a sign you're doing things wrong. I'm quite new to F#, so any style-centric tips are greatly appreciated.

To simplify you should just split at the F's and replace each section of +, -, X and Y characters with a string of +, a string of - or the empty string depending what the final balance of +/- characters are. Then you will only be making one pass over the string.

Consider what happens with your approach if the string to be simplified is ++---. If I understand your code correctly you end up with +++ which you would like to represent as -. In order to do this, however, you have to make another pass over the string.

Using my phone right now. When I get to a real computer I'll update with some code.

Update: You can see a Haskell implementation of simplify here: (link). It should readily translate to F#.