Different factorial algorithms I'm new to Ruby and as an exercise I implemented different factorial algorithms. Other rules are: raise an ArgumentError exception if n < 0 or if n is not an integer.

I'm interested in whatever feedback you can think of, including Ruby idioms (can something be implemented more idiomatic?). If you see anything that can be improved in any way, let me know.

# While loop
def factorial_w(n)
    validate(n)
    total = 1
    while n > 1
        total *= n
        n -= 1
    end
    total
end

# For loop
def factorial_f(n)
    validate(n)
    total = 1
    for i in (2 .. n)
        total *= i
    end
    total
end

# Recursive
def factorial_r_recursion(n)
    if n < 2
        1
    else
        n * factorial_r(n - 1)
    end
end

def factorial_r(n)
    validate(n)
    factorial_r_recursion(n)
end

# Tail-recursive
def factorial_tr_recursion(n, acc)
    if n < 2
        acc
    else
        factorial_tr(n - 1, n * acc)
    end
end

def factorial_tr(n, acc = 1)
    validate(n)
    factorial_tr_recursion(n, acc)
end

# Iterative with Range.each
def factorial_re(n)
    validate(n)
    if n == 0
        1
    else
        (2 .. n - 1).each {|i| n *= i}
        n
    end
end

# Iterative with Range.inject
def factorial_ri(n)
    validate(n)
    (1 .. n).inject(:*) || 1 # || 1 handles n = 0 so !0 = 1
end

# Iterative with Range.reduce
def factorial_rr(n)
    validate(n)
    (1 .. n).reduce(:*) || 1 # || 1 handles n = 0 so !0 = 1
end

# Stirling's Approximation, will return infinity if n > 170
def factorial_sa(n)
    validate(n)
    Math.sqrt(2 * Math::PI * n) * (n / Math::E) ** n
end

# Helper for argument validation
def validate(n)
    if n < 0 or not n.kind_of? Integer
        raise ArgumentError, "n must be an integer and >= 0"
    end
end

if n < 2
    1
else
    n * factorial_r(n - 1)
end

This is exactly equivalent to the following, which is called a ternary statement:

n < 2 ? 1 : n * factorial_r(n - 1)

Your factorial_r and factorial_r_recursion functions call each other, but this makes you call your validate function over and over. You should rewrite it so that factorial_r_recursion only ever calls itself, because you can ensure that everything it produces is valid already. (Same with the tr functions)

if n < 0 or not n.kind_of? Integer

This would be much better worded as:

unless n.is_a? Integer && n >= 0

No one uses kind_of?, and you need to check if the number is an Integer before you compare it to 0.

Although it doesn't appear to be in the scope of your project, ideally you would not call validate at the beginning of each factorial method. You would call it once, when you actually get n, and THEN pass it in to the factorial function.

Some last points; I recognize that this is a learning step for you, but do not use for loops in ruby. They have bad side effects and you do not need them, there are plenty of ways to iterate over collections. Also the while loop looks rather ugly but that's because it's the wrong tool for the job, so it's to be expected.

All in all, it's pretty decent ruby! Good job on using reduce / inject, a lot of beginners don't use/comprehend what they do., however reduce is an alias of inject so to have them both is kind of silly :P


A Simple Connect 4 game in Python I have made a simple command line version of connect 4. It is my first time using classes and importing one part of my project into another so any feedback on how I've done and any way it could be changed or improved would be helpful.

This code file (connect4.py) contains the basic functions I have made:

class connect4:
    def __init__(self, height=6, width=7):
        self.height = height
        self.width = width
        self.board = [['' for x in range(width)] for i in range(height)]

    def return_board(self):
        return self.board

    def get_column(self, index):
        return [i[index] for i in self.board]

    def get_row(self, index):
        return self.board[index]

    def get_diagonals(self):
        diagonals = []
        for p in range(self.height + self.width - 1):
            diagonals.append([])
            for q in range(max(p - self.height + 1, 0),
                           min(p + 1, self.height)):
                diagonals[p].append(self.board[self.height - p + q - 1][q])
        for p in range(self.height + self.width - 1):
            diagonals.append([])
            for q in range(max(p - self.height + 1, 0),
                           min(p + 1, self.height)):
                diagonals[p].append(self.board[p - q][q])
        return diagonals

    def make_move(self, team, col):
        if '' not in self.get_column(col):
            return self.board
        i = self.height - 1
        while self.board[i][col] != '':
            i -= 1
        self.board[i][col] = team
        return self.board

    def check_win(self):
        for i in range(self.height):  # check rows
            for x in range(self.width - 3):
                if self.get_row(i)[x:x + 4] in [['0', '0',
                                                 '0', '0'], ['1', '1', '1', '1']]:
                    return self.board[i][x]
        for i in range(self.width):  # check columns
            for x in range(self.height - 3):
                if self.get_column(
                        i)[x:x + 4] in [['0', '0', '0', '0'], ['1', '1', '1', '1']]:
                    return self.board[x][i]
        for i in self.get_diagonals():
            for x in range(len(i)):
                if i[x:x + 4] in [['0', '0', '0', '0'], ['1', '1', '1', '1']]:
                    return i[x]

        return None

This file is what I run and imports the connect4.py file:

import connect4

b = connect4.connect4()

while True:
    for i in b.return_board():
        print(i)
    if b.check_win() != None:
        break
    col = int(input('Team 0 choose column: '))-1
    b.make_move('0' , col)
    for i in b.return_board():
        print(i)
    if b.check_win() != None:
        break
    col = int(input('Team 1 choose column: '))-1
    b.make_move('1' , col)

print('Thank you for playing')

Docstrings

You should include a docstring at the beginning of every method, class, and module you write. This will allow any documentation to identify what your code is supposed to do. Pulling from GeeksforGeeks, this is what a docstring should be:

What should a docstring look like?

The doc string line should begin with a capital letter and end with a period.
The first line should be a short description.
If there are more lines in the documentation string, the second line should be blank, visually separating the summary from the rest of the description.
The following lines should be one or more paragraphs describing the object’s calling conventions, its side effects, etc.

ClassNamingShouldBePascalCase

Class names should normally use the CapWords convention. The naming convention for functions may be used instead in cases where the interface is documented and used primarily as a callable. Note that there is a separate convention for builtin names: most builtin names are single words (or two words run together), with the CapWords convention used only for exception names and builtin constants. [source]

In your case, you have a class named connect4. This does not conform to PEP-8 standards. It should instead be ConnectFour.

Variable Naming

You variable names should be descriptive and thoughtful enough for you to understand what that variable is holding. In your test file, you use b as the variable to hold your connect4 object. If you end up writing a larger program from this, how will you remember b is the game? What if you decide to use c, d, e, etc? Then you end up with variable names spanning the entire alphabet. This is become confusing very fast. Changing b to game makes it very clear that that's the game object.

Accessor Method Naming

Having accessor and mutator (getters and setters) methods aren't really common in python, at least not as much as in java or c#. When dealing with accessor methods, the name should be get_...(). This makes it clear that you're getting the thing from the object you're calling it on. For example, get_board() as opposed to return_board(). Not convinced? Consider this:

...
if b.get_board() is not None:
    do_stuff()
...

...
if b.return_board() is not None:
    do_stuff()
...

In this context, which is what you're using, it seems like return_board is returning a boolean, True or False, if the board was "returned successfully". get_board makes it clear that you're getting a board.

Unnecessary accessor methods

On the subject of get_board(), the whole method is unnecessary. In fact, all of the get_...() are unnecessary, but some of them are there so the code isn't all messy. Which I can appreciate. However, the get_board() is not needed. Just do game.board instead of game.get_board().

enumerate vs range(len())

Many python beginners, whether they be coming from other languages like C, or brand new to programming, inevitably go through a phase where they try to do the same thing.

This results in things like this:

for i in range(len(sequence)):
    print(sequence[i])

This is never the right answer.  In python we have the power and ability to loop directly over the items in a sequence.

We can instead do this:

for item in sequence:
    print(item)

It is simpler to type; simpler to read; and most importantly makes more sense.  As we were never concerned with the index to begin with, we don't need to bother with it.

There are however cases in which one does want the index as well as the item.  In such cases enumerate is the tool of choice.

Our loop becomes:

