Method for returning valid URLs from a sitemap URL I need a method which fetches all of the text from a URL (generally a sitemap URL), and returns an IEnumerable of all valid URLs contained in the text returned from the initial address. What I have so far is:

public IEnumerable<Uri> GetSitemapUrls(Uri sitemapUrl)
{
    var sitemapText = GetSitemapText(sitemapUrl);

    if (string.IsNullOrWhiteSpace(sitemapText))
        yield break;

    var urls = new List<string>();

    var urlRegex = new Regex(@"\b(?:https?://|www\.)[^ \f\n\r\t\v\]]+\b", RegexOptions.Compiled | RegexOptions.IgnoreCase);

    foreach (Match m in urlRegex.Matches(sitemapText))
        urls.Add(CleanUriString(m.Value));

    foreach (var url in urls)
    {
        var cleanedUriString = CleanUriString(url);

        if (Uri.IsWellFormedUriString(cleanedUriString, UriKind.RelativeOrAbsolute))
            yield return new Uri(cleanedUriString);
    }
}

string GetSitemapText(Uri sitemapUri)
{
    var wc = new WebClient
    {
        Encoding = System.Text.Encoding.UTF8
    };

    return wc.DownloadString(sitemapUri);
}

string CleanUriString(string dirtyUriString)
{
    var legalCharacters = @"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~:/?#[]@!$&'()*+,;=".ToCharArray();

    var cleanedString = dirtyUriString;

    foreach (var character in dirtyUriString)
    {
        var matchIndex = dirtyUriString.IndexOf(character);

        if (!legalCharacters.Any(x => x.Equals(character)) && matchIndex > 0)
            cleanedString = dirtyUriString.Substring(0, matchIndex);
    }

    return cleanedString;
}

It seems to work as intended for actual sitemaps, null/empty responses from the URL it receives, and URLs with URL-illegal characters in them. I have a feeling I'm missing out on potential issues or letting bad URLs through anyway, not to mention I haven't thought about spidering through URLs that the initial sitemap (if it is a sitemap) returns.

Is there anything I can do to improve it?

Just a couple of thougths...

if (string.IsNullOrWhiteSpace(sitemapText))
  yield break;

This is a very unusual handling of null/empty argument values and should be documented. A more meaniningful name like GetSitemapUrlsOrDefault would also help. Without it the expected way is to throw an ArgumentException.

foreach (Match m in urlRegex.Matches(sitemapText))
  urls.Add(CleanUriString(m.Value));

foreach (var url in urls)
{
  var cleanedUriString = CleanUriString(url);

  if (Uri.IsWellFormedUriString(cleanedUriString, UriKind.RelativeOrAbsolute))
      yield return new Uri(cleanedUriString);
}

You don't need all this. Instead you can build a nice query that does everything in one go and in a single run:

return
    urlRegex
        .Matches(sitemapText)
        .Cast<Match>()
        .Select(m => CleanUriString(m.Value))
        .Where(cleanedUriString => Uri.IsWellFormedUriString(cleanedUriString, UriKind.RelativeOrAbsolute))
        .Select(cleanedUriString => new Uri(cleanedUriString));

or alternatively:

return
    from m in urlRegex.Matches(sitemapText).Cast<Match>()
    let cleanedUriString = CleanUriString(m.Value)
    where Uri.IsWellFormedUriString(cleanedUriString, UriKind.RelativeOrAbsolute)
    select new Uri(cleanedUriString);

You don't need the .ToCharArray(); on the legalCharacters string. A string is already a char-array.

Oh, and of course the WebClient needs to be disposed. Wrapping it with a using would be good.


Converting click count data to JSON I have an array filled with 5 other arrays that consists of arrays of 2 values (first a date then a count):

[
  [
    [dt, cnt], [dt, cnt], ....
  ],
  [
    [dt, cnt], [dt, cnt], ....
  ],
  [
    [dt, cnt], [dt, cnt], ....
  ],
  [
    [dt, cnt], [dt, cnt], ....
  ],
  [
    [dt, cnt], [dt, cnt], ....
  ],
]

It concerns click statistics of different websites. This data needs to be converted to data to make a consistent chart with Google visualisations. So first a conversion is done in Python, then this result converted to JSON to pass to the Google libs.

My current code is this:

# determine min date
mindate = datetime.date.max
for dataSet in sets:
    if (dataSet[0][0] < mindate):
        mindate = dataSet[0][0];
# fill a dictionary with all dates involved
datedict = {}
for dat in daterange(mindate, today):
    datedict[dat] = [dat];
# fill dictionary with rest of data
arrlen = 2
for dataSet in sets:
    # first the values
    for value in dataSet:
        datedict[value[0]] = datedict[value[0]] + [value[1]];
    # don't forget the missing values (use 0)
    for dat in daterange(mindate, today):
        if len(datedict[dat]) < arrlen:
            datedict[dat] = datedict[dat] + [0]
    arrlen = arrlen + 1
# convert to an array
datearr = []
for dat in daterange(mindate, today):
    datearr = datearr + [datedict[dat]]
# convert to json
result = json.dumps(datearr, cls=DateTimeEncoder)

(The DateTimeEncoder just gives a JavaScript-friendly datetime in the JSON)

The output looks like this:

[
    ["new Date(2008, 7, 27)", 0, 5371, 1042, 69, 0],
    ["new Date(2008, 7, 28)", 0, 5665, 1100, 89, 0],
    ...
]

This is one of my first Python adventures, so I expect this piece of code can be improved upon easily. I want this to be shorter and more elegant. Show me the awesomeness of Python because I'm still a bit disappointed.

I'm using Django, by the way.

Trailing semicolons

At many places there is a ; at the end of the line.
It's completely unnecessary in Python and you should remove those.

Appending to arrays

At several places you are appending values to arrays in a strange way, for example:

    datedict[val[0]] = datedict[val[0]] + [val[1]];

The common and shorter way to do this is using .append():

    datedict[val[0]].append(val[1])

Augmented assignment operator +=

Instead of:

l = l + 1

This is shorter and better:

l += 1

Use list comprehensions

Instead of:

datearr = []
for dat in daterange(mindate, today):
    datearr = datearr + [datedict[dat]]

You can use a list comprehension, a powerful feature of Python:

datearr = [datedict[dat] for dat in daterange(mindate, today)]


Pruning logs, burning the remains After implementing some logging in my system, I got tired of digging through very long files trying to determine where specific runs started/stopped to look for differences, so I began adding a date/time stamp to the file name so I'd get a new log file each time the processing kicked off. This was wonderful, but solving one problem created a new one - now I have a proliferation of log files and I have to keep the collection of files under control.

I've written the following procedure to do the pruning and it works quite well. I'm wondering if there are any gotcha's I may have missed in my extensive (2 hours of) testing, or if there are any more efficient ways of doing this.

I believe the implementation is fairly efficient. There's no need to keep a list of the naxLogsToKeep newest (or oldest) files in memory, only the one I'm looking at now and the one known to be the oldest I've looked at so far. By not having an array, collection, dictionary or other structure of all the files, there's no need to sort anything, so this should be fairly efficient. But, I don't know it all, so I'm open to suggestions.

There are some situations that I'm aware of:

What happens if filePath doesn't have a trailing slash? It's already been checked in the calling procedure. I suppose it should be checked here again, just in case I ever reuse this code.
The error handling is... weak. Yup. I threw something in there just for giggles and grins, but will probably remove it and let it bubble up to be dealt with at a higher level.
It doesn't work well if there are more than maxLogsToKeep logs. Since Dir() gives you no indication whatsoever of what order you'll get files, if there are extra files than this will delete the oldest one(s) it's come across when it hits maxLogsToKeep. I'm pretty sure, in my limited testing, that it will work just fine, once there are only maxLogsToKeep log files in the given directory.

With that, the code:

Private Sub DeleteExtraLogFiles(ByVal filePath As String, ByVal fileNamePattern As String, ByVal maxLogsToKeep As Long)

  On Error GoTo ErrorHandler

  Dim fileCount As Long

  Dim oldestFileDate As Date
  oldestFileDate = #12/31/3999#

  Dim currentFile As String
  currentFile = Dir(filePath & fileNamePattern)

  Do While Len(currentFile) > 0
    fileCount = fileCount + 1
    Dim currentFileDate As Date
    currentFileDate = FileDateTime(filePath & currentFile)
    If fileCount < maxLogsToKeep Then
      If currentFileDate < oldestFileDate Then
        Dim oldestFile As String
        oldestFile = currentFile
        oldestFileDate = currentFileDate
      End If
    Else
      If currentFileDate < oldestFileDate Then
        Kill filePath & currentFile
      Else
        Kill filePath & oldestFile
        oldestFile = currentFile
        oldestFileDate = currentFileDate
      End If
    End If
    currentFile = Dir
  Loop

CleanExit:
  Exit Sub

ErrorHandler:
  With Err
    MsgBox "Error: " & .Number & vbCrLf & .Description
  End With
  Resume CleanExit

End Sub

Let's create a scenario. You have lots of log files to prune - some from January, February and March. You have, for whatever reason, way more than maxLogsToKeep. I can see a possibility that your routine would delete the February files and keep the January ones because you do not sort and you trust the order in which Dir returns the files. However, this may not be important to you.

Firstly, reference Microsoft Scripting Runtime in your VBA project. This gives you access to the FileSystemObject and related classes. This gives you more helper functions that will make the code easier to read and maintain (e.g. the File class allows you to access file information without additional reference to the file path; and a File.Delete).

The only way around that scenario I noted is to sort. While the FSO has a Files collection, I don't see any additional functionality that allows you to sort the collection, so you can create your own collection (array in VBA for sorting) and sort that.

Some web references (links are now old - while the fundamentals stay the same the coding could take advantage of newer functionality in VBA):

https://forums.overclockers.com.au/threads/vba-retrieve-files-names-in-order.863553/
http://www.vb-helper.com/howto_dir_quicksorted.html
https://docs.microsoft.com/en-us/dotnet/visual-basic/programming-guide/language-features/arrays/
http://www.cpearson.com/excel/CollectionsAndDictionaries.htm

If I was writing this, then I would traverse the Files collection and build the Array (of File) as I went (the Files.Count will let you dimension the array properly in the first place) but perhaps a quick sort would be more efficient. After you have built the sorted array, you can traverse the array from the end until maxLogsToKeep and delete the files (File.Delete) (*).

Yes, a little more complicated, but addresses a scenario where you might retain ancient logs at the expense of more recent ones.

(*) I know I am working with an array rather than a For Each or other collection enumeration, but I now have a habit of working from the end when deleting things.


Many event listeners Is there an alternative possibly cleaner way (without so much repetition) to write the following? I'm thinking about the headache it's going to be when I have to remove all of my even listers.

customCursor() {
const prevButton = document.querySelector('.previous');
const nextButton = document.querySelector('.next');
const brandLogo = document.querySelector('.brand-logo');
const navButton = document.querySelector('.nav-button');
const carouselCellLink = document.querySelectorAll('.carousel-cell__link');

const cursorContainer = this.cursorContainer;
const cursorOffset = {
  left : 50,
  top  : 50
}

document.querySelector('.page-flickity').addEventListener("mousemove", function (e) {
  cursorContainer.style.left = (e.pageX - cursorOffset.left) + 'px';
  cursorContainer.style.top = (e.pageY - cursorOffset.top) + 'px';
}, false);

prevButton.addEventListener('mouseover', () => {
  cursorContainer.classList.add('previous');
});

prevButton.addEventListener('mouseleave', () => {
  cursorContainer.classList.remove('previous');
});

nextButton.addEventListener('mouseover', () => {
  cursorContainer.classList.add('next');
});

nextButton.addEventListener('mouseleave', () => {
  cursorContainer.classList.remove('next');
});

brandLogo.addEventListener('mouseover', () => {
  cursorContainer.classList.add('hidden');
});

brandLogo.addEventListener('mouseleave', () => {
  cursorContainer.classList.remove('hidden');
});

navButton.addEventListener('mouseover', () => {
  cursorContainer.classList.add('hidden');
});

navButton.addEventListener('mouseleave', () => {
  cursorContainer.classList.remove('hidden');
});

for(var i = 0; i < carouselCellLink.length; i++) {
  carouselCellLink[i].addEventListener('mouseover', () => {
    cursorContainer.classList.add('link');
  });

  carouselCellLink[i].addEventListener('mouseleave', () => {
    cursorContainer.classList.remove('link');
  });
}
}

Somewhat obvious rule: the good way to avoid repetition is always to factorize as much as possible.

Here this can be achieved for all but page-flickity of your targets, which are almost identical: only involved elements and added/removed class-names vary.
It already reduces the code to add listeners, like this:

function customCursor() {
  const
    todo = {
      'previous': [false, 'previous'],
      'next': [false, 'next'],
      'brand-logo': [false, 'hidden'],
      'nav-button': [false, 'hidden'],
      'carousel-cell__link': [true, 'link']
    },
    events = {'mouseover': true, 'mouseleave': false},
    cursorContainer = this.cursorContainer,
    cursorOffset = {left : 50, top  : 50};

  document.querySelector('.page-flickity').addEventListener(
    'mousemove',
    function (e) {
      cursorContainer.style.left = (e.pageX - cursorOffset.left) + 'px';
      cursorContainer.style.top = (e.pageY - cursorOffset.top) + 'px';
    },
    false
  );

  var className, action, elements, i, n;
  for (className in todo) {
    action = todo[className];
    elements = action[0]
      ? document.querySelectorAll(className)
      : [document.querySelector(className)];
    for (i = 0, n = elements.length; i < n; i++) {
      for (var event in events) {
        elements[i].addEventListener(
          event,
          () => cursorContainer.classList.toggle(action[1], events[event])
        )
      }
    }
  }
}

The first important point is the todo object, where we register all targets:

each key is the class-name to operate querySelector[All] on
the corresponding value states true/false to indicate if all elements must be selected or not, and the class-name which will be added/removed when the mouse events occur

Based on that we iterate this object, then for each target we iterate the collection of involved elements (note that the unique element returned by querySelector is turned into an array).

Then it becomes pretty easy to adapt it for also removing them.
This is achieved by introducing a "listeners archive", populated when adding listeners, then returned by the function:

function customCursor(listeners) {
  const
    todo = {
      'previous': [false, 'previous'],
      'next': [false, 'next'],
      'brand-logo': [false, 'hidden'],
      'nav-button': [false, 'hidden'],
      'carousel-cell__link': [true, 'link']
    },
    events = {
      'mouseover': true,
      'mouseleave': false
    },
    cursorContainer = this.cursorContainer,
    cursorOffset = {left : 50, top  : 50};

  document.querySelector('.page-flickity').addEventListener(
    'mousemove',
    function (e) {
      cursorContainer.style.left = (e.pageX - cursorOffset.left) + 'px';
      cursorContainer.style.top = (e.pageY - cursorOffset.top) + 'px';
    },
    false
  );

  addListeners = !listeners;
  listeners = listeners || [];

  var className, action, elements, element, i, n;
  if (addListeners) {
    for (className in todo) {
      action = todo[className];
      elements = action[0]
        ? document.querySelectorAll(className)
        : [document.querySelector(className)];
      for (i = 0, n = elements.length; i < n; i++) {
        element = elements[i];
        for (var event in events) {
          listener = function() {
            cursorContainer.classList.toggle(action[1], events[event])
          };
          element.addEventListener(event, listener);
          listeners.push([element, event, listener]);
        }
      }
    }
    return listeners;
  } else {
    for (i in listeners) {
      listener = listeners[i];
      listener[0].removeEventListener(listener[1], listener[2]);
    }
  }
}