for i, item in enumerate(sequence):
    print("{} : {}".format(i,item)

In short, if you ever catch yourself writing range(len(...)) rethink your goal and figure out what it really is you are interested in. You can apply this principle to your code.

is not vs !=

This StackOverflow answer explains this amazingly:

== is an equality test. It checks whether the right hand side and the left hand side are equal objects (according to their __eq__ or __cmp__ methods.)

is is an identity test. It checks whether the right hand side and the left hand side are the very same object. No methodcalls are done, objects can't influence the is operation.

You use is (and is not) for singletons, like None, where you don't care about objects that might want to pretend to be None or where you want to protect against objects breaking when being compared against None.

_ for unused loop variables

_ has 4 main conventional uses in Python:

To hold the result of the last executed expression(/statement) in an interactive
interpreter session. This precedent was set by the standard CPython
interpreter, and other interpreters have followed suit
For translation lookup in i18n (see the
gettext
documentation for example), as in code like:
raise forms.ValidationError(_("Please enter a correct username"))
As a general purpose "throwaway" variable name to indicate that part
of a function result is being deliberately ignored (Conceptually, it is being discarded.), as in code like:
label, has_label, _ = text.partition(':').
As part of a function definition (using either def or lambda), where
the signature is fixed (e.g. by a callback or parent class API), but
this particular function implementation doesn't need all of the
parameters, as in code like: callback = lambda _: True

(For a long time this answer only listed the first three use cases, but the fourth case came up often enough, as noted here, to be worth listing explicitly)

The latter "throwaway variable or parameter name" uses cases can conflict with the translation lookup use case, so it is necessary to avoid using _ as a throwaway variable in any code block that also uses it for i18n translation (many folks prefer a double-underscore, __, as their throwaway variable for exactly this reason).

Use i / j for double loops

This one is my opinion / possibly a common practice. When using two nested for loops, I like to use i and j as the outer and inner loop variables, respectively. i and j have typically been used as subscripts in quite a bit of math for quite some time (e.g., even in papers that predate higher-level languages, you frequently see things like "Xi,j", especially in things like a summation). Most people seem to have seen little reason to change that.

Multiple anonymous array checks

You have this code:

...
for i in range(self.height):  # check rows
    for x in range(self.width - 3):
        if self.get_row(i)[x:x + 4] in [['0', '0','0', '0'], ['1', '1', '1', '1']]:
            return self.board[i][x]
for i in range(self.width):  # check columns
    for x in range(self.height - 3):
        if self.get_column(i)[x:x + 4] in [['0', '0', '0', '0'], ['1', '1', '1', '1']]:
            return self.board[x][i]
for i in self.get_diagonals():
    for x in range(len(i)):
        if i[x:x + 4] in [['0', '0', '0', '0'], ['1', '1', '1', '1']]:
            return i[x]
...

You create the anonymous array [['0', '0', '0', '0'], ['1', '1', '1', '1']] three times! Instead of this repetition, you should assign this array to a variable, and check against that variable. The updated code reflects these changes.

'' => ' ' formatting

You have a couple of formatting problems when outputting the board to the console. When there's a column a couple stacks high in the middle of the board, it does this:

['', '', '', '', '', '', '']
['', '', '', '', '', '', '']
['', '', '', '', '', '', '']
['', '1', '', '', '', '', '']
['0', '1', '', '', '', '', '']
['0', '1', '0', '', '', '', '']

Even after just one input, it gets shifted:

['', '', '', '', '', '', '']
['', '', '', '', '', '', '']
['', '', '', '', '', '', '']
['', '', '', '', '', '', '']
['', '', '', '', '', '', '']
['0', '', '', '', '', '', '']

You should allocate this space in the beginning. In __init__, self.board should look like this:

self.board = [[' ' for x in range(width)] for i in range(height)]

and make_move(self, team, col) should now look like this:

def make_move(self, team, col):
    """
    Simulates a move and puts a 0/1 in the specified column
    """
    if ' ' not in self.get_column(col):
        return self.board
    i = self.height - 1
    while self.board[i][col] != ' ':
        i -= 1
    self.board[i][col] = team
    return self.board

Updated Code

"""
Module Docstring
A description of your program/class goes here
"""

class ConnectFour:
    """
    Class for creating a connect four game
    """
    def __init__(self, height=6, width=7):
        self.height = height
        self.width = width
        self.board = [[' ' for x in range(width)] for i in range(height)]

    def get_column(self, index):
        """
        Returns a column at the specified index

        :param index: Index at which column will be returned
        """
        return [i[index] for i in self.board]

    def get_row(self, index):
        """
        Returns a row at the specified index

        :param index: Index at which row will be returned
        """
        return self.board[index]

    def get_diagonals(self):
        """
        Returns all the diagonals in the game
        """

        diagonals = []

        for i in range(self.height + self.width - 1):
            diagonals.append([])
            for j in range(max(i - self.height + 1, 0), min(i + 1, self.height)):
                diagonals[i].append(self.board[self.height - i + j - 1][j])

        for i in range(self.height + self.width - 1):
            diagonals.append([])
            for j in range(max(i - self.height + 1, 0), min(i + 1, self.height)):
                diagonals[i].append(self.board[i - j][j])

        return diagonals

    def make_move(self, team, col):
        """
        Simulates a move and puts a 0/1 in the specified column
        """
        if ' ' not in self.get_column(col):
            return self.board
        i = self.height - 1
        while self.board[i][col] != ' ':
            i -= 1
        self.board[i][col] = team
        return self.board

    def check_win(self):
        """
        Checks self.board if either user has four in a row
        """

        four_in_a_row = [['0', '0', '0', '0'], ['1', '1', '1', '1']]

        #Check rows
        for i in range(self.height):
            for j in range(self.width - 3):
                if self.get_row(i)[j:j + 4] in four_in_a_row:
                    return self.board[i][j]

        #Check columns
        for i in range(self.width):
            for j in range(self.height - 3):
                if self.get_column(i)[j:j + 4] in four_in_a_row:
                    return self.board[j][i]

        #Check diagonals
        for i in self.get_diagonals():
            for j, _ in enumerate(i):
                if i[j:j + 4] in four_in_a_row:
                    return i[j]

        return None

def start_game():
    """
    Starts a game of ConnectFour
    """
    game = ConnectFour()

    while True:

        for i in game.board:
            print(i)
        if game.check_win() is not None:
            break

        col = int(input('Team 0 choose column: ')) - 1
        game.make_move('0', col)

        for i in game.board:
            print(i)
        if game.check_win() is not None:
            break

        col = int(input('Team 1 choose column: ')) - 1
        game.make_move('1', col)

    print('Thank you for playing')

if __name__ == '__main__':
    start_game()


Simple MVP Name-Finder I've recently learnt about the Model-View-Presenter architectural pattern and I wanted to give it a go with a simple VBA project.

The aim is to retrieve a person's name from a UserForm.

The userform looks like this:

A user should select a team, which updates the name field > Select a name belonging to that team > Hit OK.

The control names are:

TeamSelect
NameSelect
OkButton
CloseButton

  I'm particularly interested in feedback on my implementation of MVP
  (or lack of!)

... So here is the code:

GetName Sub - Standard Module: Macros

Public Sub GetName()
    With New NamePresenter
        .Present
    End With
End Sub

Model - Class Module: NameModel

Private Type TNameView
    Team As String
    Name As String
End Type
Private this As TNameView

Public Property Get Team() As String
    Team = this.Team
End Property

Public Property Let Team(ByVal value As String)
    this.Team = value
End Property

Public Property Get Name() As String
    Name = this.Name
End Property

Public Property Let Name(ByVal value As String)
    this.Name = value
End Property

Public Property Get IsValid() As Boolean
    IsValid = (this.Name <> vbNullString) And (this.Team <> vbNullString)
End Property

View - UserForm Module: NameView

Public Event UpdateNames(ByVal Team As String)

Private Type TNameView
    Model As NameModel
    Data As NameData
    IsCancelled As Boolean
End Type
Private this As TNameView

Public Property Set Model(ByVal Model As NameModel)
    Set this.Model = Model
End Property

Public Property Set Data(ByVal Data As NameData)
    Set this.Data = Data
End Property

Public Property Get IsCancelled() As Boolean
    IsCancelled = this.IsCancelled
End Property

Private Sub TeamSelect_Change()
    this.Model.Team = TeamSelect.value
    RaiseEvent UpdateNames(TeamSelect.value)
End Sub

Private Sub NameSelect_Change()
    this.Model.Name = NameSelect.value
End Sub

Private Sub OkButton_Click()
    If this.Model.IsValid Then
        Me.Hide
    Else
        MsgBox "Something is missing!"
    End If
End Sub

Private Sub CloseButton_Click()
    this.IsCancelled = True
    Me.Hide
End Sub

Private Sub UserForm_Activate()
    PositionForm
    TeamSelect.List = this.Data.Teams
End Sub

Private Sub UserForm_QueryClose(Cancel As Integer, CloseMode As Integer)
    Cancel = True
    CloseButton_Click
End Sub

Private Sub PositionForm()
    With Me
        .StartUpPosition = 0
        .Left = Application.Left + (0.5 * Application.Width) - (0.5 * .Width)
        .Top = Application.Top + (0.5 * Application.Height) - (0.5 * .Height)
    End With
End Sub

Presenter - Class Module: NamePresenter

Private WithEvents View As NameView

Private Model As NameModel
Private Data As NameData

Public Sub Present()
    Set View = New NameView
    Set Model = New NameModel
    Set Data = New NameData

    With View
        Set .Model = Model
        Set .Data = Data
        .Show vbModal
        If .IsCancelled Then Exit Sub
        MsgBox Model.Team & " / " & Model.Name
    End With
End Sub

Private Sub View_UpdateNames(ByVal Team As String)
    View.NameSelect.List = Data.Names(Team)
End Sub

Data - Class Module: NameData

Public Property Get Teams() As Variant
    Teams = Array("Team1", "Team2", "Team3")
End Property

Public Function Names(ByVal Team As String) As Variant
    Select Case Team
        Case "Team1"
            Names = Array("Name1", "Name2", "Name3")
        Case "Team2"
            Names = Array("Name4", "Name5", "Name6")
        Case "Team3"
            Names = Array("Name7", "Name8", "Name9")
    End Select
End Function

Note: I've ommitted Option Explicit only in this post to keep the code-section as short as possible

The UpdateNames event should be TeamChanged or something, so that it tells the presenter what happened on the view, as opposed to what the presenter should be doing; "update names" sounds more like a method name than an event, too.

The presenter shouldn't know about any form controls: the view knows about the model - there's no need to round-trip to the presenter to update the list of names, IMO that's pure presentation logic that should remain encapsulated in the view.

The IsValid logic could be used to keep the OK button disabled until the model is in a valid state; that way you wouldn't need that MsgBox, as the user wouldn't even be able to attempt to submit incomplete/invalid data.


PHP download counter script with htaccess I'd like to show you my script and would like to hear your security hints. Maybe the code is also useful to someone else.

What I want to archive:

Someone clicks on a link on my web page
a htaccess file looks for some file-extensions and if the requested file is one of these, it will be counted.
After the count (done with PDO prepare), the file is proceed to the user.

Because I want to get maximum security and not offering someone other files than from the upload folder I ask before doing it wrong.

.htaccess:

RewriteEngine on
RewriteRule ^(.*).(rar|zip|pdf|tar|apk|rpm|exe)$ /download.php?file=$1.$2 [L,NC]

download.php

<?php

// Allowed extensions
$filetypes = array("rar", "zip", "pdf", "tar", "apk", "rpm", "exe");

// Details of requested file
$file_path  = $_REQUEST['file'];

// NullByte cleaner
$file_path = str_replace(chr(0), '', $file_path);

// Split details
$path_parts = pathinfo($file_path);
$file_name  = $path_parts['filename'];
$file_ext   = $path_parts['extension'];

// CleanUp
$file_name  = preg_replace('/[^\0-9_a-zA-ZäöüÄÖÜ]+/', '', $file_name);
$file_ext  = preg_replace('/[^\0-9a-zA-Z]+/', '', $file_ext);

// Create path
$dir_path = $_SERVER['DOCUMENT_ROOT']."/";
$finalpath  = $dir_path . $file_name . "." . $file_ext;

// Allowed extension?
if ( in_array($file_ext, $filetypes) ) {

    // Does the file exist?
    if ( is_file($finalpath) ) {

        // Final security check
        setlocale(LC_ALL,'en_US.UTF-8');
        $local_ext = pathinfo($finalpath, PATHINFO_EXTENSION);

        // Allowed local extension?
        if ( in_array($local_ext, $filetypes) ) {

            // Logging
            // ..........

            // Download
            header('Content-Description: File Transfer');
            header('Content-Type: application/octet-stream');
            header('Content-Disposition: attachment; filename=' . basename($finalpath));
            header('Content-Transfer-Encoding: binary');
            header('Expires: 0');
            header('Cache-Control: must-revalidate, post-check=0, pre-check=0');
            header('Pragma: public');
            header('Content-Length: ' . filesize($finalpath));
            ob_clean();
            flush();
            readfile($finalpath);
            exit;
        }

    }

}

// Something went wrong
echo "Error...";
exit;

You are doing a lot of things right. You are using a whitelist instead of a blacklist for extensions, you are using pathinfo to get the extension, you clean nullbytes (optional, but still good practice), you use basename in the filename (also optional), you have a strict whitelist for filenames (theoretically), you use a good content type, and so on.

There is one issue, and two small suggestions:

Your filename whitelist allows all characters between ascii 0 and ascii 57, eg NUL, TAB, !, ", <, etc. Given the rest of your code, I'm assuming that this is a bug and that you only wanted to allow alphanum + umlaute. The same is true for the extension filter.
There is pretty much never a good reason to use REQUEST. You probably know the HTTP method you are using, so you should use that. This is somewhat of an issue if the method is POST, as an attacker can downgrade that to GET, which can make exploitation of some issues easier.
You may want to consider checking if the resolved $finalpath is inside the download directory. This isn't necessary right now, as your pathinfo approach as well as your whitelist both on their own are defense enough against directory traversal. But it is best-practice, and it may be useful if the code is reused in a different context, or if your whitelist (and pathinfo) approach need to be changed for some reason.

Regarding non-security aspects:

Your comments do not add all that much and should probably be removed. Some can be used to create better variable names (filetypes might be allowedExtensions), others are just repeating the code, and some are somewhat unclear (what is a local extension?).
Your ifs are a bit too nested for my taste. If you reverse the ifs they will be easier to read. If you return/die on the else case, you would also get rid of the nesting.


Use va_list to format a string I wrote a function that basically works like sprintf, but instead returns the formatted string (std::string). I haven't worked on this long, and I'm aware there are tons of improvements that could be made (alloc/realloc perhaps?). I was just looking for some advice before taking the next step.

GLOBAL string format(const string& format, ...)
{
    LOCAL const size_t initialSize = 64;

    string returnVal;

    char buffer[initialSize];
    int length;

    va_list args;

    va_start(args, format);
    {
        length = vsnprintf(buffer, initialSize, format.c_str(), args);
    }
    va_end(args);

    char bufferCorrectSize[length];

    va_start(args, format);
    {
        vsnprintf(bufferCorrectSize, length + 1, format.c_str(), args);
    }
    va_end(args);

    return bufferCorrectSize;
}

Ignore GLOBAL and LOCAL. They are just aliases for static I defined for organization.

Don't invent your own language through macros

GLOBAL and LOCAL might look cute but they make the code harder to read for everyone else. Not to mention the name-space pollution caused by such macros.

You say that they both expand to static. That seems wrong. A function declared static is not “very global” – it is private to its translation unit. A local variable declared static is not “very local” – it is shared between all invocations of the function. This also seems wrong. If you want to use format as a library function, it shouldn't be static. And declaring the integer initialSize as static seems pointless at best.

#include all required headers

Probably you have just left it out of your question but your code should always #include all required header files.  In your case, that would be

<cstdio> for std::vsnprintf,
<string> for std::string,
<cstddef> for std::size_t and
<cstdarg> for va_start, va_end and std::va_list.

Since you're using string without the std:: qualifier, I assume you also have a using namespace std; somewhere in your code. Get rid of it and type the extra five characters instead. It makes it clearer where the types are coming from and doesn't pollute the global name-space.

Use namespaces to organize your functions

Maybe you just didn't show it but you should put everything except main in some namespace. Instead of declaring stuff static, consider using an anonymous namespace. But since your function ought to be useful in multiple translation units, it probably shouldn't be static anyway.

Properly allocate and deallocate dynamic memory

The line

char bufferCorrectSize[length];

is not correct. (Your compiler should have warned you about it.) In C++, an array must have a fixed size known at compile-time. Since the value of length is only obtained at run-time from a previous function call, this is not possible. You could dynamically allocate a buffer of the wanted size using

char * bufferCorrectSize = new char[length];

and release it again once you're done with it.

delete[] bufferCorrectSize;

But this leaves you with all kinds of memory management issues. So instead of managing it yourself, use a std::vector<char>.

Unfortunately, you cannot use the std::string directly because its data member function will only give you a const buffer. If you know exactly what you're doing, you can take the address of the first character of the string and use it a s a pointer to a writable buffer. But I'd rather not play such tricks unless you're comfortable with what you're doing and the additional performance is critical.

Don't repeat yourself

You're calling std::vsnprintf twice; basically with the same arguments. Also, there is no need to call it again if the buffer was large enough the first time. You can re-factor this into a loop.

Get rid of the useless nested scope

While a nested scope { … } can be useful from time to time, in your case, it serves no purpose. You're not even declaring any variables inside it.

Handle errors properly

If std::vsnprintf encounters an error, it will return a negative number. You should test for this and properly handle the error. throwing an exception is probably an adequate reaction. Don't forget to still call va_end in this case.

Handle embedded NUL bytes correctly

You're returning bufferCorrectSize which will decay to char *. Unlike others have commented, this is not returning a pointer to a local object. Since format is declared with a return type of std::string and the latter is implicitly constructible from a const char *, you're safely returning the std::string as intended. However, if there is a NUL byte somewhere in the buffer, you'll truncate. Therefore, you should use the constructor taking a pointer and a length and call it explicitly.

Eliminate unused variables

The variable returnVal is never used. Your compiler might be able to warn you about this.

Make debugging easier with function attributes

The C-style printf family of functions has a bad reputation of being notoriously not type-safe and responsible for numerous bugs that can lead to serious security holes. A good compiler like GCC can warn you if such a function is called with arguments that do not match the type specifiers in the format string. You can enable the same warnings for your function, too. Of course, GCC can only check the arguments if it can see the format string. Using an untrusted format string is a very bad idea anyway so you should consider making it a const char *. If somebody really wants to call your function with a computed format string, they can always obtain a C-style string via the c_str member function of std::string. This will give them the compiler warning they deserve so they can think twice about their decision. Having format accept a std::string as format string also leads to the false assumption that one can embed NUL characters in it. But since you're going to call c_str on it anyway, std::vsnprintf will only ever see a NUL terminated string. Making the type of the format string const char * avoids this confusion.

The following annotation informs GCC that the function accepts a printf-style format string as parameter number 1 and parameters 2 and following are the format arguments.

std::string
format(const char *const format, ...)
  __attribute__ ((format (printf, 1, 2)));

See the GCC manual for more information about the (arguably messy) syntax.

Document your interface

Add a comment to the declaration of each public function that describes precisely how to use it.

Putting it all together

Combining all of the above advice, we end up with the following two files.

my_format.hxx:

#ifndef MY_FORMAT_HXX
#define MY_FORMAT_HXX

#include <string>  // std::string

namespace my
{

  /**
   * @brief
   *         Like `std::sprintf` except that the formatted string is `return`ed
   *         as a `std::string`.
   *
   * This function internally forwards to `std::vsnprintf`.  It accepts the
   * exact same parameters and has the exact same gotchas as that function
   * except that allocating a buffer of sufficient size is done automatically.
   *
   * @param format
   *         C-style format string
   *
   * @param ...
   *         any format arguments required by the given format string
   *
   * @returns
   *         the formatted string
   *
   * @throws std::runtime_error
   *         if `std::vsnprintf` fails
   *
   * @throws std::bad_alloc
   *         if insufficient memory is available
   *
   */
  std::string
  format(const char *const format, ...)
    __attribute__ ((format (printf, 1, 2)));

}  // namespace my

#endif  // #ifndef MY_FORMAT_HXX

my_format.cxx:

#include "my_format.hxx"

#include <cstdarg>    // va_start, va_end, std::va_list
#include <cstddef>    // std::size_t
#include <stdexcept>  // std::runtime_error
#include <vector>     // std::vector

namespace my
{

  std::string
  format(const char *const format, ...)
  {
    auto temp = std::vector<char> {};
    auto length = std::size_t {63};
    std::va_list args;
    while (temp.size() <= length)
      {
        temp.resize(length + 1);
        va_start(args, format);
        const auto status = std::vsnprintf(temp.data(), temp.size(), format, args);
        va_end(args);
        if (status < 0)
          throw std::runtime_error {"string formatting error"};
        length = static_cast<std::size_t>(status);
      }
    return std::string {temp.data(), length};
  }

}  // namespace my


Caching a function view for a day I am writing a flask app that I will deploy it on Heroku to test it. There is a page that will generate daily photos and information based on a JSON file. The view is valid for 24 hours beginning from 00:00 to 23:59 of everyday.

I am thinking about using a cache, otherwise the app will consume unnecessary resources.

import ..

TODAY = date.today()

 app = Flask(__name__)
 app.jinja_env.auto_reload = False
 cache = Cache(config={'CACHE_TYPE': 'simple', 'CACHE_ARGS' : TODAY})
#This function reads a json file, filter the daily data to process and render a html template with photos and text .

# 3600 *  24 = 86400
@cache.cached(timeout=86400)
@app.route('/inf')
def inf():
    input_file = open(os.path.join(__location__, 'json/output.json'));
    j = json.loads(input_file.read().decode("utf-8-sig"))
    etc ..

    return render_template('inf.html',
                        title = title,
                        config={'environment_args':{'autoescape':False}},
                        zipped =zipped)

if __name__ == '__main__':
    if TODAY != cache.config['CACHE_ARGS'] :
        with app.app_context():
            cache.clear()
    port = int(os.environ.get('PORT', 33507))
    app.run(debug = True, host='0.0.0.0', port=port)

I am not sure if this is the best solution to cache the view for a day.  That's why I am asking here for more suggestions.

Why not take advantage of the RESTful architecture of the internet? I suggest you have the code generate a static page and set the expires header on that resource (or set of resources) to the end of that day (considering your audience's time zones, I guess). This might help.

Sorry, if I had experience with heroku or flask I would provide code or more details.


Learning F# with Prime Factorization In my quest to learn F#, I implemented Prime Factorization.

First, the code:

let primefactors x =

    let check = seq { let limit = uint64((ceil(sqrt(float(x)))));
                      for x in Seq.concat [seq { yield 2UL }; { 3UL .. 2UL .. limit }] do
                        yield x }

    let getFirstOrDefault def ns =
        if ns |> Seq.isEmpty then
            def
        else
            ns |> Seq.head

    let nextfactor x =
        match x with
        |1UL -> x
        |_ -> check
              |> Seq.skipWhile(fun idx -> x % idx <> 0UL)
              |> getFirstOrDefault x

    let rec getfactors x factors =
        match nextfactor x  with
        |1UL -> factors
        |factor -> (factors |> List.append [factor]) |> getfactors (x / factor)

    [] |> getfactors x

let smallFactors = primefactors 3672215407307975928UL
let bigPrimeFactor = primefactors 18446744073709551556UL
let biggestuint64Prime = primefactors 18446744073709551557UL

Im satisfied with the performance, but have some gripes with this code that I'd like feedback on:

I'm checking for 1UL in two places. In nextfactor it's there to skip the last run through the numbers; in getfactors it's the break condition. I don't feel like this is as elegant as it could be.

Another thing I don't like is the getFirstOrDefault function. At first I simply had a Seq.head after the Seq.skipWhile but that throws if the number itself is prime and so the sequence is empty. I'd like to do pattern matching with that as I would with lists but in that case I'd have to materialize the whole sequence which I'd like to avoid. Is there something like |[] -> ... for sequences?

I would also like to hear suggestions on how to construct the check sequence in a more elegant way.

I am aware that the algorithm itself is a pretty naive implementation of the problem, but I'm ok with that as it's (for me) sufficiently fast in the range of uint64. If you do have suggestions on how to improve performance while keeping with this simple algorithm, I'd love to hear them, though!

I'm also interested in hearing where I'm missing functional concepts that might be applicable here.

I think you show a good understanding of F# as a language and functional programming in general as far as I can see. A couple of things though:

You use x as variable name on multiple levels of function definitions. It makes it hard to read.

IMO match .. with is a very readable construct for more than two matches. But for only two I prefer if-statements.

The check function/sequence is a kind of double sequence.
I would do it this way:

let primefactors target =
    let check ta = Seq.concat [seq { yield 2UL }; { 3UL .. 2UL .. uint64(ceil(sqrt(float(ta))))}]

and then call it like this:

let nextfactor x =
    match x with
    |1UL -> x
    |_ -> check x
          |> Seq.skipWhile(fun idx -> x % idx <> 0UL)
          |> getFirstOrDefault x

It will make the function much faster.

The getfactors - function is not tail-recursive, but can easily be changed to be:

let rec getfactors x factors =
    match nextfactor x  with
    |1UL -> factors
    // Here the factors list is handled before the recursive call.
    |factor -> getfactors (x / factor) (factor :: factors)


Excel file format converter I'm using C# .NET Core 2.1 and Excel lib DotnetCore.NPOI

I'm converting xls file to xlsx by loading xls and copying sheet by sheet, cell by cell to xlsx - How can I improve performance / ram usage here?

private string ConvertToXSLX(string path)
{
    using (var fs = File.OpenRead(path))
    {
        var result = XLS_to_XLSX_Converter.Convert(fs);

        // (file.xls) + x = file.xlsx
        path = $"{path}x";

        using (var fs2 = new FileStream(path, FileMode.OpenOrCreate, FileAccess.Write))
        {
            fs2.Write(result, 0, result.Length);
        }
    }
    return path;
}

public static byte[] Convert(Stream sourceStream)
{
    var source = new HSSFWorkbook(sourceStream);
    var destination = new XSSFWorkbook();

    for (int i = 0; i < source.NumberOfSheets; i++)
    {
        var hssfSheet = (HSSFSheet)source.GetSheetAt(i);

        var xssfSheet = (XSSFSheet)destination.CreateSheet(source.GetSheetAt(i).SheetName);

        CopyRows(hssfSheet, xssfSheet);
    }

    using (var ms = new MemoryStream())
    {
        destination.Write(ms);
        return ms.ToArray();
    }
}

private static void CopyRows(HSSFSheet hssfSheet, XSSFSheet destination)
{
    for (int i = 0; i < hssfSheet.PhysicalNumberOfRows; i++)
    {
        destination.CreateRow(i);

        var cells = hssfSheet.GetRow(i)?.Cells ?? new List<ICell>();

        for (int j = 0; j < cells.Count; j++)
        {
            var row = destination.GetRow(i);
            row.CreateCell(j);

            CopyCell((HSSFCell)cells[j], (XSSFCell)row.Cells[j]);
        }
    }
}

private static void CopyCell(HSSFCell oldCell, XSSFCell newCell)
{
    CopyCellValue(oldCell, newCell);
}

private static void CopyCellValue(HSSFCell oldCell, XSSFCell newCell)
{
    switch (oldCell.CellType)
    {
        case CellType.String:
            newCell.SetCellValue(oldCell.StringCellValue);
            break;
        case CellType.Numeric:
            newCell.SetCellValue(oldCell.NumericCellValue);
            break;
        case CellType.Blank:
            newCell.SetCellType(CellType.Blank);
            break;
        case CellType.Boolean:
            newCell.SetCellValue(oldCell.BooleanCellValue);
            break;
        case CellType.Error:
            newCell.SetCellErrorValue(oldCell.ErrorCellValue);
            break;
        default:
            break;
    }
}

You're writing to a MemoryStream only to get a byte[] to write to another stream, you can definitely avoid this intermediate conversion:

private string ConvertToXSLX(string inputPath)
{
    string outputPath = Path.ChangeExtension(inputPath, ".xlsx");

    using (var input = File.OpenRead(inputPath))
    using (var output = new FileStream(outputPath, FileMode.OpenOrCreate, FileAccess.Write))
    {
        XLS_to_XLSX_Converter.Convert(input, output);
    }

    return outputPath;
}

The change in Convert() should be trivial.

Also note that I'm using Path.ChangeExtension() instead of manually adding a character, code is clear without any comment and it handles special cases for you (for example a trailing space).

In CopyRows() you create an empty List<ICell>, you do not need to and you can avoid the initial allocation simply using Enumerable.Empty<ICell>() (and changing your code to work with an enumeration) or - at least - reusing the same empty object (move it outside the loop and create it with an initial capacity of 0).

In CopyCellValue() you have an empty default case for your switch. It's a good thing to always have default but it has a purpose: detect unknown cases. If you put a break then you're silently ignoring them. Is it on purpose? Write a comment to explain you ignore unknown cells. It's an error? Throw an exception.

I never used DotnetCore.NPOI then I can't comment about the way you use it, be sure to dispose all the intermediate IDisposable objects.


Follow up - Decide whether a triangle is isosceles or not Previous question: Decide whether a triangle is isosceles or not

Using the comments on the previous question (from people such as Carcigenicate, Luke Sawczak, dgnuff, alecxe and 200_success) I have revised the code and made an improved version but I'm sure that there is still more that I can do to improve it. (And while the calls to sleep and the 'print("")' may be unnecessary, I prefer to write my code with them to make the output look better).

import time

def askSideLength(sideMessage):
    while True:
        try:
            length = float(input("How long is the " + sideMessage + " side of the triangle? "))
            if length <= 0.0:
                print("This is an impossible triangle!")
                time.sleep(0.5)
            else:
                return length
        except ValueError:
                print("That's not a number...")
                time.sleep(0.5)

def findTriangleType():
        if side1 == side2:
            print("This is an isosceles triangle!")
        elif side1 == side3:
            print("This is an isosceles triangle!")
        elif side2 == side3:
            print("This is an isosceles triangle!")
        elif side1 != side2 and side1 != side3:
            print("This is not an isosceles triangle!")
        elif side2 != side1 and side2 != side3:
            print("This is not an isosceles triangle!")
        elif side3 != side1 and side3 != side2:
            print("This is not an isosceles triangle!")

print("""I am going to ask you for three numbers.
These numbers can be integers or decimals.
They will be the sides of a triangle,
and I will tell you if it is an isosceles triangle or not.
""")
time.sleep(2.5)
side1 = askSideLength("first")
time.sleep(0.25)
side2 = askSideLength("second")
time.sleep(0.25)
side3 = askSideLength("third")
print("")
time.sleep(1)

semiSum = (side1 + side2 + side3) / 2.0
areaSquared = semiSum * (semiSum - side1) * (semiSum - side2) * (semiSum - side3)

if side1 == side2 == side3:
    print("""While this triangle is technically an isosceles triangle,
it is much more commonly known as an equilateral triangle.""")
elif areaSquared <= 0.0:
    print("This is an impossible triangle!")
elif areaSquared > 0.0:
    findTriangleType()
time.sleep(2.5)

Law of Excluded Middle

Something is either an isosceles triangle or it isn't. (This is a consequence of Law of Excluded Middle). Given this knowledge, as an exercise, what should you be able to eliminate from this code:

    if side1 == side2:
        print("This is an isosceles triangle!")
    elif side1 == side3:
        print("This is an isosceles triangle!")
    elif side2 == side3:
        print("This is an isosceles triangle!")
    elif side1 != side2 and side1 != side3:
        print("This is not an isosceles triangle!")
    elif side2 != side1 and side2 != side3:
        print("This is not an isosceles triangle!")
    elif side3 != side1 and side3 != side2:
        print("This is not an isosceles triangle!")

Don't sleep on me.

There really is no reason to write:

time.sleep(2.5)
side1 = askSideLength("first")
time.sleep(0.25)
side2 = askSideLength("second")
time.sleep(0.25)
side3 = askSideLength("third")
time.sleep(1)

The prompt will wait for the user to input the data, don't make the user wait.

Naming conventions

askSideLength for example should be converted to ask_side_length. We don't use the camelCase for functions and variables in Python. Instead we use snake_case.


Print a sequence of n numbers I have a simple function that takes an Integer n and return a string representing the array from 1..n.

nthLine :: Integer -> String
nthLine n = intercalate " " $ map show [1..n]

The main function takes input n from the user and prints the nthLine out to screen:

getInteger :: String -> Integer      # This seems redundant
getInteger xs = (read xs)::Integer

nthLineStr :: String  -> String      # And so does this function
nthLineStr xs = nthLine (getInteger xs )

main = do line <- fmap nthLineStr getLine
          putStrLn line

Any tips on how this may be simplified/made better?

You can use unwords in place of intercalate " ".

Given the type signature of getInteger :: String -> Integer the return type of read xs must be Integer in order for the type system to unify what you've written, making your type annotation on the function implementation there redundant. You can perform eta-reduction on getInteger then (that is, write it in pointfree style) and drop the xs for an implementation of—

getInteger :: String -> Integer
getInteger = read

This is now obviously a specialization of read, which isn't all that interesting. Look now to the implementation of nthLineStr where you can see that the result of getInteger xs is passed to nthLine. Because the type of nthLine is given as Integer -> String, the compiler can again determine that read must return an Integer if you change the definition of nthLineStr to—

nthLineStr :: String -> String
nthLineStr xs = nthLine (read xs)

You can perform eta-reduction again to write nthLineStr as nthLineStr = nthLine . read. nthLineStr is a bit of a kludge though, in that it muddies the waters between your pure code and the outside world. Keep your (de-)serialization functions apart from the rest of your program, it shouldn't matter to the core logic of your application whether you were passed a String or a Text value or length encoded sequence of unit values ([()]).

main :: IO ()
main = do n <- fmap read getLine
       putStrLn $ nthLine n

Or you could use another Prelude function, interact :: (String -> String) -> IO () (a useful concept, but note this changes the semantics a bit).

main :: IO ()
main = interact (nthLine . read)


Live character count for EditText Could you review it and said if it's written good and/or is better way to do that?

My algorithm is:

Input roomNumber
   If length of roomNumber is 8 set goToRoom state Enabled and color 0xFFFFFFFF
   Else set goToRoom state Disabled and color 0xBBFFFFFF

Code of that algorithm is:

public class Join_room_screen extends Activity {

    EditText numberRoom;
    Button goToRoom;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);

        setContentView(R.layout.joinroom);

        numberRoom = (EditText) findViewById(R.id.roomNumber);
        goToRoom = (Button) findViewById(R.id.goToRoom);

        TextWatcher watcher = new LocalTextWatcher();
        goToRoom.addTextChangedListener(watcher);
        updateButtonState();
    }

    void updateButtonState() {
        boolean enabled = checkEditText(numberRoom);
        if (enabled) {
            goToRoom.setBackgroundColor(0xFFFFFFFF);
            goToRoom.setEnabled(enabled);
        } else {
            goToRoom.setBackgroundColor(0xBBFFFFFF);
            goToRoom.setEnabled(false);
        }
    }

    private boolean checkEditText(EditText edit) {
        return ((edit.getText().toString()).length() == 8 );
    }

    private class LocalTextWatcher implements TextWatcher {
        public void afterTextChanged(Editable s) {
            updateButtonState();
        }

        public void beforeTextChanged(CharSequence s, int start, int count, int after) {
        }

        public void onTextChanged(CharSequence s, int start, int before, int count) {
        }
    }
}

Here is my list of improvements:

Encapsulate all the logic in a custom textwatcher. This gives you the benefit to make the EnableViewTextWatcher reusable in other parts of your project (if you extract it from the Activity).
Programming against TextView vs EditText and View vs Button will make the code more flexible.
Limit visibility by using private keyword (class members, parameters and local variables)
Make class members, params and local variables final is possible
Follow the general Android coding guidelines regarding naming.
Use meaningful names when naming class, class members, params, and local variables
Use annotations in your code to improve code quality
Use constants instead of Magic Numbers.

public class RoomSelectActivity extends Activity {

    @Override
    protected void onCreate(@Nullable final Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_select_room);

        final TextView roomNumberText = (TextView) findViewById(R.id.tv_room_number);
        final View enabledView = findViewById(R.id.btn_go_to_room);

        final TextWatcher watcher = new EnableViewTextWatcher(roomNumberText, enabledView);
        roomNumberText.addTextChangedListener(watcher);
    }

    private static class EnableViewTextWatcher implements TextWatcher {

        private static final int LENGTH_TO_ENABLE_VIEW = 8;
        private static final int ENABLED_TEXT_COLOR  = 0xFFFFFFFF;
        private static final int DISABLED_TEXT_COLOR = 0xBBFFFFFF;

        private final TextView mWatchedTextView;
        private final View     mEnableView;

        public EnableViewTextWatcher(@NonNull final TextView textViewToWatch, @NonNull final View viewToEnable) {
            mWatchedTextView = textViewToWatch;
            mEnableView = viewToEnable;
            updateEnabledViewState();
        }

        public void afterTextChanged(@NonNull final Editable s) {
            updateEnabledViewState();
        }

        public void beforeTextChanged(@NonNull final CharSequence s, final int start, final int count, final int after) {
        }

        public void onTextChanged(@NonNull final CharSequence s, final int start, final int before, final int count) {
        }

        private boolean checkTextView(@NonNull final TextView textView) {
            return ((textView.getText().toString()).length() == LENGTH_TO_ENABLE_VIEW);
        }

        private void updateEnabledViewState() {
            final boolean enabled = checkTextView(mWatchedTextView);
            mEnableView.setBackgroundColor(enabled ? ENABLED_TEXT_COLOR : DISABLED_TEXT_COLOR);
            mEnableView.setEnabled(enabled);
        }
    }
}