(please note: I didn't do anything about the "lonely" event about page-flickity element; you may do your own here)

Then it's up to you to pass this archive in to the function when you want to remove these listeners:

// add listeners:
var listeners = customCursor();

// remove listeners:
customCursor(listeners);


Login/registration part of an Android application I made some PHP files for the login/registration part of my Android application and I would like to know if it can be made safer.

config.php:

<?php

    define('hostname', 'localhost');
    define('username', 'alexiz');
    define('password', '12qwas!@QWAS');
    define('db_name', 'shhaapp_db');

?>

connection.php:

<?php

    require_once 'config.php';

    class DB_Connection {

        private $connect;
        function __construct() {
            $this->connect = mysqli_connect(hostname, username, password, db_name) or die(" DB connection error");
        }

        public function get_connection() {
            return $this->connect;
        }

    }

?>

register.php:

<?php

    require_once 'connection.php';
    header('Content-Type: application/json');

    class User {

        private $db;
        private $connection;

        function __construct() {
            $this->db = new DB_Connection();
            $this->connection = $this->db->get_connection();
        }

        public function registration($email,$username,$password) {
            $query = "INSERT INTO users(email,username,password) VALUES ('$email','$username','$password')";
            $is_inserted = mysqli_query($this->connection, $query);
            if ($is_inserted == 1) {
                $json['success'] = 'account for '.$username.' was created';
            } else {
                $json['error'] = 'email or username is already taken';
            }
            echo json_encode($json);
            mysqli_close($this->connection);
        }

    }

    $user = new User();
    if (isset($_POST['email'],$_POST['username'],$_POST['password'])) {
        $email = $_POST['email'];
        $username = $_POST['username'];
        $password = $_POST['password'];

        if (!empty($email) && !empty($username) && !empty($password)) {
            $encrypted_password = md5($password);
            $user -> registration($email,$username,$encrypted_password);
        } else {
            $json['error'] = 'please fill in all fields';
            echo json_encode($json);
        }
    }

?>

login.php:

<?php

    require_once 'connection.php';
    header('Content-Type: application/json');

    class User {

        private $db;
        private $connection;

        function __construct() {
            $this->db = new DB_Connection();
            $this->connection = $this->db->get_connection();
        }

        public function login($username,$password) {
            $query = "SELECT * FROM users WHERE username = '$username' AND password = '$password'";
            $result = mysqli_query($this->connection, $query);
            if (mysqli_num_rows($result) > 0) {
                $json['success'] = 'welcome '.$username;
            } else {
                $json['error'] = 'incorrect username or password';
            }
            echo json_encode($json);
            mysqli_close($this->connection);
        }

    }

    $user = new User();
    if (isset($_POST['username'],$_POST['password'])) {
        $username = $_POST['username'];
        $password = $_POST['password'];

        if (!empty($username) && !empty($password)) {
            $encrypted_password = md5($password);
            $user -> login($username, $encrypted_password);
        } else {
            $json['error'] = 'you must fill out all fields';
            echo json_encode($json);
        }
    }

?>

Security

You are completely open to SQL injection (in your login as well as registration page). You should always use prepared statements.

Also, md5 is not good enough, use bcrypt.

Misc

class files should not have side effects as it makes it hard to reuse them. Just create a file called user.php and place the class - and nothing else - inside it.


Set-uid root program that runs a program as the user “restrict” (follow-up) See Set-uid root program that runs a program as the user "restrict" for context.

I've written a very short program that is intended to run the program specified in its arguments (argv) as the user named "restrict" (which exists). I want to make sure that there are no subtle bugs in the program, and that the program is as simple as possible.

Is this program doing what I intend?
Can someone use this program to do anything malicious? Assume that anything running as the user "restrict" is not able to do anything malicious, but anything running as the user "root" obviously can.
Can this code be simplified, or (assuming the code is correct) can this code's correctness be made more obvious?

The code (compiled with gcc --std=c11):

#define _GNU_SOURCE
#include <sys/types.h>
#include <unistd.h>
#include <pwd.h>
#include <grp.h>
int main (int argc, char **argv) {
    if (argc < 2) return 1;
    struct passwd *r = getpwnam("restrict");
    gid_t supp[] = {};
    // De-escalate privileges.
    if (setgroups(0, supp)) return 1; // Remove supplementary groups.
    if (setgid(r->pw_gid)) return 1; // Switch group to "restrict".
    if (setuid(r->pw_uid)) return 1; // Switch user to "restrict".
    char *env[] = {NULL}; // Delete all environment variables.
    // Finally, attempt to replace this process.
    execvpe(argv[1], &argv[1], env);
    return 1; // Getting here means we failed.
}

The final binary will be owned by root, and will have the set-uid bit set, and will be executable by users in a particular group.

The ultimate goal is to allow users in a particular group to run any executable as the user "restrict".

Alternatively, is there a better way to accomplish this goal?

You should really check the return value of getpwnam(). If, for whatever reason, the user "restrict" is not found, your program will crash. I'm not sure whether a null dereference can be exploited (I think not), but it's a "bad thing" anyways.

As already mentioned, giving a descriptive error message when execv fails is a good idea, too. In fact, do this on every possible error; no need for extra verbosity where you don't exepect an error -- in this case a perror with the failed function name should be enough.

You probably don't want the user name configurable in any way for security reasons, but just to make it easier to "configure" at compile time, I'd suggest to #define it.

All in all somehow like this:

#define _GNU_SOURCE
#include <stdio.h>
#include <string.h>
#include <errno.h>
#include <sys/types.h>
#include <unistd.h>
#include <pwd.h>
#include <grp.h>

#define USERNAME "restrict"

int main (int argc, char **argv)
{
    if (argc < 2) return 1;
    struct passwd *r = getpwnam(USERNAME);
    if (!r)
    {
        perror("User " USERNAME " not found");
        return 1;
    }

    gid_t supp[] = {};
    if (setgroups(0, supp))
    {
        perror("setgroups");
        return 1;
    }

    if (setgid(r->pw_gid))
    {
        perror("setgid");
        return 1;
    }

    if (setuid(r->pw_uid))
    {
        perror("setuid");
        return 1;
    }

    char *env[] = {NULL};
    execvpe(argv[1], &argv[1], env);
    /* the following is only reached on error */

    fprintf(stderr, "Cannot execute `%s': %s\n", argv[1],
            strerror(errno));
    return 1;
}


SPOJ Problem - CPTTRN5 I have solved this problem from SPOJ and the code runs well. I want to improve this code using C++11 or C++14.

  Using two characters: . (dot) and * (asterisk) print a grid-like pattern. The grid will have l lines, c columns, and each square shaped element of the grid will have the height and width equal to s.

  Moreover, each of the grid elements will have a diagonal. The diagonal of the first square in the first line of the grid is directed towards down and right corner - use the \ (backslash) character to print it; while the next diagonal will be directed towards upper right corner - use the / (slash) character to print it. Print the successive diagonals alternately (please consult the example below).

Input

  You are given t - the number of test cases and for each of the test case three positive integers: l - the number of lines, c - the number of columns in the grid and s - the size of the single square shaped element.

Output

  For each of the test cases output the requested pattern (please have a look at the example). Use one line break in between successive patterns.

Example

  Input:

3

3 1 2

4 4 1

2 5 2

  Output:

****
*\.*
*.\*
****
*./*
*/.*
****
*\.*
*.\*
****

*********
*\*/*\*/*
*********
*/*\*/*\*
*********
*\*/*\*/*
*********
*/*\*/*\*
*********

****************
*\.*./*\.*./*\.*
*.\*/.*.\*/.*.\*
****************
*./*\.*./*\.*./*
*/.*.\*/.*.\*/.*
****************

  Time Limit 1s

  Source Limit 50000 B

  Memory Limit 1536 MB

Here is the code

#include <iostream>

using namespace std;

int main()
{
  int t;
  cin>>t;
  while(t--)
  {
    int l, c, s;
    cin >> l >> c >> s;
    int breadth = (c * (s + 1)) + 1;
    int height = (l * (s + 1)) + 1;
    for(int i = 0; i < height; i++)
    {
      for(int j = 0; j < breadth; j++)
      {
        if(i % (s+1) == 0 || j % (s+1) == 0)
        {
          cout << "*";
        }
        else if((i/(s+1) + j/(s+1)) % 2 == 0)
        {
          if(i % (s+1) == j % (s+1))
          {
            cout << "\\";
          }
          else
          {
             cout << ".";
          }
        }
        else
        {
          if(i % (s+1) == (s+1) - (j % (s+1)))
          {
            cout <<"/";
          }
          else
          {
            cout << ".";
          }
        }
      }
      cout << "\n";
    }
    cout << "\n";
   }
}

I don't see how this could benefit from C++11/14 techniques much - modern C++ gives you more tools mainly in terms of template metaprogramming, container algorithms, functional programming etc. You are dealing solely with primitive types and operate on streams, and newer standards haven't brought a revolution to these areas. The code is clean, variables have proper names etc, and that's why I'll stick to some general remarks.

Optimize I/O streams usage

Since you may possibly print a lot of information to std::cout, it might be a good idea to decouple it from the C streams. Synchronizing with C streams is associated with a significant performance hit, but necessary if you try to write to the stream concurrently or mix C and C++ streams - you are not doing any of these things though, so it's safe to do this at the start of your program:

std::ios_base::sync_with_stdio(false);

See this link for further information on this topic. Also, there are some benchamrks backing up this approach on SO, e.g. here.

Usually you don't have to care about the performance of I/O streams that much, but several times this trick helped me get to the top of the shortest execution times in coding exercises like this. Also it does make a difference if you are printing thousand of lines.

Don't use using-directive file-wide

It's considered a bad practice to put e.g. using namespace std; on top of the file as it may introduce subtle bugs when you use functions of the same name coming from different namespaces - it also requires manual checking to see from which namespace a particular function comes. It's not a problem in a simple program like that of course, but it's good to nurture good habits.

Put your code into proper function

Again, maybe you know it and it's just because this program is just a small competitive programming task, but usually you want to put your code inside a function of proper name, avoiding putting too much bare code inside main().

Make variables that are not to be mutated const

breadth and height are set once and never changed. Making them const would prevent you from accidentally mutating them while you expect them to hold their value. It also shows their role in the code better.


Unit test for a maximum-finding function A while ago, I got this question as a part of an interview test which asked to write a function to find the biggest number in a vector and write all the unit tests required to show that the code works under any circumstance using any language or pseudo-code.
To the best of my knowledge, one would need a testing framework in order to do that, so I wrote this and I got a rejection email a couple of hours later.
#include <cassert>
#include <vector>

int largest (std::vector<int> vec ) {

int max = vec[0];
for (int i=1; i<vec.size(); i++)
   if (vec[i] > max)
      max = vec[i];
return max;
}

void test (std::vector<int> vec, int i) {
assert (largest (vec) == i);
}

int main() {
std::vector<int> vec = { 7, 50, 16, 8, 25, 9, 12,112 };
test (vec, 112);
test (vec, 7);

return 0;
}

I have zero experience in professional software development and I used C++ since I find it easy to compile and run.
What did I do wrong? What should I have written? Any input would be great.

I'm not a C++ developer, and can't comment on the details of the code. But I can comment on the test cases and the general style of the code.
A good test should be enough to recreate the code from reading just the test cases. Especially when you get the task to make sure the code works under any circumstance.
Successful test cases
Let's have a look at your first test case:
std::vector<int> vec = { 7, 50, 16, 8, 25, 9, 12,112 };
test (vec, 112);

You have a vector with a max number 112, and you test for that. If I were to implement the most simple way of accepting your test I would do this (using pseudocode):
return 112;

Let's change add another test to avoid that implementation:
std::vector<int> vec = { 7, 50, 16, 8, 25, 9, 12,113 };
test (vec, 113);

Now I can't just return 112, I have to actually do something else. I would use either of these as my implementation:
return vec.lastElement();
return vec[7];

More test cases needed to fix those problems. I could go on, but I think you understand. I think the following test cases could be a good base:
{ 111 }
{ 7, 50, 16, 8, 25, 9, 12, 112 }
{ 7, 50, 16, 8, 113, 25, 9, 12 }
{ 114, 7, 50, 16, 8, 25, 9, 12 }
{ -1, -2, -3 }
{ -1, 0, 1 }
{ -333, 0 }
{ 0, 123456 }

That's a good start, now we have a lot of test cases with numbers. But we're still missing one important test:
std::vector<int> vec = { };

I have no idea what you want to be returned in this case, but it is a valid case that needs to be handled.
Naming tests
Now we have 10 tests for that simple function. But we need to look at the code to see what the test is testing, and it might not be obvious in all cases. It would be better to have a name for each test that explains what the test is testing in plain English. This is easier with testing frameworks, but a comment would also make the intention clear.
Indent
Looking at your code I struggle to see where the functions begin and end. Make sure to indent properly.
Is this a failing test?
Your second test is:
std::vector<int> vec = { 7, 50, 16, 8, 25, 9, 12,112 };
test (vec, 7);

What is this testing? My understanding is that 7 shouldn't be returned, failing this test. I don't understand why this is added, please explain in a comment.


Pseudoportable C script pattern - follow-up See the initial/previous iteration.

I have rewritten the script following the answer of @200_success.

Now it looks like this:

#! /bin/bash

# Create a temporary file name for the executable file:
TMP_PROGRAM_FILE="$(mktemp programXXXXXX)"

# Compile the embedded C program:
gcc -o "$TMP_PROGRAM_FILE" -x c - <<- END_OF_SOURCE
#include <stdio.h>

int main(int argc, char* argv[])
{
    int i;

    puts("Hello, world! I am a pseudoportable C program.");
    for (i = 1; i < argc; ++i)
    {
        printf("Argument %d: %s\n", i, argv[i]);
    }

    return argc - 1;
}
END_OF_SOURCE

# Run the program delegating all the arguments:
./$TMP_PROGRAM_FILE $@

# Save the exit status of the C program:
EXIT_STATUS=$(echo $?)

# Remove the executable file:
trap "rm $TMP_PROGRAM_FILE" EXIT

# Exit returning the exit status of the C program:
exit $EXIT_STATUS

Any critique much appreciated.

It can be made even simpler, by arranging for the C program to remove itself when it runs:

#! /bin/bash

# Create a temporary file name for the executable file:
TMP_PROGRAM_FILE="$(mktemp programXXXXXX)"

# Compile the embedded C program:
gcc -o "$TMP_PROGRAM_FILE" -x c - <<- END_OF_SOURCE
#include <stdio.h>
#include <unistd.h>

int main(int argc, char* argv[])
{
    int i;

    puts("Hello, world! I am a pseudoportable C program.");
    for (i = 1; i < argc; ++i)
    {
        printf("Argument %d: %s\n", i, argv[i]);
    }
    unlink("$TMP_PROGRAM_FILE");
    return argc - 1;
}
END_OF_SOURCE

# Run the program delegating all the arguments:
./$TMP_PROGRAM_FILE $@

This will not work under Windows but does work under Linux.  I haven't tested Mac, but believe it will work there, too. The change, of course, is that the C program now deletes itself.


Dice game rules implementation My in-laws taught me a dice game a couple years ago, and we play every once in a while. A recent excellent answer from @radarbob inspired me to proceed with translating the rules of that dice game into code.

Here's what came out of it:

So it works perfectly, and somehow I'm just as unlucky with this virtual version as in the real-life one (had to click dozens of time to get a freakin' opening roll; by that time my mother-in-law already has thousands of points, every time). At least in this version I can inject some CrookedDie implementation if I want!

I'd like the CalculateRollScore method reviewed, to see what could be improved.

It turns out I thought the code was working as it should. So I wrote a couple unit tests and... well this is where not writing unit tests has bitten me.

Well I had to change the rules anyway, since the rule for 4x 1's was just my wife being mixed-up. My mother-in-law and I agree that 3x 1's is 1000, 4x 1's is 2000 and 5x 1's is 3000.

So before I show any code, I'll show this:

Now here's the working code (view original code here), with only minor changes that don't mootinize any already posted answer:

namespace DiceGame
{
    public interface IRollScoreRules
    {
        int CalculateRollScore(IEnumerable<IRollResult<int>> results);
    }

    public class GameRollScoreRules : IRollScoreRules
    {
        public virtual bool IsOpeningRoll(IEnumerable<IRollResult<int>> results)
        {
            return CalculateRollScore(results) >= 500;
        }