A countdown clock in TypeScript The gist of the code is, you give it a date, it will start counting down how many days, hours, minutes and seconds left by emitting an event 'countDown' and once 0 is reached 'expired' event is emitted.

How can this code be improved?

class DateCountDown {
    listeners: Map<string, Array<Function>>;

    constructor(date: Date) {

        this.startCountDown(date);
        this.listeners = new Map();
        this.listeners.set("countDown", []);
        this.listeners.set("expired", []);
    }

    on(eventName: "countDown", listener: (values: CountDownValues) => void);
    on(eventName: "expired", listener: (isExpired: boolean) => void);
    on(eventName: string, listener: Function) {
        this.listeners.get(eventName).push(listener);
    }

    startCountDown(date: Date) {
        const thousand = 1000;
        const sixty = 60;
        const twentyfour = 24;

        const timer = setInterval(() => {
            const now = new Date().getTime();
            const t = date.valueOf() - now;

            const days = Math.floor(t / (thousand * sixty * sixty * twentyfour));
            const hours = Math.floor((t % (thousand * sixty * sixty * twentyfour)) / (thousand * sixty * sixty));
            const minutes = Math.floor((t % (thousand * sixty * sixty)) / (thousand * sixty));
            const seconds = Math.floor((t % (thousand * sixty)) / thousand);

            if (t <= 0) {
                clearInterval(timer);
                this.listeners.get("expired").forEach(listener =>
                    listener(true));
                return;
            }

            this.listeners.get("countDown").forEach(listener =>
                listener(new CountDownValues(days, hours, minutes, seconds))
            );
        }, 1000);
    }
}
    class CountDownValues {
    days: number;
    hours: number;
    minutes: number;
    seconds: number;

    constructor(days: number, hours: number, minutes: number, seconds: number) {
        this.days = days;
        this.hours = hours;
        this.minutes = minutes;
        this.seconds = seconds;
    }

}
var date = new Date();
date.setSeconds(date.getSeconds() + 20);