        public virtual int CalculateRollScore(IEnumerable<IRollResult<int>> results)
        {
            var score = 0;

            // if less than 3 1's were rolled, each rolled 1 is 100pts:
            score += results.GroupBy(e => e.Value)
                            .Where(g => g.Key == 1)
                            .Where(g => g.Count() < 3)
                            .Sum(g => g.Count() * 100);

            // if less than 3 5's were rolled, each rolled 5 is 50pts:
            score += results.GroupBy(e => e.Value)
                            .Where(g => g.Key == 5)
                            .Where(g => g.Count() < 3)
                            .Sum(g => g.Count() * 50);

            // if more than 3 of anything other than 1 were rolled, determine number of "extra dice":
            var extras = results.GroupBy(e => e.Value)
                                .Where(g => g.Key != 1 && g.Count() > 3)
                                .ToDictionary(kvp => kvp.Key, kvp => kvp.Count() - 3);

            var extraOnes = results.GroupBy(e => e.Value)
                                   .Where(g => g.Key == 1 && g.Count() > 3)
                                   .ToDictionary(kvp => kvp.Key, kvp => kvp.Count() - 3);

            // any triplet is 100x nominal value; each extra die is another 100x nominal value:
            score += results.GroupBy(e => e.Value)
                            .Where(g => (g.Key != 1 && g.Count() >= 3))
                            .Sum(g => (g.Key * 100) + (extras.ContainsKey(g.Key) ? extras[g.Key] : 0) * (g.Key * 100));

            //score += results.GroupBy(e => e.Value)
            //    .Where(g => (g.Key == 1 && g.Count() >= 3))
            //    .Sum(g => (g.Key * 100) + (extraOnes.ContainsKey(g.Key) ? extraOnes[g.Key] : 0) * (g.Key * 100));

            // 3x 1's is 1000x nominal value; each extra die is another 1000x nominal value:
            score += results.GroupBy(e => e.Value)
                            .Where(g => g.Key == 1 && g.Count() >= 3)
                            .Sum(g => (g.Key * 1000) + (extraOnes.ContainsKey(g.Key) ? extraOnes[g.Key] : 0) * (g.Key * 1000));

            return score;
        }
    }
}

Well, I am not familiar with die, but the obvious improvement is to encapsulate those rules into separate entities. For example:

public interface IRollScoreRule
{
    int CalculateRollScore(IEnumerable<IRollResult<int>> results);
}

public interface IRollScoreRules : IRollScoreRule
{
    bool IsOpeningRoll(IEnumerable<IRollResult<int>> results);
    void AddRule(IRollScoreRule rule);
    void RemoveRule(IRollScoreRule rule);
}

public class ScoreFivesRule : IRollScoreRule
{
    int CalculateRollScore(IEnumerable<IRollResult<int>> results)
    {
        // i think some of this logic can be extracted to base class
        // results.GroupBy(e => e.Value).Where(g => g.Key == 5) part for example
        // is common for most of the rules
        var score = results.GroupBy(e => e.Value)
                        .Where(g => g.Key == 5)
                        .Where(g => g.Count() < 3)
                        .Sum(g => g.Count() * 50);
        return score;
    }
}

Then in your GameRollScoreRules.CalculateRollScore method you can iterate through collection of rules (using IEnumerable.Aggregate, for example) to get the total score, instead of having one huge method.

I also think you might want to tweak the IsOpeningRoll method. I mean, if I understand the use-case from your screenshots correctly, then with every dice roll, you want to know both - score value and if this value is higher then 500. Which means that using your GameRollScoreRules class as it is you would essentially calculate score twice (by calling CalculateRollScore and IsOpeningRoll methods). I think you sould either pass the resulting score to IsOpeningRoll method as a parameter, or remove this method all together and replace CalculateRollScore return type with some complex object (which will contain score value and bool flag) instead.


Get external IP via email I wrote a script for my Raspberry Pi that keeps checking my external IP. If it changes, the Raspberry Pi will send me the new IP via email.

Any suggestions for improvements?

#!/usr/bin/python3

import os, time

def getIP():
        os.system('dig +short myip.opendns.com @resolver1.opendns.com > raspip.txt')
        with open('raspip.txt', 'r') as f:
                return f.readline().rstrip('\n')

def sendIP(currentIP):
        os.system('sendEmail '                            +
                  '-o tls=yes '                           +
                  '-f \'***@outlook.com\' '               +
                  '-t \'***@outlook.com\' '               +
                  '-s \'smtp.live.com\' '                 +
                  '-xu \'***@outlook.com\' '              +
                  '-xp \'**password**\' '                 +
                  '-u \"raspberry via python\" -m '       +
                  '\"' + currentIP + '\"')

def main():
        lastIP = None
        while (True):
                currentIP = getIP()
                if (lastIP != currentIP):
                        sendIP(currentIP)
                        lastIP = currentIP
                time.sleep(60)

if __name__ == "__main__":
        main()

You should probably prefer #!/usr/bin/env python3 as it's less likely to vary on different systems. It's not a big deal, though.

You should use snake_case, even for IP, so names like get_ip and send_ip.

Don't call os.system; use subprocess.Popen instead:

subprocess.Popen('dig +short myip.opendns.com @resolver1.opendns.com > raspip.txt'.split())

subprocess.Popen([
    'sendEmail',
    '-o', 'tls=yes',
    '-f', '***@outlook.com',
    '-t', '***@outlook.com',
    '-s', 'smtp.live.com',
    '-xu', '***@outlook.com',
    '-xp', '**password**',
    '-u', 'raspberry via python', '-m',
    current_ip
])