let countDown = new DateCountDown(date);

countDown.on("countDown", values => {
    console.log("from first", values);
});
countDown.on("expired", isExpired => {
    console.log("expired:", isExpired);
});

First, taking a look at your code as a consumer of your API:

I like that the autocomplete for .on(" shows what events I can listen to.
How do I stop listening to an event? If I don't want countDown events anymore then I have to do countDown.listeners.get("countDown")!.splice(index, 1) which is ugly.
How do I cancel a countdown?
Why is listeners exposed? This is an internal property of your class that I shouldn't need to know about as a consumer. It should be either private or protected. It's also probably worth marking startCountDown as private or protected.
Why does the expired event pass an extra parameter, isExpired? If the expired event is fired, doesn't that imply that the countdown is expired?
The countDown event is not fired every second. What happened to second 9 and 6?

This is a common problem with timers, and is present in (almost) every JavaScript clock/timer question on this site. There are several possible solutions, the simplest of which is to check the time more frequently. The MDN notes on it is worth reading.

[![broken countdown ticks][1]][1]

TypeScript specific notes:

Turn on strict null checks. Yesterday. Without it, TypeScript ignores a whole class of errors.
Turn on no implicit any. Once turned on this will point out that the on method has a return type of any
Don't create a class to just hold properties. An interface is fine.

interface CountDownValues {
    days: number;
    hours: number;
    minutes: number;
    seconds: number;
}

Consider initializing properties where they are declared when using classes.

Other:

new Date().getTime() can be more concisely written as Date.now()
The thousand, sixty, and twentyfour variables make it harder to read your code. Get rid of them. You can make the code more readable by instead defining day, hour, etc.
Countdown is a word, you don't need to capitalize the D.
Using a map for events here is probably overkill. There are only two events and they are both known at compile time. I'd just use an object.
countDown is kind of an odd name for an event, without looking at your code I had no idea when it would be fired. I prefer tick, but there are other good options.

Rewrite:

I kept the feature set mostly the same, but wanted to add the ability to stop the timer, remove event listeners, and initialize a countdown before starting it. I let this run for ~15 minutes, and it never skipped a second or repeated one twice, which isn't conclusive, but I'm fairly confident it is stable.

Note that instead of firing the tick function multiple times per second, I do some math to figure out how long to wait and then schedule a tick based on that. This results in some more complicated code, but avoids the need to track what the last reported second was.

interface TickEvent {
    days: number;
    hours: number;
    minutes: number;
    seconds: number;
}

interface CountdownEvents {
    tick(values: TickEvent): void;
    expired(): void;
    stop(): void;
}

// Unfortunately we can't use T[K][] without getting messy.
type EventMap<T> = { [K in keyof T]: Function[] };

class Countdown {
    private listeners: EventMap<CountdownEvents> = { tick: [], expired: [], stop: [] };
    private timer?: number;

    on<K extends keyof CountdownEvents>(eventName: K, listener: CountdownEvents[K]): void {
        this.listeners[eventName].push(listener);
    }

    off<K extends keyof CountdownEvents>(eventName: K, listener: CountdownEvents[K]): void {
        const listeners = this.listeners[eventName];
        const index = listeners.indexOf(listener);
        if (index !== -1) {
            listeners.splice(index, 1);
        }
    }

    start(date: Date) {
        const end = Math.floor(date.getTime() / 1000);

        const tick = () => {
            const now = Date.now();
            const nowSec = Math.floor(now / 1000);
            const time = end - nowSec;

            if (time <= 0) {
                delete this.timer;
                this.listeners.expired.forEach(listener => listener());
                return;
            }

            const minute = 60;
            const hour = minute * 60;
            const day = hour * 24;

            const days = Math.floor(time / day);
            const hours = Math.floor(time % day / hour);
            const minutes = Math.floor(time % hour / minute);
            const seconds = time % minute;

            this.listeners.tick.forEach(listener => listener({ days, hours, minutes, seconds }));

            const timeToNextSecond = (nowSec + 1) * 1000 - now;
            this.timer = setTimeout(tick, timeToNextSecond);
        }

        tick();
    }

    stop() {
        if (this.timer) {
            clearTimeout(this.timer);
            delete this.timer;
            this.listeners.stop.forEach(listener => listener());
        }
    }
}

const countdown = new Countdown();
countdown.on("tick", event => console.log("tick", event));
countdown.on("expired", () => console.log("expired"));
countdown.on("stop", () => console.log("stopped"));

const date = new Date();
date.setSeconds(date.getSeconds() + 20);
countdown.start(date);

setTimeout(() => countdown.stop(), 3 * 1000);


Finding shortest sequences of integers whose product is larger than n Here is the task:

For an integer n, return a list of the k length integers sequences whose product is larger than n but as close to n as possible (i.e. smallest larger than n number), and whose largest member is the smallest possible. For each sequence length k, there is one such sequence, and we want to have the sequences for all possible k's. 1 doesn't count as a valid member. The sequences are ordered from largest to smallest member.

For example, for n=100, the longest non-trivial sequence is with k=7 and is [2, 2, 2, 2, 2, 2, 2]. sequences with k>7 will be just an extension to this sequence (i.e. additional 2's appended to this list) and thus we will disregard them as trivial and will not care for them.

For k=2 the sequence of choice for n=100 is [10, 10]. It is better than [12, 9] since the largest member is smaller. If the largest member is the same in two sequences of the same length then the decision moves to the next largest, etc.

The shortest sequence for n=100 is obviously the trivial [100].

And the full list for n=100 is:

In: productsequences(100)

out:
[[2, 2, 2, 2, 2, 2, 2],
 [3, 3, 2, 2, 2, 2],
 [3, 3, 3, 2, 2],
 [4, 3, 3, 3],
 [5, 5, 4],
 [10, 10],
 [100]]

This is the recursive code I've came up with. It seems to be working well. However for large numbers it becomes slow.

How can I improve it? Can the same be achieved faster for large numbers?

def productsequences(n, maxn=None):
    """
    Returns a list of valid integer sequnces of different lengths whose
    product is the smallest larger than n.
    """
    if n <= 1:
        return []
    if maxn == None:
        maxn = n
    seqlist= []
    for i in range(2, min(maxn, n) + 1):
        if n % i:
            r = productsequences((n // i) + 1, i)
        else:
            r = productsequences(n // i, i)
        if len(r) == 0:
            seqlist.append([i])
        else:
            for k in r:
                seqlist.append([i] + k)
    nontrivial = []
    lim = n # Just a large enough initial value. needs to be larger than
    # log(n, 2) + 1
    for k in seqlist: # keep just one sequnce of every length
        if len(k) < lim:
            nontrivial.append(k)
            lim = len(k)
    return nontrivial

Yes, you can improve that algorithm, by optimizing the following inequation:

$$n \le \prod_{i=1}^{k}{\lfloor\sqrt[k]{n}\rfloor+b_i} \quad \text{with} \quad b_i \in \{0, 1\}\land b_i >= b_{i+1}$$

Just initialize the guess for all factors for a given \$k\$ with \$\lfloor\sqrt[k]{n}\rfloor+1\$, and replace the factors one by one with \$\lfloor\sqrt[k]{n}\rfloor\$ while you are still above \$n\$.

That means you will effectively only need to calculate one root per \$k\$, followed by at most \$k\$ divisions, multiplications and comparisons.

Your problem would be far more complicated if being closer to \$n\$ had precedence over using the smallest factors. However, if only smallest factors are demanded, the \$k\$-th root of \$n\$ is the best approaximation you can get. Otherwise the problem would be a derivative of prime number factorization.


Simple math quiz in C# I am only a beginner when it comes to programming, but after reading tons of tutorials on and offline, I have been able to create this simple math quiz. It does work as intended, and I'm really just fishing for someone to yell at me for doing something glaringly bad practice in the code below.

using System;

namespace ForthConsoleProject
{
    class MainClass
    {
        public static void Main(string[] args)
        {
            Random randomgen = new Random();

            //int timer = 30; I considered a timer function but that ended up being way over my head.
            int num01 = randomgen.Next(11);
            int num02 = randomgen.Next(11);
            int playeranswer;
            int answer;
            int numofquestions;
            int numofquestionsleft;
            int numofcorrect = 0;

            Console.Write("How many questions would you like to answer? ");
            numofquestions = Convert.ToInt32(Console.ReadLine());
            numofquestionsleft = numofquestions;

            // This is the loop which handles the actual question/answer core of the game.
            // Answering a question correctly increases your score.
            while (numofquestionsleft > 0){

                Console.Write("What is " + num01 + " times " + num02 + "? ");
                answer = num01 * num02;
                playeranswer = Convert.ToInt32(Console.ReadLine());
                if (answer == playeranswer)
                {
                    Console.WriteLine(playeranswer + " is correct!");
                    numofcorrect++;
                }
                else {
                    Console.WriteLine(playeranswer + " is incorrect! Try again.");
                }
                Console.ReadKey();
                numofquestionsleft--;
                num01 = randomgen.Next(11);
                num02 = randomgen.Next(11);
            }
            // Letting the user know how many answers they got right.
            Console.WriteLine("You got " + numofcorrect + " of " + numofquestions + " correct!");
            Console.ReadKey();

        }
    }
}

This is pretty nice. I got some comments, some of which are my personal opinion (for example how to declare variables), some are accepted conventions (naming), and some are bug fixes.

Declaring variables

While it might be more performance friendly to declare all variables from the outset, it is best practice, I think, to declare variables only within the scope they're used. This is to decrease variable pollution in bigger functions. In such a simple app, it doesn't matter. But as your application gets bigger and better you don't want too many variables in scope.

From your code: num01, num02, playeranswer, and answer are only used within the loop. So it is better to declare then inside the loop (or you might accidentally use them outside or simply crowd your intellisense).

Also, for some variables you're splitting declaration (int numofquestions;) and assignment (numofquestions = Convert.ToInt32(Console.ReadLine());), and for other you're not. I think it is better to have declaration and assignment in one line, it is terser and more readable: (int numofquestions = Convert.ToInt32(Console.ReadLine());).

Naming

.Net convention is to use PascalCase for methods and classes and the like, and camelCase for variables. So, in your code: playeranswer becomes playerAnswer, and so on. You don't have any methods defined but you need to know the convention. It makes it easier to read your code. Again, it doesn't really matter for such a short app but for bigger apps such practices are a life saver. (A crude example, but I am sure you have seen the photo where KidsExchange reads like KidSexChange due to uniform capitalization KIDSEXCHANGE.)

I also like questionCount more than numOfQuestions. Shorter and uses clearer vocab.

Using a for loop

for loops are easier for humans to process than while loops, I think. Well easier for me.

So this :

int questionCount= Convert.ToInt32 (Console.ReadLine ());
int questionLeftCount = questionCount;

while (questionLeftCount > 0)
{
    // .. do stuff
    questionLeftCount --;
}

Can be better written as:

int questionCount= Convert.ToInt32 (Console.ReadLine ());

for (int questionLeftCount = questionCount; questionLeftCount > 0; questionLeftCount--)
{
    // do stuff ..
}

And it is exactly the same.

Also, since questionLeftCount is only used within the for loop definition, just use i or iter for iterator. It is standard practice. (It also standard practice to increment rather decrement). I am sure you have seen something that looks exactly like this in your readings.

for (int i = 0; i < questionCount; i++)
{
    // do stuff ..
}

Much shorter and nicer.

Verifying input

Your code crashes when I type something that is NOT a number. Obviously if you're shipping the app (I know you're not, but humor me), you can't have that.

The trick is to use int.TryParse to parse inputs. TryParse returns a bool to indicate whether the conversion is successful or not, and has an out variable which assigns a value to an already declared variable. The syntax is a bit weird and takes some getting used to, but it is being made nicer in C# 7.0 (not released yet).

// Usage
int playerAnswer;

if (int.TryParse (Console.ReadLine (), out playerAnswer))
{
    // do stuff with playerAnswer
}
else
{
    // show error message
}

If you want to keep asking for input until you get something valid, you can use a while loop :

 int playerAnswer;

 while (!int.TryParse (Console.ReadLine (), out playerAnswer))
 {
     Console.WriteLine ("Invalid Input. Please type a number using digits");
 }

Something of note: you need this little piece of code in two places. When you ask for the number of questions and when you ask for the answer. Instead of typing it twice, you should extract it into a method. You can easily do that in Visual Studio by selecting the code you want to extract into a method, pressing Ctrl + . or the small light bulb to the side, and selecting, well, Extract Method. It does all the thinking for you and it is fantastic.

I am not very imaginative so I just called it RequestInput.

static int RequestInput ()
{
    int playerAnswer;

    while (!int.TryParse (Console.ReadLine (), out playerAnswer))
    {
        Console.WriteLine ("Invalid Input. Please type a number using digits");
    }

    return playerAnswer;
}

Final result

class MainClass
{
    public static void Main ()
    {
        var rand = new Random ();

        Console.Write ("How many questions would you like to answer? ");

        int correctCount = 0;
        int questionCount = RequestInput ();

        for (int i = 0; i < questionCount; i++)
        {
            int num01 = rand.Next (11);
            int num02 = rand.Next (11);

            Console.Write ($"What is {num01} times {num02} ?");

            int playerAnswer = RequestInput ();

            if (num01 * num02 == playerAnswer)
            {
                Console.WriteLine ($"{playerAnswer} is correct!");
                correctCount++;
            }
            else
            {
                Console.WriteLine ($"{playerAnswer} is incorrect!");
            }
        }

        Console.WriteLine ($"You got {correctCount} of {questionCount} correct.!");
        Console.ReadKey ();

    }

    static int RequestInput ()
    {
        int playerAnswer;
        while (!int.TryParse (Console.ReadLine (), out playerAnswer))
        {
            Console.WriteLine ("Invalid Input. Please type a number using digits");
        }

        return playerAnswer;
    }
}

I did a few other things I like but that are just style choices and mild annoyances (for me). Using var instead of Random, for example, makes it easier for me to read, since Visual Studio has the same coloring for keywords like var and int but different for class names like Random. I also removed Console.ReadKey () from within the loop because it annoyed me when trying out your app. I removed Try Again because you didn't actually let me try again the same question. You could make it possible to try until you get the right answer in a similar pattern to the one dome earlier when verifying input. Finally, I used string interpolation syntax (introduced in C# 6.0) which is much, much, more pleasant to use.

Bonus, how to set a timer

There is a Timer class, which I have never used. (I am a beginner myself, mind you). There is also System.Diagnostics.StopWatch, which as you can tell is used for diagnostics.

Here is the code with Stopwatch. Mind that you'll need using System.Diagnostics at the top of the file.

class MainClass
{
    public static void Main ()
    {
        var rand = new Random ();

        Console.Write ("How long do want to play in seconds?");

        var ts = TimeSpan.FromSeconds(RequestInput ()); // type is System.TimeSpan

        int correctCount = 0;
        int questionCount = 0;

        var sw = Stopwatch.StartNew ();

        while (sw.Elapsed < ts)
        {
            // .. same old
            questionCount++;
        }
        sw.Stop ();

        Console.WriteLine ($"You got {correctCount} of {questionCount} correct in {sw.Elapsed}.!");
        Console.ReadKey ();

    }
}

You will notice I used Stopwatch.StartNew() which is a static method that creates a new instance of Stopwatch and starts it immediately. You could also do var sw = new Stopwatch(); sw.Start();

Edit: i realized there was a bug in my original implementation because I didn't understand how the TimeSpan struct works. I think it should work as expected now for longer times that 1 minute. Make sure to check the MSDN documentation

Let me know in the comments if there is anything unclear.

Edit : On the other answer, I think creating new classes for a simple console app is overkill, but it is definitely good practice for larger apps and it allows you to extend your game more flexibly.


Animal, Dog, and IDangerous - Using interfaces and inheritance with C# I'm quite new to object-oriented C#. I wanted to test my knowledge by creating a Dog/Animal classes. I also tried to implement an IDangerous interface in my code.
Is there anything I could've done better/differently? Again, this is my first time on this Stack Exchange so if I could've restructured this post differently please tell me.
public class Animal
{
    public int Age { get; }
    public string Color { get; }
}
public class Dog : Animal
{
    public bool IsVaccinated { get; set; }
    public string Name { get; set; }
}
public interface IDangerous
{
    void TightenLeash(string Reason, int LeashTightness);
}
public class GoldenRetriever : Dog { }
public class Beagle : Dog { }
public class PitBull : Dog, IDangerous
{
    public void TightenLeash(string Reason, int LeashTightness)
    {
        Console.WriteLine("Leash tightened...");
    }
}
static class Program
{
    static void Main(string[] args)
    {
        Dog obj1 = new GoldenRetriever();

        obj1.Name = "Fluffy";
        obj1.IsVaccinated = true;
        Console.WriteLine(obj1.Name + obj1.IsVaccinated + obj1.Color);

        List<Dog> DogList = new List<Dog>()
        {
            new Beagle()
            {
                IsVaccinated = false,
                Name = "Korg"
            },
            new GoldenRetriever()
            {
                IsVaccinated = true,
                Name = "Juan"
            },
            new Beagle()
            {
                IsVaccinated = false,
                Name = "Toby"
            },
        };

        List<IDangerous> AngryPitBulls = new List<IDangerous>()
        {
            new PitBull()
            {
                IsVaccinated = false,
                Name = "Angry"
            },
            new PitBull()
            {
                IsVaccinated = false,
                Name = "Ryder"
            }
        };
        AngryPitBulls[0].TightenLeash("For biting another dog", 3);
        AngryPitBulls[1].TightenLeash("For biting the cat", 2);

        Console.ReadLine();
    }

}

Dog obj1 = new GoldenRetriever();

    obj1.Name = "Fluffy";
    obj1.IsVaccinated = true;
    Console.WriteLine(obj1.Name + obj1.IsVaccinated + obj1.Color);

Variable names: avoid name s like obj1 why not
Dog fluffy=new GoldenRetriever();
You have derived GoldenRetriever and Beagle from Dog. Obviously this is an incomplete example and you may have wanted to add further methods according to breed. In this case however it is often best to modify the implmentation at the time you are introducing the extra methods.
Unless there are different implementations best not to create new classes. So you could have had a member property Breed in the Dog class.
The IDangerous interface looks like it should be called  IDangerousDog
If you had a tiger in data tightening the leash is not a likely scenario except if tigers in the data can only be circus tigers. Even then I am not sure I would want to pull an angry tiger closer to me.
When you construct your Dogs it may be better to use a constructor to instantiate the Name and colour properties. This allows you then to have read only members. This is debateable in your example as you could argue that the name of a dog may change if the owner changes. It is best to keep properties immutable where they should be considered so.
If you stored the date of birth rather than the age then this could be immutable too.
It is much easier then to be confident that the age has been corrupted by a bug. Consider what would happen if a bug where to cause the Update age to be fired more than once or at the wrong time. It would be difficult to go back and identify what the correct age of a dog is. However if the Date of birth is stored you can always calculate the age and if there is a bug in the calculation you have not lost the value on which the age calculation is based.


Craps Simulator Exercise I did another exercise, this time it simulates n games of craps(dice game) and outputs the wins and win percentage!

#craps.py -- Simulates multiple games of craps
#   and estimates the probability that the player wins.

import random

def output(wins, total):
    winrate = wins / total
    print("The player won {0} of {1} games ({2:0.2%})".format(wins, total, winrate))

def rolldies():
    dice1 = random.randint(1, 6)
    dice2 = random.randint(1, 6)
    roll = dice1 + dice2
    return roll

def simOneGame():
    initial_roll = rolldies()
    if initial_roll == 2 or initial_roll == 3 or initial_roll == 12:
        return True     # won
    elif initial_roll == 7 or initial_roll == 11:
        return False    #lost
    else:
        #Roll until roll is 7 or initial roll
        roll = rolldies()
        while roll != 7 and roll != initial_roll:
            roll = rolldies()
        if roll == 7:
            return True     #won
        else:
            #roll is inital_roll
            return False    #lost

def simNGames(games_to_sim):
    wins = 0
    for i in range(games_to_sim):
        if simOneGame():
            wins += 1
    return wins

def main():
    games_to_sim = int(input("How many games to simulate? "))
    wins = simNGames(games_to_sim)
    output(wins, games_to_sim)

if __name__ == "__main__":
    main()

Always open for suggestions on what to improve/do differently or more pythonic.

First, Python has an official style-guide, PEP8, which programmers are encouraged to follow. This makes it easier for other Python programmers to read your code. PEP8 recommends using lower_case both for variable and function names, which I have used in the code below.

In your output function you use string formatting (good). However note that if you just want to paste the values in order, there is no need to explicitly mention the indices. So you could write "The player won {} of {} games ({:0.2%})".format(wins, total, winrate). But there is an even easier way in Python 3.6+: f-strings. I also think this is not enough to put it into a function, I would just leave it as a single line in main:

print(f"The player won {wins} of {total} games ({wins/total:0.2%})")

Your rolldies function could be made more general if it accepted a parameter telling it how many dice to roll. You can also use a list comprehension here, or random.choices:

def roll_dice(n=2):
    return sum(random.randint(1, 6) for _ in range(n))

DIE = [1, 2, 3, 4, 5, 6]
def roll_dice(n=2):
    return sum(random.choices(DIE, k=n))

Note that the singular is "die" and the plural is "dice", not "dies".

In your simOneGame function, instead of chaining a lot of comparisons with or, just use in:

def sim_game():
    initial_roll = roll_dice()
    if initial_roll in {2, 3, 12}:
        return True     # won
    elif initial_roll in {7, 11}:
        return False    # lost

Your rolling in the else branch can also be a bit shortened:

    else:
        #Roll until roll is 7 or initial roll
        roll = roll_dice()
        while roll not in {7, initial_roll}:
            roll = roll_dice()
        return roll == 7    # 7 wins

You could theoretically replace the while loop with a single random.choices call, but you would have to manually set the probabilities to the right values.

The simNGames function can be shortened a lot by using the fact that False == 0 and True == 1:

def sim_n_games(n):
    return sum(sim_game() for _ in range(n))


MS Access query complexity on finding duplicates I have a list of names and ID's, with many multiplication (it's a payroll list). I'd like to see those ID's and names where the name changed somewhen.
Input:
id1 name

1   Josephine Baker
2   Norma Jeane Morteson
2   Marilyn Monroe
33  Sandra Bullock
15  Issur Danielovitch
1   Freda Josephine McDonald
15  Kirk Douglas
28  Meg Ryan
28  Margaret Hyra
36  Vin Diesel
36  Mark Sinclair
8   Sean Connery
11  Mel Gibson
16  Julia Roberts
Desired output:
ID  Name

1   Freda Josephine McDonald
1   Josephine Baker
2   Marilyn Monroe
2   Norma Jeane Morteson
15  Issur Danielovitch
15  Kirk Douglas
28  Margaret Hyra
28  Meg Ryan
36  Mark Sinclair
36  Vin Diesel
I'm running this query. It works well, it is fast enough, I just have a feeling that this could be a lot more professional.
SELECT first(ST.id1) as ID , first(ST.nam) as Name
FROM Stars AS ST
INNER JOIN
       (select id1, first(nam), count(id1) from
            (select [ST1.id1]&[ST1.nam], first(ST1.id1) as id1, first(ST1.nam)  as nam,
                   count(*) from Stars ST1
             group by [ST1.id1]&[ST1.nam])
       group by id1
       having count(id1)>1) AS SJ
ON SJ.id1=ST.id1
GROUP BY ST.id1, ST.nam
ORDER BY ST.id1;

The logic behind is to select unique id&name pairs, then select those id's that come up more than once, then (self)join the table and select the unique names to those id's.
Any advise is welcome!

You'll have to take this with a huge grain of salt, because the following advice is for generic, standards-abiding SQL - which Access is not.
In regular SQL, this is as simple as:
select dupes.id1, ST1.nam
from (
    select id1
    from ST1
    group by id1
    having count(*) > 1
) dupes
join ST1 on ST1.id1 = dupes.ST1
order by dupes.id1;

Another way to do it without a join at all is a partition (window) function that adds a count column within each group. Unfortunately, the Access idea of a partition diverges significantly from the rest of reality, so this probably isn't possible for you. Regardless, for what it's worth, it would look like
select id1, nam
from (
    select id1, nam,
           count(*) over (partition by id1) as n
    from ST1
)
where n > 1
order by id1;

If at all possible, consider moving away from Access. It introduces more problems than it solves.


JSON storage of data for a to-do list application I am making a to-do list application in Python that will show tasks/subtasks/sub-sub-tasks in a tree view, but with higher-level clades of tasks grouped into blocks (e.g., Work block, Home block). So something like:

To Do List
|--Home Tasks
|  |--Clean room
|  |--Get storage unit
|
|--Work Tasks
  |--Analysis
  |--Sorting
     |--Save Files
     |  |--Pull from computer 1
     |  |--Walk to computer 2
     |  |--Save on computer 2
     |--Sort files
     |--Plot data

My goal is to store the data as JSON, but I am totally new to this format. I am curious about the quality of the following, and any improvements I could make:

{
    "todoList":
    [
    {
    "taskBlock": "Home", "tasks":
        [
        {"task": "Clean room", "done": false},
        {"task": "Get storage unit", "done": false}
        ]
    },
    {
    "taskBlock": "Work", "tasks":
        [
        {"task": "Analysis", "done": false},
        {"task": "Sorting", "done": false, "subtasks":
            [
            {"task": "Save files", "done": false, "subtasks":
                [
                {"task": "Pull from computer 1", "done": false},
                {"task": "Walk to computer 2", "done": false},
                {"task": "Save on computer 2", "done": false}
                ]
            },
            {"task": "Sort files", "done": false},
            {"task": "Plot data", "done": false}
            ]
        }
        ]
    }
    ]
}

I am used to XML, and defining a root item, but frankly I wonder if my "root" item here is superfluous (the 'todoList' parent item).

You are on the right track! Your hunch is correct that JSON doesn't really have a "root" element the way XML does. Actually, it does have a root - the opening {. Read that as "I am starting an object", the way you'd read the opening element in XML. (The thing about JSON that feels weird if you're used to XML is that you don't actually specify what type of object it is. That's the beauty [and danger] of playing fast and loose with types the way JavaScript does.)

Here's a suggested revision:

{
  "id": 100,
  "belongsToUser": "782950ce-f920-4f98-a69e-a899729e6267",
  "created": "2015-02-05T02:40:31Z",
  "lists": [{
      "name": "Home",
      "tasks": [
        {
          "name": "Clean room",
          "done": false

        }, {
          "name": "Get storage unit",
          "done": false
        }
      ]
    }, {
      "name": "Work",
      "tasks": [
        {
          "name": "Analysis",
          "done": false
        }, {
          "name": "Sorting",
          "done": false,
          "subtasks": [
            {
              "name": "Save files",
              "done": false,
              "subtasks": [
                {
                  "name":
                  "Pull from computer 1",
                  "done": false

                }, {
                  "name": "Walk to computer 2",
                  "done": false

                }, {
                  "name": "Save on computer 2",
                  "done": false

                }]
            },
            {
              "name": "Sort files",
              "done": false

            }, {
              "name": "Plot data",
              "done": false
            }]
        }]
    }]
}

A few things of note:

There's no need for a root todoList attribute, but there's plenty of other good metadata that could go at the root level: primary keys or IDs, timestamps, ownership information, and so on.
I think it makes sense to have a lists array at the root, and then have object for each clade (the Home and Work blocks) under that.
Each list object can have metadata of its own (name, creation timestamp, etc), in addition to an array of tasks.


Singly linked list delete() method I would like a quick review on my method to delete a node from a singly linked list. I'm fairly certain that this is not near optimally done, and would love to get feedback.

It uses a SinglyLinkedNode element as the node:

public class SinglyLinkedNode {
    private SinglyLinkedNode next = null;
    int data;
    public SinglyLinkedNode(int data){
        this.data=data;
    }
    public int getData(){
        return this.data;
    }
    public void setNext(SinglyLinkedNode next){
        this.next = next;
    }
    public SinglyLinkedNode getNext(){
        return this.next;
    }
}

The element definitions and the delete function are as so:

import data_structures.LinkedLists.SinglyLinkedNode;

public class SinglyLinkedList{
    private SinglyLinkedNode head = null;
    private SinglyLinkedNode tail = null;
    private int length = 0;
    public SinglyLinkedList(int data){
        this.head = new SinglyLinkedNode(data);
        this.tail = this.head;
        this.length = 1;
    }

    public void delete(int data){
        SinglyLinkedNode n = this.head;
        if (n==null){
            return;
        }
        if (n.getData()==data){//If the head is the data we want
            if (n.getNext()==null){//If it's the only node, null the head and tail
                this.tail = null;
                this.head = null;
                this.length = 0;
            }
            else {//Or move the head to the next node
                this.head = n.getNext();
                this.length--;
            }
            return;
        }
        while (n.getNext()!=null && n.getNext().getData()!=data){//Get the data node or the last node at n.next
            n=n.getNext();
        }
        if (n.getNext() == null){//If n is the last element in the list
            if (n.getData()!=data){//If data wasn't in array then we're done
                return;
            }
            //If we're deleting the only remaining element
            if (this.length <=1){
                this.tail = null;
                this.head = null;
                this.length=0;
            }
            else {//If we're just moving the tail
                n.setNext(null);
                this.tail = n;
                this.length--;
            }
        }
        else {//If n is not the last element
            if (n.getNext().getNext()==null){//If n.next is the last element
                n.setNext(null);
                this.tail = n;
                this.length--;
            }
            else {//If n.next is not the last element
                n.setNext(n.getNext().getNext());
                this.length--;
            }
        }
    }
}

As you can see, this is quite a lot of lines of code - 46 to be exact. In particular, the section below the while loop seems redundant - but I can't figure out how to replace any of it.

That's a very lengthly piece of code. To delete a Node, it's not about doing something confusing; it's about changing links around. Imaging this list:

1, 2, 3, 4, 5

In this case, 1 links to 2, which links to 3, and so on. So what if you want to remove 3, for example? Well, change the links so that 2 links to 4!

How? Well...

Rewrite the method so that it returns a boolean depending on if the delete was a success:

public boolean delete(int data) {

Handle special case head and empty list:

    SinglyLinkedNode nodeBeforeDelete = this.head;
    if (nodeBeforeDelete == null) { // List in empty
        return false;
    } else if (nodeBeforeDelete.getData() == data) {
        this.head = this.head.getNext();
        return true;
    }

Loop until you find the data:

    while (true) {
        SinglyLinkedNode next = nodeBeforeDelete.getNext()
        if (next == null) { // No data found in list
            return false;
        } else if (next.getData() == data) {
            break;
        }
        nodeBeforeDelete = next;
    }

Relink the nodes:

    SinglyLinkedNode next = nodeBeforeDelete.getNext();
    nodeBeforeDelete.setNext(next.getNext());
    next.setNext(null);

End the method:

    return true;
}

Result:

    public boolean delete(int data) {
        SinglyLinkedNode nodeBeforeDelete = this.head;
        if (nodeBeforeDelete == null) { // List in empty
            return false;
        } else if (nodeBeforeDelete.getData() == data) {
            this.head = this.head.getNext();
            return true;
        }
        while (true) {
            SinglyLinkedNode next = nodeBeforeDelete.getNext()
            if (next == null) { // No data found in list
                return false;
            } else if (next.getData() == data) {
                break;
            }
            nodeBeforeDelete = next;
        }
        SinglyLinkedNode next = nodeBeforeDelete.getNext();
        nodeBeforeDelete.setNext(next.getNext());
        next.setNext(null);
        return true;
    }


How can I make this primality test algorithm faster? Is it possible to make this algorithm faster ?  Algorithm represents primality test for Wagstaff numbers .

import java.math.BigDecimal;
import java.math.BigInteger;
import java.math.MathContext;
import java.math.RoundingMode;

public class WPT
{
public static void main(String[] args)
{
   int n;
n = Integer.parseInt(args[0]);

BigInteger m;
m = BigInteger.valueOf(n);

BigDecimal a = new BigDecimal("1.5");
BigDecimal b = new BigDecimal("5.5");
BigDecimal c = new BigDecimal("13.5");
BigDecimal d = new BigDecimal("16.5");

BigDecimal s;
BigDecimal r;

if (m.mod(BigInteger.valueOf(4)).equals(BigInteger.ONE))
  {
     s = a;
     r = a;

  } else {
      if (m.mod(BigInteger.valueOf(6)).equals(BigInteger.ONE))
      {
           s = b;
           r = b;

      } else {
          if (m.mod(BigInteger.valueOf(12)).equals(BigInteger.valueOf(11)) &&
              (m.mod(BigInteger.valueOf(10)).equals(BigInteger.valueOf(1))) ||
              m.mod(BigInteger.valueOf(10)).equals(BigInteger.valueOf(9)))
          {
               s = c;
               r = c;

          } else {
              s = d;
              r = d;

          }
      }
  }

          BigDecimal W;
          W = BigDecimal.valueOf(2).pow(n).add(BigDecimal.ONE).divide(BigDecimal.valueOf(3));

          int k = (n-1)/2;

           for (int i = 1; i <= k; i ++)
    {
   s = s.pow(4).multiply(BigDecimal.valueOf(8)).subtract(s.pow(2).multiply(BigDecimal.valueOf(8))).add(BigDecimal.ONE).remainder(W).setScale(1, BigDecimal.ROUND_UP);

     }

     if (s.equals(r))

  {
      System.out.println("prime");

  } else {

     System.out.println("composite");

     }
}
}

very fast corresponding Mathematica code :

p = 269987;
W = (2^(p) + 1)/3;
If[Mod[p, 4] == 1, a = 3/2, If[Mod[p, 6] == 1, a = 11/2,
If[Mod[p, 10] == 3 || Mod[p, 10] == 7, a = 33/2, a = 27/2]]];
For[i = 1; s = a, i <= (p - 1)/2, i++,
s = Mod[ChebyshevT[4, s], W]];
If[s == a, Print["prime"], Print["composite"]];

Avoid dealing with BigDecimals at all costs - they are slow. I changed the formula to work with "twice the number", so my s is actually 2*s. Further keep the numbers in the loop small by "modding" the intermediate results, too. Finally I tried to simplify the syntax and the initial conditions for s a little bit.

import java.math.BigInteger;

public class WPT {

    private final static BigInteger _1 = BigInteger.ONE;
    private final static BigInteger _2 = _(2);
    private final static BigInteger _4 = _(4);

    public static BigInteger _(long n) {
        return BigInteger.valueOf(n);
    }

    public static void main(String[] args) {
        int n = Integer.parseInt(args[0]);

        BigInteger m = _(n);

        BigInteger s =
                (m.mod(_(4)).equals(_1)) ? _(3) :
                        (m.mod(_(6)).equals(_1)) ? _(11) :
                                (m.mod(_(12)).equals(_(11)) &&
                                        (m.mod(_(10)).equals(_1)) ||
                                        m.mod(_(10)).equals(_(9))) ? _(27) : _(33);
        BigInteger r = s;

        BigInteger W = _2.pow(n).add(_1).divide(_(3));

        int k = (n - 1) / 2;

        for (int i = 0; i < k; i++) {
            BigInteger s2 = s.modPow(_2, W);
            BigInteger s4 = s2.modPow(_2, W);
            s = s4.subtract(s2.multiply(_4)).add(_2).mod(W);
        }

        System.out.println(s.equals(r) ? "prime" : "composite");
    }
}


Toggle button that will hide/unhide columns within a range I have the below toggle button sub that works;

Private Sub Toggle_Unused_Click()
    Dim xAddress As String
    xAddress = "DR:FI"

Call KeyOpen 'unprotects the sheet
If Toggle_Unused.Value Then
    Application.ActiveSheet.Columns(xAddress).Hidden = True
    'This returns a value to my notes tab.
    ActiveWorkbook.Sheets("Notes").Cells(5, 11).Value = "Hidden"
Else
    Application.ActiveSheet.Columns(xAddress).Hidden = False
    'This returns a value to my notes tab.
    ActiveWorkbook.Sheets("Notes").Cells(5, 11).Value = "Visible"
End If
'This will record how many times the button has been used.
     ActiveWorkbook.Sheets("Notes").Cells(9, 8).Value = _
     ActiveWorkbook.Sheets("Notes").Cells(9, 8).Value + 1
Call KeyLock 'protects the sheet
End Sub

Then I updated the code to look like this-

Private Sub Toggle_Unused_Click()
    Application.ScreenUpdating = False
    'This will unprotect the sheet
    Call ActiveWorkbook.Sheets("Notes").All_Sheets_UnProtect("Notes")

    If Toggle_Unused.Value Then
        Application.ActiveSheet.Range("NAMED_RANGE").Columns.Hidden = True
        'This will return a value to my notes tab.
        ActiveWorkbook.Sheets("Notes").Cells(5, 11).Value = "Hidden"
    Else
        Application.ActiveSheet.Range("NAMED_RANGE").Columns.Hidden = False
        'This will return a value to my notes tab.
        ActiveWorkbook.Sheets("Notes").Cells(5, 11).Value = "Visible"
    End If
    'This will track how many times the button has been used.
    ActiveWorkbook.Sheets("Notes").Cells(9, 8).Value = _
    ActiveWorkbook.Sheets("Notes").Cells(9, 8).Value + 1
    'This will protect the sheet
    Call ActiveWorkbook.Sheets("Notes").All_Sheets_Protect ("Notes")
    Application.ScreenUpdating = True
    'This will close the user form.
    Control_Buttons.Hide

End Sub

I have updated to use a named range because my sheet keeps growing with new columns and I am trying to limit how many declared columns I have in my code. I have four of these in a user form that each toggle a different section of sheet(Master).

You use ActiveWorkbook.Sheets("Notes") 5 times, throw that into a variable or use CodeName property - View Properties window (F4) and the (Name) field can be used as the worksheet name. This way you can avoid Sheets("mySheet") and instead just use mySheet.

You don't need to Call subs, it's obsolete. Instead just use Sub argument, argument

As far as troubleshooting the second, that's not really on topic here because it's not working. But what is -

Call ActiveWorkbook.Sheets("Notes").All_Sheets_UnProtect("Notes")

You were using

Application.ActiveSheet.Columns(xAddress).Hidden

and then changed to

Range("NAMED_RANGE").Columns.Hidden

It works for me. If your sheet is changing, it might fail because you don't specify the sheet.