This is safe against quoting errors (what if the IP contained "?).

You also didn't need the +s; strings automatically concatenate. You don't need to escape 's inside "" or " inside '' - but it doesn't matter with Popen since you don't need to worry about quoting.

You do:

with open('raspip.txt', 'r') as f:
        return f.readline().rstrip('\n')

It's more idiomatic to treat files as iterables, so instead one would do

with open('raspip.txt', 'r') as f:
        return next(f).rstrip('\n')

This also lets you add a default if wanted with next(f, default). It might be a better idea to catch the error explicitly, since StopIteration can interfere with generators:

with open('raspip.txt', 'r') as f:
        try:
                return next(f).rstrip('\n')
        except StopIteration:
                raise ValueError("No text in raspip.txt!")

You also don't need the 'r' argument - it's default.

Don't put brackets around conditions in loops of ifs. It's just while True: and if last_ip != current_ip.

A more fancy example might use, say, Python's email library - but this is fine for what it does.


Error handling to restart Word from Excel inside a loop I'm often opening, from Excel, all Word documents inside a folder.

And sometimes during the treatments, Word crash without throwing an error.

So I've added a bit of error handling to restart Word and reopen the doc I was working on.

I'm just wondering if I'm doing it properly, or if I should go further like :

Test the error number
If it's 462 : The remote server machine does not exist or is unavailable, go to RestartWord
If it's something else, show the error message

It may also not be only Error 462, but I don't have any other examples for this.

My code :

Private Sub Update_Word_DocMs_In_Folder(ByVal FolderToScan As String, _
                                            ByVal PathTxtWithInfo As String)
Dim oW As Object, NewFile As String
'''Boolean to detect if it was the initial launch
Dim NormalWordLaunch As Boolean
NormalWordLaunch = True
Set oW = OpenOrReopen("Word.Application", NormalWordLaunch)

On Error GoTo RestartWord
    NewFile = Dir(FolderToScan & "*.docm")
    Do While NewFile <> vbNullString
'''ErrHdl: Jump back here after re-creating a Word's instance
ReOpenWordFile:
        Call OpenWordDocAndApplyTreatment(oW, FolderToScan & NewFile, PathTxtWithInfo)
        NewFile = Dir()
    Loop
    oW.Quit
On Error GoTo 0

Set oW = Nothing
Set oDoc = Nothing
Exit Sub
RestartWord:
    If Err.Number <> 462 Then
        MsgBox Err.Number & vbCrLf & Err.Description, vbCritical + vbOKOnly, "Error not handled"
        DoEvents
        Resume
    Else
        Set oW = OpenOrReopen("Word.Application", NormalWordLaunch)
        '''ErrHdl: If this isn't the initial launch go back to the loop
        If Not NormalWordLaunch Then Resume ReOpenWordFile
    End If
End Sub

Function to handle creating an app instance :

Function OpenOrReopen(ByVal ClassName As String, NormalLaunch As Boolean) As Object
    Set OpenOrReopen = Nothing
    On Error Resume Next
    '''Check if there is already an instance
    Set OpenOrReopen = VBA.GetObject(, ClassName)
    On Error GoTo 0

    If OpenOrReopen Is Nothing Then
        '''If there isn't any instance, create one
        Set OpenOrReopen = VBA.CreateObject(ClassName)
    Else
        '''If detected instance is visible, create a new one
        If OpenOrReopen.Visible Then Set OpenOrReopen = VBA.CreateObject(ClassName)
    End If
    '''Set instance to be hidden
    OpenOrReopen.Visible = False
    OpenOrReopen.Options.UpdateLinksAtOpen = False

    '''For ErrHdl: Change boolean the 1st time
    If NormalLaunch Then NormalLaunch = False
End Function

Code for treatment :

Sub OpenWordDocAndApplyTreatment(WordApp As Object, _
                                 ByVal FileFullName As String, _
                                 ByVal PathTxtWithInfo As String)
    Dim oDoc As Object
    Set oDoc = WordApp.Documents.Open(FileFullName)
    With oDoc
        Do While .ReadOnly
            DoEvents
        Loop
        '''Some treatment to update doc's properties
        WordApp.Run "Properties_Updating.Properties_Update", PathTxtWithInfo
        DoEvents
        .Save
        .Close
    End With 'oDoc
End Sub

The name Update_Word_DocMs_In_Folder isn't following the PascalCase convention that's otherwise observed by your code; underscores should generally be avoided in procedure names in VB6/VBA (especially for Public members), because underscores have a very specific meaning in identifier names:

[Interface]_[Member]
[EventSource]_[EventName]

So Properties_Update is a problem, or can become one.

I find the indentation hard to follow, it doesn't seem consistent: some statements aren't indented, others are, but there's no apparent rhyme or reason to it.

The Call statement is obsolete and its use isn't consistent either; I'd remove it.

Call OpenWordDocAndApplyTreatment(oW, FolderToScan & NewFile, PathTxtWithInfo)

Becomes:

OpenWordDocAndApplyTreatment oW, FolderToScan & NewFile, PathTxtWithInfo

The RestartWord label is misleading: it's just an error handler subroutine, it doesn't necessarily "restart Word".

oDoc and oW have that useless and annoying Systems Hungarian prefix ("o" for "object", right?) and the identifiers are rather poorly named. I would've had wordApp instead of oW (funny just noticed that's quite exactly what another procedure is calling it), and oDoc is declared and assigned to Nothing, but it's never assigned an actual object reference, never passed ByRef to anything, and never referenced either: it's not used, and should be removed.

Not sure why CreateObject is being fully-qualified with the VBA standard library identifier - seems rather arbitrary. Why isn't MsgBox fully-qualified then?

I find this a bit dangerous:

MsgBox Err.Number & vbCrLf & Err.Description, vbCritical + vbOKOnly, "Error not handled"
DoEvents
Resume

If the error is unknown and unhandled, IMO the right thing to do is to gracefully halt execution, not enter a potentially infinite "try that again in an unknown error state" loop.

If Not NormalWordLaunch Then Resume ReOpenWordFile

I don't like this jumping into a loop body. If OpenWordDocAndApplyTreatment is what's throwing error 462, then OpenWordDocAndApplyTreatment is what should be handling error 462 - I'd make the procedure a Function that returns False when that error is thrown and the Word instance needs to be reset.

Honestly, that procedure-that-should-be-a-function wants its own error handling: it can blow up given an invalid path/filename, or Documents.Open fails for whatever reason, or there's no Properties_Updating module, or there's no Properties_Update macro, or .Save fails - so many things can go wrong in that scope, and all of it is just bubbling back up to the calling code's loop body - that's why you have all this jumping-around.

Handle these errors in OpenWordDocAndApplyTreatment, and the loop body and its error-handling logic can get much more straightforward.

OpenOrReopen has its side-effects backwards IMO. I would have returned a Boolean and altered an Optional ByRef outWordApp As Object = Nothing parameter. The ClassName parameter is misleading too: that's a registered ProgId, not a ClassName. Not sure why a ProgId would be needed though - it looks like you could use it to get any registered object, but then there's this:

OpenOrReopen.Visible = False
OpenOrReopen.Options.UpdateLinksAtOpen = False

Which, aside from treating the return value as if it were a declared local variable (a bad practice IMO), assumes things it shouldn't be assuming about the object's members - if it's meant to only work with a Word.Application, then don't take a ProgId parameter, hard-code it in (or make it a Const somewhere, but don't pretend it's a parameter).


Paginate IEnumerable sequence I used the following to split sequence into batches:

source
    .Select((item, index) => new {Item = item, Index = index})
    .ToLookup(x => x.Index / batchSize, x => x.Item);

but it requires full materialization of the sequence - memory complexity could be high.

Are there anything ready to be used? I think there is nothing in MoreLinq for that.

Here is a custom solution I am trying now:

public static class Batch
{
    public static IEnumerable<T[]> Split<T>(this IEnumerable<T> source, int batchSize)
    {
        var it = source.GetEnumerator();
        for (var b = split(); b.Length > 0; b = split())
            yield return b;

        T[] split() => Enumerable
            .Range(0, batchSize)
            .TakeWhile(i => it.MoveNext())
            .Select(i => it.Current)
            .ToArray();
    }
}

Since you mentioned morelinq in your question.  It does have the Batch method which is similar.  It's an IEnumerable<IEnumerable<TSource>> instead of IEnumerable<TSource[]> but if you look at the source code it's actually an IEnumerable of an array but you can't count on that as it's an implementation detail.

The only issue I see with your implementation is that source.GetEnumerator() returns back IEnumerator<T> which implements IDisposable and should be wrapped in a using statement.


Continuously scrolling content loader I have written the following Javascript class

function InScroll( container, options ) {
    "use strict";

    var isRunning = false;
    // utilities
    var noop = function() {};

    var inter = null;

    if(!container) {
        alert('container element not provided');
        return;
    }
    if(!options.interval) options.interval = 500;

    // save screen info
    var page = {
        contentHeight: $(container).getHeight(),
        pageHeight: document.viewport.getHeight(),
        scrollPosition : 0,
    }

    function scroll() {
        var pos = $(container).select("div.kurz").length;

        if(pos == 0) {
            this.kill();
            return;
        }

        // get scroll position y-axis.
        page.scrollPosition = document.viewport.getScrollOffsets()[1];

        if( !isRunning && (page.contentHeight - page.pageHeight - page.scrollPosition ) < 450) {
            new Ajax.Request(options.url + "?pos=" + pos, {
                onCreate: onCreate,
                onSuccess: append,
                onFailure: error,
                onComplete: onComplete

            });
        }
    }
    function onCreate() {
        isRunning = true;
    }

    function onComplete() {
        isRunning = false;
        page.contentHeight= $(container).getHeight();

    }

    function append( response ) {
        var resp = response.responseText.strip();

        if(resp == "") kill();

        container.innerHTML += resp;
    }

    function error( response ) {
        var resp = response.responseText.strip();
        container.innerHTML += resp;
    }

    function start() {

        if(!container) {
            alert('container element not provided');
            return;
        }

        if(!options || !options.url) {
            alert('content loader script should be set as options.url');
            return;
        }

        (function(that) {
            inter = window.setInterval( function() { that.scroll()}, 50);
        })(this)
    }

    function kill() {
        clearInterval(inter);
        $(options.loader).hide();
    }

    return {
        start: start,
        kill : kill,
        scroll: scroll
    }
}

Here I wanted to expose only the start and kill functions but due to the use of windows.setInterval i had to expose also the scroll function.

Can someone please have a look at the code, and give some suggestion how may I optiomize it?

It seems to me that you don't need the self invoking function and the anonymous function:

inter = window.setInterval( scroll, 50);

should do


Searching and sorting ranges in excel with toggle buttons I have a spreadsheet containing ±100 named ranges (5 rows, 15 colums) which are sorted based on time.

My plan was to make toggle buttons to hide or unhide named ranges when they contain a certain value.

So when I select the toggle button for 'Opel' for instance, all the named ranges containing 'Opel' are displayed. And when I select 'Opel' and 'Ford. All the ranges containing 'Opel' and 'Ford' are displayed while also being able to sort the ranges in that view.

For the sorting I use the following code:

Sub SorterenOpdrachten()

Dim Detail As Worksheet
Dim I As Long
Dim ListRng As Range
Dim LijstWks As Worksheet
Dim NamedRng As Name
Dim R As Long
Dim Rng As Range
Dim SortWks As Worksheet

'Worksheet declareren als variabele
Set Detail = Worksheets("detail")
Set LijstWks = Worksheets("LijstWks")
Set SortWks = Worksheets("SortWks")

'Startrij voor de lijst instellen = Rij 1 fungeert als "hoofding"
R = 2

'Ranges naar lijst kopiëren - Opdracht en uur
For Each NamedRng In ActiveWorkbook.Names

LijstWks.Cells(R, 1) = NamedRng.Name
LijstWks.Cells(R, 2) = NamedRng.RefersToRange.Cells(1, 2)
R = R + 1
Next NamedRng

'Ranges sorteren in de lijst
R = R - 1
Set ListRng = LijstWks.Range("A2").Resize(R - 1, 2)
ListRng.Sort Key1:=ListRng.Cells(1, 2), Order1:=xlAscending

'Ranges kopiëren naar SortWks
R = 1
For I = 1 To ListRng.Rows.Count
Set Rng = ActiveWorkbook.Names(ListRng.Cells(I, 1).Text).RefersToRange
Rng.Copy
SortWks.Cells(R, 1).PasteSpecial Paste:=xlPasteAll
R = R + Rng.Rows.Count
Next I

'Opdrachten naar detail kopiëren
R = 1
Worksheets("SortWks").Range("A1:T499").Copy
Worksheets("detail").Range("A5:T504")

Next intCounter
End Sub

This works fine.

But when I use it in combination with the toggle buttons it is too slow.

For the toggle buttons I use the following code:

Sub Tegels()

Dim nm As Name

For Each nm In Application.Names
Range(nm).EntireRow.Hidden = True
Next nm

If TglOpel Then
For Each nm In Application.Names
If Application.CountIf(Range(nm), "*" & "Opel" & "*") Then
Range(nm).EntireRow.Hidden = False
Next nm
End If

If TglChevrolet Then
For Each nm In Application.Names
If Application.CountIf(Range(nm), "*" & "Chevrolet" & "*") Then
Range(nm).EntireRow.Hidden = False
Next nm
End If

If TglFord Then
For Each nm In Application.Names
If Application.CountIf(Range(nm), "*" & "Ford" & "*") Then
Range(nm).EntireRow.Hidden = False
Next nm
End If

If TglBuick Then
For Each nm In Application.Names
If Application.CountIf(Range(nm), "*" & "Buick" & "*") Then
Range(nm).EntireRow.Hidden = False
Next nm
End If

If TglDodge Then
For Each nm In Application.Names
If Application.CountIf(Range(nm), "*" & "Dodge" & "*") Then
Range(nm).EntireRow.Hidden = False
Next nm
End If

End Sub

Sub CheckTegels()

If TglOpel Then
Call Tegels
Exit Sub
Else
If TglChevrolet Then
Call Tegels
Exit Sub
Else
    If TglFord Then
    Call Tegels
    Exit Sub
    Else
        If TglBuick Then
        Call Tegels
        Exit Sub
        Else
            If TglDodge Then
            Call Tegels
            Exit Sub
            Else

                        Dim nm As Name

For Each nm In Application.Names
Range(nm).EntireRow.Hidden = False
Next nm

End If
End If
End If
End If
End If
End If
End If
End If
End Sub

Do you have any tips for speeding this process up?

As you can probably tell I am quite new to this. Any help would be greatly appreciated!

Some basic things. You need your code to be indented consistently. That way you can see the levels of your code. If you had that you would see that on the last line of SorterenOpdrachten is a Next without a For. This won't compile.

In Tegels you have missed your first End If. And the second End If. And your third, fourth and fifth.

You haven't defined tglopel or TglChevrolet


Adding  items dynamically to a to-do list For a homework assignment, I have to create a "TO-DO" list, which will add a list item to the unordered list dynamically using JavaScript. The input element is also added using JavaScript, having to generate a unique id that changes with each site visit. I'm looking for any suggestions that will improve the efficiency of the code.

/*
 - get user input
 - create
 element
 - append that to
 list
*/

function Form_Processes() {

 this.count = 0;
 this.input_id = generate_id();

 this.addToList = function() {
  var itemToAdd = document.getElementById(this.input_id).value;
  if(!(itemToAdd === "")) {
   this.count += 1;
   var list_tag = document.getElementById('list');
   var li_tag = document.createElement('li');
   li_tag.id = "li_" + this.count;
   var li_text = document.createTextNode("- " + itemToAdd);
   li_tag.appendChild(li_text);
   list_tag.appendChild(li_tag);

   //clear textbox for next input
   document.getElementById(this.input_id).value = "";
  }
 }

 this.removeFromList = function() {
  var numsOfElements = document.getElementById('list').childElementCount;
  if(numsOfElements > 0) {
   var itemToRemove = document.getElementById('li_' + this.count);
   itemToRemove.outerHTML = "";
   this.count -= 1;
  } else {
   console.log('ERROR: Not Enough Child Elements to remove (script.js:25)');
  }
 }

 function generate_id() {
    var text = "";
    var possible = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_";

    for (var i = 0; i < 8; i++){
      text += possible.charAt(Math.floor(Math.random() * possible.length));
    }
    return text;
 }

}

var f = new Form_Processes();

function execute(x) {
 if(x == 1) {
  f.addToList()
 } else if(x == 2) {
  f.removeFromList();
 }
}

function load_form() {
 var x = document.getElementById('in');

 //create input tag
 var input_tag = document.createElement('input');
 input_tag.type = "text";
 input_tag.name = "user_input";
 input_tag.id = f.input_id;
 input_tag.placeholder = "Enter here...";

 x.appendChild(input_tag);

 //insert line break
 var break_tag = document.createElement('br');
 x.appendChild(break_tag);

 var button1 = createButton("button", "add_list", 1, "Add To List");
 var button2 = createButton("button", "remove_list", 2, "Remove From List");

 x.appendChild(button1);
 x.appendChild(button2);

}

function createButton(type, name, e, text) {
 //create buttons
 var button_tag = document.createElement('button');
 button_tag.type = type;
 button_tag.name = name;
 button_tag.onclick = function() { execute(e); }

 var button_text = document.createTextNode(text);
 button_tag.appendChild(button_text);

 return button_tag;
}
* {
 font-family: "Times New Roman", Times, serif;
}

body {
 background-color: pink;
}

fieldset {
 border: none;
}

input[type=text] {
 padding: 12px 20px;
 border-radius: 5px;
 border: 1px solid grey;
 font-size: 25px;
 margin: 2px;
}

button {
 background-color: green;
 border: none;
 border-radius: 6px; /* rounded corner look */
 color: white; /* button text color */
 text-align: center;
 font-size: 16px;
 margin: 1px;
}

ul {
 list-style-type: none;
   margin: 0;
   padding: 0;
}

li {
 background-color: grey;
 margin: 3px;
 font-size: 50px;
 border-radius: 5px;
 color: white;
 padding-left: 15px;
}
<!DOCTYPE html>

  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <title>TO-DO</title>

  <link rel="stylesheet" type="text/css" href="style.css">

   <form action="">
    <fieldset id="in">

    </fieldset>
   </form>

TO-DO

I recommend putting the script at the end of the page, just before </body> The reason is that you'll want to have scripts run when the DOM is ready (i.e. all the elements are present and ready for JS manipulation).

<center> is a deprecated stylistic element. To center an element, set left and right margins to auto. For text, use text-align: center:

.centered-element {
  display: block;
  width: 250px; /* Your desired width */
  margin-left: auto;
  margin-right: auto;
}

.element-with-centered-text {
  text-align: center;
}

This way, you have separation of concerns. HTML deals with describing the structure, CSS deals with styling your HTML.

* {
    font-family: "Times New Roman", Times, serif;
}

This is unnecessary. Applying font-family to body should be enough to apply it to all elements. Font style is inherited by all descendants until an element overrides it (and styles its own descendants).

fieldset {...}

input[type=text] {...}

button {...}

ul {..}

li {...}

I don't recommend styling elements directly because it breaks expectations. If I add a <ul> in your app for whatever purpose, I expect it to have bullets. But since your CSS removes them, my list won't have bullets. I'd have to add them back when, by default, they should have had bullets.

Instead, use classes to target app-specific styling and leave element defaults alone. The only time I would style an element directly is if it's part of a globally-applied theme or a normalizer.

As for your JavaScript, you could abstract away your element creation with a function like the following. This way, you don't have to repeat element creation scripts everywhere and you can easily describe dynamic HTML in a nested manner, like HTML.

const e = (name, properties = {}, children = []) => {
  // Create the element
  const element = document.createElement(name)

  // Apply properties
  Object.keys(properties).forEach(property => {
    element.setAttribute(property, propertyes[property])
  })

  // Append children
  children.forEach(c => {
    if(!c) return
    const node = (typeof c === 'string') ? document.createTextNode(c) : c
    element.appendChild(node)
  })

  return element
}

// Usage
const root = e('div', {}, [
  e('p', {}, [
    e('span', {}, [
      'Hello, World!',
      'Lorem Ipsum'
    ])
  ])
])

root.appendChild(e('span', {}, ['another piece of text']))

If the syntax looks familiar, it's because this is the basic premise of how most VDOM libraries work under the hood. They're just nested calls of functions that either return an actual element, or objects that represent elements (which are turned to elements later).

From there, you can describe and build your "components" like:

const LoadForm = () => {
  e('div', {}, [
    e('input', { type: 'text', name: 'user_input', placeholder: 'Enter here...' }),
    e('br'),
    e('button', { name: 'add_list' }, ['Add To List']),
    e('button', { name: 'remove_list' }, ['Remove From List']),
  ])

document.getElementById('in').appendChild(LoadForm())

You can even encapsulate this in a class if you want so that you can also add some helper methods.

class LoadForm {
  onAdd () {
    // Do something on add
  }
  onRemove () {
    // Do something on remove
  }
  render() {
    return (
      e('div', {}, [
        e('input', { type: 'text', name: 'user_input', placeholder: 'Enter here...' }),
        e('br'),
        e('button', { name: 'add_list', onclick: this.onAdd }, [
          'Add To List'
        ]),
        e('button', { name: 'remove_list', onclick: this.onRemove }, [
          'Remove From List'
        ]),
      ])
    )
  }
}

document.getElementById('in').appendChild((new LoadForm()).render())

Lastly, JavaScript is written in camelCase, not snake_case.


Count pairs in list of integers such that their addition is equal to the input value Given a List<int>, the problem I am trying to solve is:
Find the number of unique pairs in List<int> such that their addition is exactly equal to the input value.

The bottleneck is a nested for() loop that I have used to go through the list

// *** OPTIMIZE THIS LOOP ***
for (int i = 0; i < intList.Count - 1; i++)
{
    for (int j = i + 1; j < intList.Count; j++)
    {
        if ((intList[i] + intList[j] == totalValue) &&
              IsPairUnique(intList[i], intList[j]) == true)
        {
            nCombinations++;
        }
    }
}

How can this be made better for performance? I understand 'better' is a subjective word and has no real meaning as such. For a List<int> of size 50000 the code takes about 18 seconds on one of my VM. For 500000 it's way too worst. So, here by 'better' I mean 'faster'. Clearly this problem deserves much less than 18 seconds to solve in my opinion. With 1 Parallel.For() I have managed to get the loop time to 10-11 seconds, but I have a feeling that this whole algorithm needs a fresh set of eyes to look at.

Parallel.For(0, intList.Count - 1,
        i =>
        {
            for (int j = i + 1; j < intList.Count; j++)
            {
                if ((intList[i] + intList[j] == totalValue) && IsPairUnique(intList[i], intList[j]) == true)
                {
                    nCombinations++;
                }
            }
        });

How can I speed this up?
Full code from my console application is as below:

class TestClass
{
    static List<int> compareList = new List<int>();
// GetPossibleCombination method.
// This method finds out the unique number of possible combinations where
// addition of any 2 values from the list is exactly equal to 'totalValue'
static int GetPossibleCombinations(List<int> intList, long totalValue)
{
    // handle edge conditions
    if (intList == null ||
        intList.Count == 0 ||
        intList.Count > 500000 ||
        totalValue > 5000000000)
        return 0;

    compareList.Clear();
    int nCombinations = 0;

    // *** OPTIMIZE THIS LOOP ***
    for (int i = 0; i < intList.Count - 1; i++)
    {
        for (int j = i + 1; j < intList.Count; j++) // start from this element onwards
        {
            if ((intList[i] + intList[j] == totalValue) && IsPairUnique(intList[i], intList[j]) == true)
            {
                nCombinations++;
            }
        }
    }

    return nCombinations;
}

// This method creates a list of possible values we have
static bool IsPairUnique(int v1, int v2)
{
    if (compareList.Contains(v1 * 10 + v2) == true || compareList.Contains(v2 * 10 + v1) == true)
        return false;
    else
    {
        // else add a new one
        compareList.Add(v1 * 10 + v2);
        compareList.Add(v2 * 10 + v1);
    }

    return true;
}

static void Main(string[] args)
{
    int intListSize = 50000; // Optimize it for numbers upto 500,000
    long totalValue = 5000;

    List<int> intList = new List<int>();
    Random r = new Random();

    for (int i = 0; i < intListSize; i++)
    {
        intList.Add(r.Next(0, 10000)); // populate random values.
    }

    Stopwatch sw = new Stopwatch();
    sw.Start();

    // Find the number of unique pairs in 'intList' such that
    // their addition is exactly equal to 'totalValue'
    int res = GetPossibleCombinations(intList, totalValue);

    sw.Stop();
    Console.WriteLine(sw.Elapsed.ToString());
    }
}

The solution has a quadratic time complexity. If it solves the 50000-strong list in 18 seconds, I'd expect about 1800 seconds (aka 30 minutes) for a 500000 one. Notice that the problem is aggravated by the way you are looking for duplicates (and IsPairUnique doesn't look correct; it is prone to false positives).

There are few standard ways to bring the complexity down.

Sort the list. Arrange two iterators, one at the beginning, another at the end.

Now add the pointed elements. If the sum is less than the target, advance the left one. If it is greater, move the right one toward the beginning. If it is a target, record it, and move both. In any case, when moving an iterator, skip duplicates. Rinse and repeat until they met.
If you have enough extra memory, put them in a set. For each element in a set check if its complement is also there.


Postfix evaluation using a stack I just wrote the code to evaluate a postfix expression and would like it if someone reviews it for me, it works perfectly fine but I'm having trouble adding the character "=" at the end.

 public static int evalPostfix(String exp) {

    int res = 0;

    myStack list = new myStack();
    int n1;     //result of 1st popping
    int n2;     // result of 2nd popping

    for (int i = 0; i < exp.length(); i++) {
        char ch = exp.charAt(i);

            if (ch == ' ') {
            } else {
                if (ch > '0' && ch < '9') {
                    list.push(ch);
                    //          list.printS();
                } else {
                    n1 = Integer.parseInt("" + list.pop());
                    n2 = Integer.parseInt("" + list.pop());

                    switch (ch) {
                        case '+':
                            list.push(n1 + n2);
                            break;
                        case '-':
                            list.push(n1 - n2);
                            break;
                        case '*':
                            list.push(n1 * n2);
                            break;
                        case '/':
                            list.push(n1 / n2);
                            break;

                        default:
                            System.out.println("Invalid operator order!");
                    }
                }
            }
        }

    res = Integer.parseInt("" + list.pop());

    return res;
}

Conventions

According to coding conventions, Java class names should start with an uppercase letter. myStack should be MyStack.

Variable names

Whenever you have a comment after declaring a variable, rename the variable to comment itself.

int n1;     //result of 1st popping

Can be renamed to make the variable name "comment itself".

int poppingResult1;

If this, then do nothing, else do something

if (ch == ' ') {
} else {

To achieve the same effect, and make the code cleaner, write instead:

if (ch != ' ') {

Integer.parseInt

Now, it's not clear which datatype you get from list.pop(). If it is an int (or Integer), then variable = list.pop() will be sufficient.

If it is an Object, instead use Integer.parseInt(String.valueOf(list.pop()))

Anyway, if possible, change the values you store in your list to always be of the same type (Integer, preferably). This would make your code cleaner as you don't have to use Integer.parseInt every time you pop something of the list.


Find maximum number in a nested array using recursion Here is my code:

# Credit: https://www.linkedin.com/pulse/ask-recursion-during-coding-interviews-identify-good-talent-veteanu/
#
# Problem:
# Find the maximum number in a jagged array. Each element of the array can be a number or an array of
# other numbers on indefinite number of levels. For example, given an array = [1, [3, 9], 5],
# the maximum number is 9.

def max_number(array):
    if len(array) == 1 and not isinstance(array[0], list):
        return array[0]

    _array = isinstance(array[0], list) or array

    point = len(_array) // 2
    m1 = max_number(_array[:point])
    m2 = max_number(_array[point:])

    if m1 >= m2:
        print(f"m1 = {m1} larger > m2 {m2}")
        return m1
    else:
        print(f"m2 = {m2} larger > m1 {m1}")
        return m2

assert max_number([1,2,3]) == 3
assert max_number([3,2,1]) == 3
assert max_number([2,3,1]) == 3
assert max_number([1, [3, 9], 8]) == 9
assert (max_number(
     [2, 4, 10, [12, 4, [100, 99], 4], [3, 2, 99], 0]) == 100)

This is fine. The minor issue I have with is the additional isinstance check.

Originally when I worked on the solution on paper, I had forgotten
about slicing a Python array [x, [w, z] ] would produce [x], and [ [w, z] ] as opposed to [w, z] for the second recursive call.

The original code looks similar, but like this:

# The original code did not verify whether the "array" argument IS a list.
# It would be this simple:

def max_number(array):
    if len(array) == 1:
        return array[0]

    point = len(array) // 2
    m1 = max_number(array[:point])
    m2 = max_number(array[point:])

    if m1 >= m2:
        print(f"m1 = {m1} larger > m2 {m2}")
        return m1
    else:
        print(f"m2 = {m2} larger > m1 {m1}")
        return m2

Could I do any better than this? Anyway without the isinstance check but still elegant?

Your solution is much too complicated.  There is no point (pardon my pun) in splitting the array in the middle.

If you just want to demonstrate your knowledge of recursion, then apply recursion to nested lists.  You can then use the max() built-in function with a generator expression.

Instead of long comments and assert statements, write docstrings with doctests.

# Credit: https://www.linkedin.com/pulse/ask-recursion-during-coding-interviews-identify-good-talent-veteanu/
def max_number(array):
    """
    Find the maximum number in a jagged array. Each element of the
    array can be a number or an array of other numbers on indefinite
    number of levels.

    >>> max_number([1, [3, 9], 5])
    9
    >>> max_number([2, 4, 10, [12, 4, [100, 99], 4], [3, 2, 99], 0])
    100
    >>> max_number([-5])
    -5
    """
    return max(
        max_number(e) if isinstance(e, list) else e
        for e in array
    )


Obfuscation of email and phone using JavaScript I'm creating a new website, so I am thinking about how to protect my email address and phone number against crawlers, I actually mean email harvesters (most of which I suppose don't have JavaScript enabled, but I don't have any proof of it). What I do now is definitely security by obscurity or obfuscation (I might be confusing the two terms), as I want the email address to be normally visible and clickable by the user.

I started with resurrecting my 10+ years old code and editing it a little. The basics remain the same as 10+ years ago when I first coded this principle.

My question for Code Review SE - as I already posted Is this a valid option? in Security SE question - is rather clear. Primarily, I am interested in a review of this piece of code:

HTML part

test [a.t] example [d.o.t] com
+444 444 6[eight]6 6[eight][eight]

JavaScript part

function fix_m_link()
{
    var item1 = '@'; var item2 = '.'; var m_clear_text = 'test' + item1 + 'example' + item2 + 'com';
    document.getElementById('m_link').innerHTML = '' + m_clear_text + '';
}

function fix_t_link()
{
    var item8 = '8'; var t_with_spaces = '444 6' + item8 + '6 6' + item8 + item8; var t = t_with_spaces.replace(/\s+/g, '');
    document.getElementById('t_link').innerHTML = '' + '+444 ' + t_with_spaces + '';
}

These functions I then call in the body's onload.

First of all, ES2015+ onwards have template literals forw easier interpolation of string and data. For instance:

function fix_m_link(){
  var item1 = '@'
  var item2 = '.'
  var m_clear_text = `test${item1}example${item2}com`
  document.getElementById('m_link').innerHTML = `<a href="mailto:${m_clear_text}">${m_clear_text}</a>`
}

Also, JavaScript uses camel case for naming. So you might want to change fix_m_link and m_clear_text.

Putting code in one line makes it hard to read. If you have a build step, let a minifier do this. If the code is very short (like in your question), the extra kilobytes saved from omitted newlines is not worth making the code unreadable. Write code for humans to read.

Now if you really want to obscure your email, use a contact form instead. Some CMSes support this out of the box. And if your site is static, there are third-party services that allow you to embed a contact form. Most of them also support captchas, which will deter automated form submissions.


Updating height of the rows in tables with jQuery Does it make any sense and how to modify the JavaScript/jQuery code below to increase the performance? The working code is:

var rightTr = $(".superBlock div.right tr:visible");

$(".superBlock div.left tr:visible").each(function(i){
    if ($(this).height() != rightTr.eq(i).height()){
        if ($(this).height() > rightTr.eq(i).height()){
            rightTr.eq(i).height($(this).height());
        } else {
            $(this).height(rightTr.eq(i).height());
        }
    }
});

The logic is to update the respective rows within two tables - to assign the bigger height to smaller height if the heights of respective rows are different.

The eq(i)s are needed in this code as the actual problem is a bit more complicated than described here.

Most of the cells are expected to be equivalent so the first thing I do is checking if the cells are equivalent. That condition should exclude most of comparisons if it is more or it is less (one comparison instead of two).

The only think I can think of to make it perform better is to store the jQuery objects and heights in variables:

var rightTr = $(".superBlock div.right tr:visible");

$(".superBlock div.left tr:visible").each(function(i){
    var leftRow = $(this),
        leftRowHeight = leftRow.height(),
        rightRow = rightTr.eq(i);
        rightRowHeight = rightRow.height();

    if (leftRowHeight != rightRowHeight){
        if (leftRowHeight > rightRowHeight){
            rightRow.height(leftRowHeight);
        } else {
            leftRow.height(rightRowHeight);
        }
    }
});

You could simplify the if-else statement a little, though I don't think you'll realistically get a performance boost:

$(".superBlock div.left tr:visible").each(function(i){
    var leftRow = $(this),
        leftRowHeight = leftRow.height(),
        rightRow = rightTr.eq(i);
        rightRowHeight = rightRow.height();

    if (leftRowHeight > rightRowHeight){
        rightRow.height(leftRowHeight);
    } else if (leftRowHeight < rightRowHeight) {
        leftRow.height(rightRowHeight);
    }
});

The big hit to performance is the repainting and reflowing of the page that results from changing the row heights. If there really is a performance problem with resizing the table rows, then you could dive a little deeper into some enhancements.

Figure out the height of the right table
In the left column, insert a DIV before the table. Give it the same height as the right table
Set the width attribute on the left table to it's current width in pixels
Change the position of the table to absolute so that the table is in its own document flow
Resize the rows
Remove the absolute positioning and width attribute
Hide the placeholder DIV

You should get a slight speed boost by making the table position: absolute while the row resizing gets done because the browser only has to reflow the TABLE element on screen, and not any surrounding elements, but really, this might not buy you very much unless the table is big.


Python dictionary program I am having a problem with my program: it does as required but largest_blanagram() is too slow. I'm not sure whether the best way to speed it up is (1) try to speed up blanagram(), which is being called a lot (2) figure out a way to call blanagram() less. I'm not sure how to go about either of these things particularly.

Things I've been thinking: is there a way to use another dictionary more efficiently, such as using map()?

# CW Q2 blanagram.py

import string
from Q1_anagram import readfile, make_anagram_dict # change

def blanagram(word, anagramdict): # split up into smaller functions?
   """
   """
    word = sorted(word) # sort alphabetically
    alphabet = list(string.ascii_lowercase)
    index = 0
    blanagrams = []
    for char in word:
        original_char = char # save original char, so can revert back to it later
        for letter in alphabet:
            word[index] = letter
            word_copy = word # as order messes up when order alphabetically, so save current state and revert back to it after the iteration
            word = ''.join(sorted(word)) # list -> string to be able to check dict and sort alphabetically
            if word in anagramdict.keys() and anagramdict[word] not in blanagrams:
                for anagramdict[word] in anagramdict[word]:
                    blanagrams.append(anagramdict[word])# change name?
            word = word_copy
        # next character in word
        word = list(word) # string -> list in order to index
        word[index] = original_char # set back to original char, as only 1 char can be different from original word
        index += 1 # to change following char

    return blanagrams # get rid of anagrams?

def largest_blanagram(words, word_length): # greatest number of blanagram variants
    """
    """
    blanagrams = {}
    length = 0
    for word in words:
        if len(word) == word_length:
            variants = blanagram(word, make_anagram_dict(words))
            #print(variants)
            if len(variants) > length:
                blanagrams[word] = variants
                length = len(variants)

    return blanagrams

if __name__ == "__main__":
    # print(blanagram('ghost', make_anagram_dict(readfile('words.txt'))))
    import sys
    print(largest_blanagram(readfile('words.txt'), 13)) #sys.argv[1]

Imports -

def readfile(filename):
"""
"""
file = open(filename,'r').readlines() # look into WITH open file
words = []
for line in file:
    line = line.lower().split() # orang and Orang? unique? or is asteer another answer
    for word in line:
        words.append(word)
return words

def make_anagram_dict(words):
"""
"""
assert len(words) != 0
anagram_dict = {}
for word in words:
    alpha = ''.join(sorted(word))  # Order word alphabetically
    # Key is alphabetically ordered word, value is list of anagrams of the key
    anagram_dict.setdefault(alpha, []).append(word)  # So not appending NoneType (as list.append returns None)

return anagram_dict

And here is a pastebin equivalent (pastebin has file size limit) link to the words.txt file that I use.

Do not compute things more often than you need to

The point of make_anagram_dict is to have a cache to perform some anagram check very efficiently. However, you rebuild it for each and every words which misses the point of having a cache.

By writing:

ana_dict = make_anagram_dict(words)
length = 0
for word in words:
    if len(word) == word_length:
        variants = blanagram(word, ana_dict)

things should be much faster already.

Strange loop

I must confess that for anagramdict[word] in anagramdict[word] really surprised me. What are you expecting from this loop?


Directed Acyclic graph implementation in Ruby on Rails I have an implementation for a graph node class that I'd like to have function as a directed acyclic graph.

The associations are roughly as follows

class Node
  has_many :parents,
           class_name: 'Edge',
           foreign_key: :child_id

  has_many :children,
           class_name: 'Edge',
           foreign_key: :parent_id
end

class Edge
  belongs_to :parent, class_name: 'Node'
  belongs_to :child, class_name: 'Node'

  has_many :edge_properties
end

Me and a co-worker of mine had a discussion about the proper way to do this has_many relationship. The rails paradigm seems to dictate that node.parents should return a Node::ActiveRecord_Relation. However, in this case, node.parents returns a list of edges, which results the ask for the node's parents into something of the sort:

Node.where(id: Edge.where(child: node))

The question is; is this a code smell? Can we do this relationship better where node.parents would return Node::ActiveRecord? If so, how would one access edge.edge_properties?

You should take a look at the through: option of has_many:

class Node
  has_many :parent_edges, class_name: "Edge", foreign_key: :child_id
  has_many :parents, through: :parent_edges

  # ditto for children
end

Getting at edge_properties depends on the version of Rails you are using.


Optimize random number generator I have a method that generates a 10 digit random number where each digit occurs only once.

eg:

0123456789
3645720918

Is there any way this method could be optimized/improved?

    private static String randomize(){
        Random r = new Random();
        List<Integer> digits=  new ArrayList<Integer>();
        String number = "";
        for (int i = 0; i < 10; i++) {
            digits.add(i);
        }
        for (int i = 10; i > 0; i--) {
            int randomDigit = r.nextInt(i);
            number+=digits.get(randomDigit);
            digits.remove(randomDigit);
        }
        return number;
    }

If you are driven to get the absolute best performance, there are a number of things I would change. These changes will not necessarily improve the readability, but the performance will be best.

First up, creating a new Random() each time is going to slow you down. This may well be about half of your time, in fact. I recommend a static version if you can be sure you won't have threading issues, or alternatively I would recommend a ThreadLocal. In general a ThreadLocal is the better option because even though Random is thread-safe, you don't want contention when it locks one thread against another.

The second thing is "Why use a List?". Seriously, it's overkill. Use a primitive array, and preferably use char.....

String concatenation is slow, so the number += digits.get(randomDigit); is essentially the same as:

StringBuilder sb = new StringBuilder(number);
sb.append(String.valueOf(digits.get(randomDigit));
number = sb.toString();

If you insist on using String concatenation then you would be better off using:

StringBuilder sb = new StringBuilder(10);
for (...) {
    ...
    sb.append(digits.get(randomDigit));
    ...
}
return sb.toString();

Finally, when there is just one member left in the array there is no need to do the random lookup (it will always return 0), so you can just use 1 as the limit of your loop and simply append the last remaining Integer from your List.

Taking the above things in to consideration, I would recommend the following (which has a couple of other differences that I 'like'):

private static final ThreadLocal<Random> RANDOM = new ThreadLocal<Random>() {
    public Random initialValue() {
        return new Random();
    }
}

public static final String randomize() {
    final char[] digits = "0123456789".toCharArray();
    final Random rand = RANDOM.get();
    int index = digits.length;
    // Fisher-Yates.
    while (index > 1) {
        final int pos = rand.nextInt(index--);
        final char tmp = digits[pos];
        digits[pos] = digits[index];
        digits[index] = tmp;
    }
    return new String(digits);
}

EDIT

I just realized another tweak could squeeze a little more performance out too.... consider this implementation which does not even need to create an array on each invocation, and who cares what order the digits are when you are just going to reshuffle them anyway. Thus, this saves creating a char[] array each time as well. The only new instance is the returned String:

private static final class Generator {
    private final Random rand = new Random();
    private final char[] digits = "0123456789".toCharArray();

    public final String generate() {
        int index = digits.length;
        // Fisher-Yates.
        while (index > 1) {
            final int pos = rand.nextInt(index--);
            final char tmp = digits[pos];
            digits[pos] = digits[index];
            digits[index] = tmp;
        }
        return new String(digits);
    }
}

private static final ThreadLocal<Generator> GENERATOR = new ThreadLocal<Generator>() {
    public Generator initialValue() {
        return new Generator();
    }
};

public static final String randomize() {
    return GENERATOR.get().generate();
}


Extracting components of a street address I have got the following function in my class that I think can be further be improved but I'm lacking ideas how:

...
private function populateAddressVars()
{
    if (!$this->service_results || !$this->service_results->results[0]) {
        return false;
    }

    foreach ($this->service_results->results[0]->address_components as $component) {
        if (in_array('street_number', $component->types)) {
            $this->streetNumber = $component->long_name;
        }
        if (in_array('locality', $component->types)) {
            $this->locality = $component->long_name;
        }
        if (in_array('postal_town', $component->types)) {
            $this->town = $component->long_name;
        }
        if (in_array('administrative_area_level_2', $component->types)) {
            $this->country = $component->long_name;
        }
        if (in_array('country', $component->types)) {
            $this->country = $component->long_name;
        }
        if (in_array('administrative_area_level_1', $component->types)) {
            $this->district = $component->long_name;
        }
        if (in_array('postal_code', $component->types)) {
            $this->postcode = $component->long_name;
        }
        if (in_array('route', $component->types)) {
            $this->streetAddress = $component->long_name;
        }
    }
}
...

Can it be further improved?

To start with, your code doesn't make much sense to me. You're looping over $this->service_results->results[0]->address_components, and on each iteration, you seem to be setting all of the address values. If the address_components array contains 4 arrays, each setting all of the address vars, you're effectively overwriting the values, and are only left with the last set of values. More on this later, though (see "other niggles" below).

To answer your question:
Yes, you can improve this code. You have a rare case where a more readable implementation of your code actually becomes more performant, too.
The first thing to note is that the function you use the most (in_array), is implemented as a simple linear search of an array (hash table internally). This means it has a time complexity of O(n), n being the size of the array. Replacing it with a loop might seem to do the same thing, but you'll only go through the array once:

foreach ($this->service_results->results[0]->address_components as $component) {
    foreach ($component->types as $k)
    {
        switch ($k)
        {
            case 'street_number': $this->streetNumber = $component->long_name; break;
            case 'postal_town': $this->town = $component->long_name; break;
            case 'administrative_area_level_1': $this->district = $component->long_name; break;
            case 'postal_code': $this->postcode = $component->long_name; break;
            case 'route': $this->streetAddress = $component->long_name; break;
            case 'administrative_area_level_2':
                $k = 'country';//same as country, so reassign, don't break
            case 'country':
            case 'locality':
                $this->{$k} = $component->long_name;//key and property share the same name...
                break;
        }
    }
}

This already looks a tad better, but it still relies on us hard-coding all possible values for $component->types and assign the value of $component->long_name to the properties. I'd use a mapping array instead:

//outside all loops:
$map = array(
    'street_number' => 'streetNumber',
    'postal_town'   => 'town',
    'postal_code'   => 'postcode',
    'route'         => 'streetAddress',
    'country'       => 'country',
    'locality'      => 'locality',
    'administrative_area_level_1' => 'district',
    'administrative_area_level_2' => 'country'
);
//inner loop:
foreach ($component->type as $k)
{
    if (isset($map[$k]))
        $this->{$map[$k]} = $component->long_name;
    else
        //handle error, type not in $map: throw exception, log or "overload" $this (bad idea IMHO)
        $this->{$k} = $component->long_name;
}

Other niggles:
There are some other issues I have with your code. Two to be precise:

Data format:
Like I said before, you seem to be re-assigning the same properties over and over (or at least, there is a possibility you are). You might want to re-evaluate the data structure you have, if the $this->service_results->results[0]->address_components array looks something along the lines of:

[
    [
        'types' => ['street_number', 'postal_town', 'country',...],
        'long_value' => 'some value here'
    ],
    [
        'types' => ['street_number', 'postal_town', 'country',...],
        'long_value' => 'Another value'
    ]
]

Then your properties ($this->streetNumber, $this->town, $this->country,...) will all end up with the same value: "Another value" in this case. This is probably not what you want. Since each $component seems to have an array of type values, but only one real value (long_name), I suspect your data to look more like this:

[
    [
        'types' => ['street_number'],
        'long_value' => '10'
    ],
    [
        'types' => ['postal_town'],
        'long_value' => 'London'
    ],
    [
        'types' => ['country','administrative_area_level_1'],//alias for country
        'long_value' => 'UK'
    ]
]

Which means that, most of the time, traversing the entire types array is rather pointless, and so we can safely refactor the last snippet I suggested to break; once we've assigned our long_name value:

foreach ($component->type as $k)
{
    if (isset($map[$k]))
    {
        $this->{$map[$k]} = $component->long_name;
        break;//found the property to set, skip to next component
    }
}

If your data essentially does look like the first version (where the types array lists all possible types), then consider checking the value of long_name, because really: if the value represents a street number, you don't want to assign that value to the $this->country property, do you?
Add a var_dump($this->service_results->results[0]->address_components); to your question, so we know what type of data you're dealing with here.

Separation of concern:
Your class is, IMO, clearly breaking the SOLID principles. Single Responsability Principle (SRP) and, as a result, the separation of concern is not adhered to. The idea is that a class is a unit of code that has "at most one reason to change". That's a fancy way of saying that a class can have only 1 job. A class can be tasked to connect to a database, or perform cUrl request. But those classes should not be responsable for processing and storing the data it they obtain.

A PDO instance connects to the database, but does not handle prepared statements, or result sets. That's not its job, that's the reason for a PDOStatement to change. Neither of these two classes report errors, that's the job of the PDOException class.
Now look at your code. Your class might use another class instance to perform queries or curl requests, or it could contain the code that does this itself. If so: that's bad, because your class now does at least 2 things: fetching data and storing it (assigning to properties).

As if that weren't enough, the code you posted here also seems to take care of raw responses, extracting the data you're after and then storing it. So the class that contains the method you posted here has at least 2, and possibly even 3 "reasons to change". That's just plain evil.
What's more: your class doesn't only store the formatted data, it carries with it the raw data from which you extracted the address, the property $this->service_results. What is it for? You process the results, store the data you're after and either get rid of the raw data, or pass it on to another class/module that needs that data.

Post more code (as a separate review-request) if you want us to have a look at your class as a whole.