Implement Immutable Collection in C# I would like to define some constant sequences (of bytes, specifically) in my C# library/API. Both length and content should not vary, preferably by any means (barring reflection). To this end, a byte[] array would not be sufficient, and a ReadOnlyCollection<T> can still be mutated. I have not done a full comparison yet, but this effort may be re-inventing ImmutableList<T>, in which case I would still appreciate feedback on the successes and weaknesses of this implementation. To be precise, this class is meant to be inherited by multiple derived classes, which actually define the constants.

Design & Style

I have chosen to build this class on the most primitive interface (IEnumerable<T>) that I could; partially to preserve maximal compatibility, and partially to avoid having to hide or throw NotSupportedException for certain ICollection<T> methods and such. Does this have any drawbacks?
The actual collection that contains the items is a private List<byte>.
I have implemented a couple convenience fields, AsString, Count, and Length (Length feels more appropriate for the type, but the existence of the Count<> LINQ extension made me feel both should be included for consistency). I considered get-only properties, however, this Q&A on SO points out that readonly is a more explicit modifier. Are there other pros/cons that I am missing in this specific case?
Given that I only intend to derive this class, I could mark it abstract, but could I also build it as an interface? (This would affect the readonly/get-only decision.) However, stylistically, it feels like derived classes should have an "is-a" relationship with Bytes rather than a "can-do".
I have omitted all namespace qualifiers here, as seems to be the standard in C#. However, I come mostly from Python, and global imports and polluting the global namespace are almost unbearable to me. I also don't want to see System.Collections.Generic.[...] everywhere; is it an acceptable compromise to use, for example, using CollectionsGen = System.Collections.Generic, or would this horrify C# programmers reading my code?

Code

/// <summary>
/// Implementation of an immutable collection of bytes, with some extras.
/// </summary>
public class Bytes : IEnumerable<byte>
{
    private List<byte> bytesList;
    public readonly string AsString;
    public readonly int    Count;
    public readonly int    Length; // just a synonym for Count

    // accepts values from -Length to Length-1 (similar to Python indexing).
    public byte this[int i] { get { return this.bytesList[ i>=0 ? i : i+this.Length ]; } }

    public Bytes(IEnumerable<byte> bytes)
    {
        this.bytesList = new List<byte>(bytes);
        this.AsString  = string.Join(" ", this.Select(b => $"0x{b:X2}"));
        this.Count     = this.bytesList.Count;
        this.Length    = this.Count;
    }

    public IEnumerator<byte> GetEnumerator() { return this.bytesList.GetEnumerator(); }

    IEnumerator IEnumerable.GetEnumerator() { return this.GetEnumerator(); }

I think could generalize this to any type by defining public class MyImmutableCollection<T> : IEnumerable<T> with roughly the same body, but I have no need for that now.

Usage

var byteArray = new byte[] { 0x01, 0x02, 0x03 };
var bytesInst = new Bytes(byteArray);
byteArray[1] = 0xFF;
// observe that byteArray has changed, where bytesInst has not.
bytesInst[1] = 0xFF; // correctly generates a read-only compiler error

Abstractness

  To be precise, this class is meant to be inherited by multiple derived classes, which actually define the constants.

Do you mean there will be many classes each of which contain a constant set of bytes, but are otherwise the same? If so, then there is no need to create a new class for each one. You could instead just provide static readonly instances for each constant, e.g.

public static class MagicNumbers
{
    public static readonly Bytes Example1 = new Bytes(new byte[] { 0xFF, 0xFF });
    public static readonly Bytes Example2 = new Bytes(new byte[] { 0xFE, 0xFF });
}

Readonlyness

This certainly provides a read-only interface, which is good. The only way to violate the read-onlyness would be with reflection (which it is essentially always fine to ignore).

You are right that the backing collection of a ReadOnlyCollection<T> can change; this is all part of the 'interface is gospel' approach of languages like C# (which personally I subscribe to whole-heartedly). Note that if you pass a ReadOnlyCollection<T> to someone else, however, that they can't modify it. This means it is up to whoever creates the backing collections for the ReadOnlyCollection<T> as to what can or cannot be done with it. ReadOnlyCollection<T>, however, is not a type I would use for anything, pretty much, because it unhelpfully implements IList<T> (among other things). It's also annoyingly named (in my opinion), because IReadOnlyCollection<T> doesn't expose indexed retrieval (that's IReadOnlyList<T> job).

It's good that your constructor takes an IEnumerable<T>. You might consider implementing IReadOnlyList<T> (which 'includes' IEnumerable<T>) if you think it appropriate; this would expose an efficient implementation of Count and T this[int] (more discussion about this below).

Inside your class you could also use a read-only collection instead of List<byte>, to enforce the contract internally, e.g. IReadOnlyList<T>.

Properties

Your Count and Length properties are, as you comment, redundant. Don't do this: it will only create confusion. Length is (in my experiance) pretty much exclusive to arrays: go with Count instead (it's the property exposed by IReadOnlyList<T>), and instead of storing the value, I'd suggest retrieving it from the actual list, which means the construtor has less to do (which means there is less to go wrong).

 public int Count => bytesList.Count;

It's good that your external properties were readonly (this is preserved with the 'expression-bodied property' syntax above). You might also want to make bytesList readonly (the class is fine as it is, because it's public API is sound, but this again will enforce the contract inside the class, making it easier to maintain and understand).

Concerning readonly vs getter-only, unless I have good reason (which basically never happens), I go with getter-only. While what Rob says in the linked thread is true, I personlly don't care what a class does so long as the API makes sense, and a public readonly field has two important disadvantages over getter-only properties: it can't implement an interface property (e.g. int IReadOnlyList.Count), and you can't quietly change it to be a property in future. For example, changing your public int read-only Count to public int Count => byteList.Count is a binary breaking change: any code that depends on the old field needs to be recompiled to work with the new property. If, however, you had made it public int Count { get; }, then you could make this change to the implementation without breaking any consuming code. One case where you might want this would be AsString: if you found that this was hardly ever being used, you might consider deferring the creation until it is needed: you can't do this with a readonly field, but you can easily replace a getter-only property with one which implements caching.

In practical terms, as Henk Holterman points out in the linked thread, readonly fields are a nightmare when it comes to serialisation. A getter-only field, however, can become a { get; private set; } field without issue if your serialisation scheme demands it. This is by no means ideal, but it doesn't change the external API, so the unpleasantness is contained within the class.

Indexer this[int]

You might consider the expression-bodied member syntax again:

public byte this[int i] => this.bytesList[ i >= 0 ? i : i + this.Count ];

Though it's just syntatic sugar we could live without, it is arguably a bit easier to read, and makes it immediately apprent that there is no set block.

If you did decide to implement IReadOnlyList, you would need 2 versions of this methods; one with the negative-index support, and one without.

byte IReadOnlyList<T>.this[int i] => this.bytesList[i];
public byte this[int i] => this.bytesList[ i >= 0 ? i : i + this.Count ];

This means that code which misuses the IReadOnlyList<T> interface will still get an out-of-bounds error, but anyone who has an instance of your class by name of the class can use the negative-indexing.

Namespace qualifiers

It's been a while since I used Python for anything substancial, but my understanding is that *-imports pull methods as well as classes, so the issue is lesser in C# (only pulls types and other namespaces). As you say, it's pretty standard for C#ers to use *-imports, and because it is expected, APIs are designed in a manner which assumes it: e.g. there are many more namespaces in C# than C++. Most importantly, however, is that C# fails at compile-time if there are observed name-collisions (e.g. even if Horse could be interpreted as two things, the code is fine so long as you don't try to use Horse unqualified). This means two things have to change for library authors to break your code - the introduction of a 'duplicate' method or type in a dependency in Python will quietly create problems - and you can't break your own (because it won't compile). The only leak in all this, is that classes in their own namespace are preferred over classes in an imported namespace (but only the author of that namespace can change the behaviour of their code, not someone else).

using CollectionsGen = System.Collections.Generic would mostly horrifying me because Gen is truncated. Anothing think you'll find in C# is that we prefer long easy-to-remember-and-understand names over short quicker-to-type (if you can remember them) names. The time-to-type is minimised by auto-complete in the IDE/editor.

Documentation

It's good that your class has an inline-summary, but inline-documentation (///) on all public non-inherited members significantly improved usabily. The Count == Length thing, for example, would be confusing no end, which is why you have a comment in your code to remind you that it is correct: this same comment should be available to the people using your code. The negative-indexing support is another thing that should be documented externally. The constructor should also state that it takes a copy of the IEnumerable; while this is implied (by virtue of taking an IEnumerable<T> rather than, say, an IReadOnlyList<T>), as a consumer of your code I would appreciate knowing that this is the intended long-term behaviour and not just an implementation detail.

Remember: code is consumed far more often than it is read or written (which people all too often seem to forget).

Misc

I wouldn't bother with the lining stuff up; it's just effort to maintain.
I'd also avoid things like "x>=2", and some IDEs will 'correct' it to x >= 2, so you'll just end up fighting them. If expressions need grouping, then use parenthesis, which are unambiguous.
If you are worried about space, consider using a byte[] rather than a list (you can use LINQ's IEnumerable<T>.ToArray() (guarantees copy) to keep the code short. (Note that byte[] implements IReadOnlyList<T>).


Getting parent relationship I am trying to find the best way to structure this. I want to avoid if-else statements and reduce the number of code lines.

def get_parent(relationships,GU)
  parent = []
  if relationships.class == Array
    relationships.each do |y|
      if y[:relationship_roles][:relationship_role][:to_role][:name] == type
        parent << y[:party][:party_id][:id]
      end
    end
  else
    if relationships[:relationship_roles][:relationship_role][:to_role][:name] == type
      parent << relationships[:party][:party_id][:id]
    end
  end
  return parent
end

Input:

:relationship=>{:name=>nil, :party=>{:party_id=>{:id=>"12344",
:id_type=>"abc"}, :name=>"XYZ", :@active=>"true", :@type=>"Organization"},
:relationship_roles=>{:relationship_role=>{:to_role=>{:name=>"GU"}}}}

The input could be only one relationship or multiple ones, which is why I have the if/else loop.

Some notes:

It would be debatable whether it's good practice to have arguments which may contain either single elements or collections.
If you convert whatever you get to an array (using Array), there is no need to repeat code.
Don't use each + << (imperative), use map + compact (functional).

I'd write:

def get_parent(relationship_item_or_collection)
  Array(relationship_item_or_collection).map do |relationship|
    if relationship[:relationship_roles][:relationship_role][:to_role][:name] == type
      relationship[:party][:party_id][:id]
    end
  end.compact
end


Adding values to rows in CSV This code below is for one of my scripts that I am trying to automate. I am dealing with the rows and all the data that is in those rows. What I am trying to do is make this look neater and maybe adding some different variables to make it look decent.

Foreach ($row in $csv) {
If ($row.Type0 -eq 'Domain') {
  $row."Unique Account Name" = "$($row.Domain0) - $($row.Account0)"
If ($row."Unique Account Name" -in @('ACCOUNTS - DODSCAN'.'ACCOUNTS - Domain Admins','ACCOUNTS - LADM_WS_Admins','ACCOUNTS - Tech Enterprise'))
        {$row."Excluded" = "True"}
    Else {$row."Excluded" = "False"}
 }
Else {
    $row."Unique Account Name" = "$($row.Netbios_name0) - $($row.Account0)"
    If ($row."Account0" -in @('esrxadm1n_esi','#Update','medco_tech','medco_admin'))
         {$row."Excluded" = "True"}
  Else {$row."Excluded" = "False"}
}

Indent the blocks properly.
Define the immutable exclusion lists before the loop.
To enhance readability, maintainability, and to avoid missing typos such as the dot after DODSCAN, which should be a comma, write just one array element per line without commas.

$excludedAccounts = @{
    unique = @(
        'ACCOUNTS - DODSCAN'
        'ACCOUNTS - Domain Admins'
        'ACCOUNTS - LADM_WS_Admins'
        'ACCOUNTS - Tech Enterprise'
    )
    account0 = @(
        'esrxadm1n_esi'
        '#Update'
        'medco_tech'
        'medco_admin'
    )
}

foreach ($row in $csv) {
    if ($row.Type0 -eq 'Domain') {
        $newName = "$($row.Domain0) - $($row.Account0)"
        $isRowExcluded = $newName -in $excludedAccounts.unique
    } else {
        $newName = "$($row.Netbios_name0) - $($row.Account0)"
        $isRowExcluded = $row.'Account0' -in $excludedAccounts.account0
    }
    $row.'Unique Account Name' = $newName
    $row.'Excluded' = [string]$isRowExcluded
}


An attempt at making a neat ls This is a simple script to make ls output first folders, then files, then other stuff (symlink). I think this is really neat and would like to share the script in exchange for comments.

It should be noted that I'm working with GNU findutils and coreutils.

Some basic criteria that I'm aiming for include:

lssort must accept the same arguments as ls
lssort should depend on bash, find, ls and xargs
the output must not be prefixed with "./"

Problems:

arguments can not be augmented (-CFXtrs does not work)

Script

#! /bin/bash
#source $HOME/.scripts/string_manipulation.mergeme
function trim {
    local var="$@"
    #$ var="  hello space    "
    #$ echo ">"
    #$ >
    var="${var#"${var%%[![:space:]]*}"}" # rm leading whitespace characters
    var="${var%"${var##*[![:space:]]}"}" # rm trailing whitespace characters
    echo -n "$var"
}

function arrayContains {
    # remove the shortest needle from array and compare lengths
    # needs more testing...
    declare -a array
    declare -a arrayLess
    declare needle
    array=( "${!1}" ); shift        # expand the passed array name
    needle="$@"                     # try to match the rest as a string
    arrayLess=( `trim "${array[@]#${needle}}"` )    # remove value $needle
    if [ ${#array[@]} -eq 0 ];then                  return 1 ;fi
    if [ ${#array[@]} -eq ${#arrayLess[@]} ];then   return 1 ;fi
    if [ ${#array[@]} -lt ${#arrayLess[@]} ];then   return 1 ;fi
    return 0
}

# d dir f file p named pipe (FIFO) l sylink s socket D door (Solaris)
# c character (unbuffered) special b block (buffered) special

function finddirs {
    find "$@" \
        -maxdepth 1 -depth -type d \
        -regextype gnu-awk -regex "$REGEX" \
        -printf '%f\0'
}
function findfiles {
    find "$@" \
        -maxdepth 1 -depth -type f \
        -regextype gnu-awk -regex "$REGEX" \
        -printf "%f\0"
}
function findspecials {
    find "$@" \
        -maxdepth 1 -depth \( -type l   -o -type p   -o -type s \) \
        -regextype gnu-awk -regex "$REGEX" \
        -printf "%f\0"
}
##
## vars and such
##
validswitches=(-X -l -r -t -C -F -1 -a -A -d -s)
PATHS=()
FPATHS=""
LSSWITCHES=""
ORIGIN_DIR="`pwd`"
LSHIDDEN="^\./.+"       # gnu-awk regex for find, single dot does not match
LSNORMAL="^\./[^\.]+.+" # exclude files starting with .
LS=ls\ --color=auto

ARG=$1
ARGC=0
while [ "$ARG" != "" ];
do
    if [ `arrayContains validswitches[@] $ARG ;echo $?` -eq 0 ];then
        LSSWITCHES="${LSSWITCHES} $ARG"
        unset ARG
    fi

    if [ -d "$ARG" ];then
        #PATHS="${PATHS} $ARG"
        PATHS[${ARGC}]=$ARG
        let ARGC=ARGC+1
    else
        FPATHS="${FPATHS} $ARG"
    fi
    shift
    ARG=$1
done

# FIX only -A excludes . and ..
if [[ "$LSSWITCHES" =~ '-a' || "$LSSWITCHES" =~ '-A' ]]; then
    REGEX=$LSHIDDEN
else
    REGEX=$LSNORMAL
fi

#PATHS=`trim $PATHS`
FPATHS=`trim $FPATHS`

if [ ${#FPATHS} -eq 0 ]; then
    if [ ${#PATHS[@]} -eq 0 ]; then
        PATHS=(".")
    fi
fi
for arg in "${PATHS[@]}";
do
    if [[ ${#arg} -eq 1 && "$arg" == "/" ]];then
        p=/
    else
        #p=${arg%%/}
        p=$arg
    fi

    cd "$p"
    if [[ "$p" != "." && "$p" != "./" ]];then echo "$p": ;fi
    finddirs | xargs -r0 $LS $LSSWITCHES -d
    findfiles | xargs -r0 $LS $LSSWITCHES
    findspecials | xargs -r0 $LS $LSSWITCHES -d
    [ ${#PATHS[@]} -gt 1 ] && echo
    cd $ORIGIN_DIR
done

echo $FPATHS |xargs -r $LS $LSSWITCHES
#echo paths.:\ size: ${#PATHS[@]}
#echo fpaths:\

lssort

If you use getopt or getopts to parse the options, you can use multiple options together, and it would simplify your code quite a bit. Example.
When you create arrays explicitly (name=(values)) you don't need to declare it first.
arrayContains should take one needle and then the contents of the array in question. This is how it works in other languages, and is easier to program and more explicit than using an array reference and concatenated needles. Also, when expanding needle the values will be concatenated with a space between them, which is rather arbitrary.
If needle is empty, arrayContains should return 0 regardless of the array contents.
You can merge the if statements in arrayContains by separating them with -o:

if [ $a -eq 0 -o $b -eq 0 -o $c -eq $a ]

ls works with globs rather than regular expressions by default. The find* functions will not return the same filenames as ls with the same input. For example, run touch example && mkdir eclectic and compare

ls [e]*

with

find . -maxdepth 1 -depth -type f -regextype gnu-awk -regex "[e]*"

You should be able to get the current working directory with $PWD rather than pwd to save a fork. This makes the code a tiny bit faster, without losing clarity, and fixes a very common but little known bug: The `command` construct (identical to the preferred $(command) when using Bash) removes newlines from the end of the command output. That means if you mkdir $'foo\n' && cd $'foo\n' , ORIGIN_DIR="`pwd`" will give the wrong result, but ORIGIN_DIR="$PWD" will give the right one.
Use More Quotes, for example do ARG="$1" to enable working with whitespace in filenames. This is a very tricky subject.
If you want to get this working with all filenames, you'll need to use an array to store the find results before sending them to ls


T-SQL simplify repeated query unions for each day of the week I've created the T-SQL code below to get the next nearest datetime value during the next 7 days.

The ScheduleDetail table schema is:

CREATE TABLE [dbo].[ScheduleDetail]
(
[uid] [int] IDENTITY(1,1) NOT NULL,
[scheduleRef] [int] NOT NULL,
[startTime] [datetime] NOT NULL,
[stopTime] [datetime] NOT NULL,
[validSunday] [bit] NOT NULL,
[validMonday] [bit] NOT NULL,
[validTuesday] [bit] NOT NULL,
[validWednesday] [bit] NOT NULL,
[validThursday] [bit] NOT NULL,
[validFriday] [bit] NOT NULL,
[validSaturday] [bit] NOT NULL,
[validHoliday] [bit] NOT NULL
)

The MIN(CAST(CAST(...) AS nextScheduleDateTime is to get the date from that day of the week (after today) and add it to the schedule's startTime (as the startTime and stopTime fields are actually TIME types stored as DATETIME's. Don't ask me why...)

The only difference between each is the value @Sunday, @Sunday + 1, etc. and the column names: validSunday, validMonday, etc.

DECLARE @waypointId INT = 100;
DECLARE @Sunday INT = 1;

SELECT MIN(nextScheduleDateTime)
FROM
    SELECT MIN(CAST(CAST(DATEADD(dd, (6 - DATEPART(dw, GETDATE()) + @Sunday) % 7 + 1, GETDATE()) AS DATE) AS DATETIME) + CAST(ScheduleDetail.startTime AS TIME)) AS nextScheduleDateTime
    FROM WaypointEventLog INNER JOIN
         Schedule ON Schedule.scheduleRef = WaypointEventLog.ScheduleID INNER JOIN
         ScheduleDetail ON Schedule.scheduleRef = ScheduleDetail.scheduleRef
    WHERE WaypointID = @waypointId
    AND Completed = 0
    AND Schedule.scheduleType = 6
    AND (validSunday = 1 AND DATEPART(dw, GETDATE()) <> @Sunday)

    UNION

    SELECT MIN(CAST(CAST(DATEADD(dd, (6 - DATEPART(dw, GETDATE()) + @Sunday + 1) % 7 + 1, GETDATE()) AS DATE) AS DATETIME) + CAST(ScheduleDetail.startTime AS TIME)) AS nextScheduleDateTime
    FROM WaypointEventLog INNER JOIN
         Schedule ON Schedule.scheduleRef = WaypointEventLog.ScheduleID INNER JOIN
         ScheduleDetail ON Schedule.scheduleRef = ScheduleDetail.scheduleRef
    WHERE WaypointID = @waypointId
    AND Completed = 0
    AND Schedule.scheduleType = 6
    AND (validMonday = 1 AND DATEPART(dw, GETDATE()) <> @Sunday + 1)

    UNION

etc for each day of the week.....

Is there any way to simplify this, maybe convert these to a UDF somehow? Or Similar.

I could probably do something with EXEC and LEFT() the column name but that feels wrong.

Syntax Issue

The derived table in the FROM clause needs to be surrounded by parentheses:

DECLARE @waypointId INT = 100;
DECLARE @Sunday INT = 1;
SELECT MIN(nextScheduleDateTime)
FROM
    SELECT MIN(CAST(CAST(DATEADD(dd, (6 - DATEPART(dw, GETDATE()) + @Sunday) % 7 + 1, GETDATE()) AS DATE) AS DATETIME) + CAST(ScheduleDetail.startTime AS TIME)) AS nextScheduleDateTime

Should be updated like this:

SELECT MIN(nextScheduleDateTime)
FROM
    (SELECT MIN(CAST(CAST(DATEADD(dd, (6 - DATEPART(dw, GETDATE()) + @Sunday) % 7 + 1, GETDATE()) AS DATE) AS DATETIME) + CAST(ScheduleDetail.startTime AS TIME)) AS nextScheduleDateTime

And then after the sub-queries:

    ) AS Subqueries

Simplifying sub-queries

I believe (but haven't tested, since I don't have your data and expected output) that the WHERE conditions can be updated like below, which would allow combining the sub-queries into one, eliminating the usage of UNION:

DECLARE @waypointId INT = 100;
DECLARE @Sunday INT = 1;

SELECT MIN(CAST(CAST(DATEADD(dd, (6 - DATEPART(dw, GETDATE()) + @Sunday) % 7 + 1, GETDATE()) AS DATE) AS DATETIME) + CAST(ScheduleDetail.startTime AS TIME)) AS nextScheduleDateTime
FROM WaypointEventLog INNER JOIN
     Schedule ON Schedule.scheduleRef = WaypointEventLog.ScheduleID INNER JOIN
     ScheduleDetail ON Schedule.scheduleRef = ScheduleDetail.scheduleRef
WHERE WaypointID = @waypointId
AND Completed = 0
AND Schedule.scheduleType = 6
AND
(( validSunday = 1 AND DATEPART(dw, GETDATE()) <> @Sunday ) OR
        ( validMonday  = 1 AND  DATEPART(dw, GETDATE()) <> @Sunday + 1) OR
        ( validTuesday = 1 AND  DATEPART(dw, GETDATE()) <> @Sunday + 2 ) OR
        ( validWednesday = 1 AND  DATEPART(dw, GETDATE()) <> @Sunday + 3 ) OR
        ( validThursday = 1 AND  DATEPART(dw, GETDATE()) <> @Sunday + 4 ) OR
        ( validFriday = 1 AND  DATEPART(dw, GETDATE()) <> @Sunday + 5) OR
        ( validSaturday = 1 AND  DATEPART(dw, GETDATE()) <> @Sunday + 6 ))

While it only contains the one table scheduleDetail, you can see this illustrated in this SQL fiddle. Feel free to update that fiddle and provide more details about the other tables, plus the data in those tables.

Programmatic way of generating the query above

One approach you could take is to define a CURSOR, and use that to loop through each field, adding the sub-queries to be joined via UNION to a long string. Originally I was thinking a varchar(5000) would be necessary but that was before I discovered the UNIONs could be eliminated.

DECLARE @name VARCHAR(50) -- field name
DECLARE @rowNum varchar(10) -- row number
DECLARE @sql VARCHAR(5000) = 'SELECT MIN(CAST(CAST(DATEADD(dd, (6 - DATEPART(dw, GETDATE()) + @Sunday) % 7 + 1, GETDATE()) AS DATE) AS DATETIME) + CAST(ScheduleDetail.startTime AS TIME)) AS nextScheduleDateTime
FROM WaypointEventLog INNER JOIN
     Schedule ON Schedule.scheduleRef = WaypointEventLog.ScheduleID INNER JOIN
     ScheduleDetail ON Schedule.scheduleRef = ScheduleDetail.scheduleRef
WHERE WaypointID = @waypointId
AND Completed = 0
AND Schedule.scheduleType = 6
AND ('

DECLARE db_cursor CURSOR FOR  -- fetch columns to loop over
SELECT COLUMN_NAME, ROW_NUMBER() OVER(ORDER BY ORDINAL_POSITION ASC) - 1 AS RowNum
FROM INFORMATION_SCHEMA.columns
WHERE table_NAME = 'ScheduleDetail' and COLUMN_NAME like 'valid%' and COLUMN_NAME not like '%holiday'
ORDER BY ORDINAL_POSITION

OPEN db_cursor
FETCH NEXT FROM db_cursor INTO @name, @rowNum

WHILE @@FETCH_STATUS = 0
BEGIN
    --append WHERE condition
   SET @sql  = @sql + ' ('+ @name + ' = 1 AND DATEPART(dw, GETDATE()) <> @Sunday + ' + @rowNum + ' )'+char(10)+char(13)   -- + CR + LF

   FETCH NEXT FROM db_cursor INTO @name, @rowNum
   if @@FETCH_STATUS=0 -- all except last condition
    BEGIN
        SET @sql  = @sql + 'OR'+char(10)+char(13)   -- + CR + LF
    END
    ELSE
    BEGIN -- last condition
        SET @sql  = @sql + ')'+char(10)+char(13)   -- + CR + LF
    END
END

CLOSE db_cursor
DEALLOCATE db_cursor

print @sql

That could be used to generate a Table-valued function or View.


implementations of strncmp, strncat, strncpy Write versions of the library functions strncpy, strncat and strncmp, which operate on the most n characters of their argument strings. For example, strncpy(s, t, n) copies at most n characters of t to s. Full Description in Appendix B.

Here is my implementation of strncmp:

int custom_strncmp(const char *s, const char *t, size_t n) {
    while(n--) {
        if(*s != *t) {
            return *s - *t;
        }
        else {
            ++s;
            ++t;
        }
    }

    return 0;
}

The while loop will run until n reaches the value 0, or until the characters that s and t point to are not equal. At each iteration I check if the *s and *t are equal; if they are, I increment s and t. If they are not equal, I return their difference.
After the loop stops, the value 0 is returned.

Here is my implementation of strncat:

char *custom_strncat(char *s, const char *t, size_t n, size_t bfsize) {
    size_t slen = strlen(s);
    char *pend = s + slen;

    if(slen + n >= bfsize)
        return NULL;

    while(n--)
        *pend++ = *t++;

    return s;
}

This version of strncat is a secure one. The first if-statement checks if there is enough space in the s buffer to store the contents of t. The while loop will run until n reaches the value 0. At each iteration the value that t points to is copied in to *s.
After the loop stops, is returned a pointer to the result.

Here is my implementation of strncpy:

char *custom_strncpy(char *s, const char *ct, size_t n) {
    char *saver = s;

    while(n--)
        *saver++ = *ct++;
    *saver = '\0';

    return s;
}

The while loop will run until n reaches the value 0, at each iteration the current character that ct points to is copied in *saver. After the loop stops, the '\0' is copied in *saver and a pointer to the result is returned.

The exercise can be found at page 121 in K&R second edition.

I think you've misinterpreted the definition of strncat, which says,

  Appends the first num characters of source to destination, plus a terminating null-character.
  If the length of the C string in source is less than num, only the content up to the terminating null-character is copied.

n is the maximum number of characters in source you you want to copy, and bfsize doesn't exist in the standard version of strncat.

char destination[256];
strpy(destination, "foo");
strncat(destination, "bar", 2);
// expect destination should now contain "fooba" !

It's good of you to write a custom version with an extra parameter to make it safe, but that's not what the question asked for.

See also Manual Reference Pages  - Strn (3) (e.g. Strncat instead of strncat); or, the Microsoft versions of the 'safe' functions have _l as a suffix.

I'm not sure that returning null and doing no copy is the standard way to avoid buffer overflow.

In your custom_strncmp method, I'm not sure what correct behaviour should be if you call

strncmp("foo", "foo", 10);

Your function might return 0 or non-zero, or crash, depending on what's after the end of the string.

You could perhaps write it more elegantly as a for loop:

for ( ; n--; ++s, ++t) {
    if(*s != *t) {
        return *s - *t;
    }
}

In custom_strncat you don't handle the condition where n > strlen(t).

Your parameters could (should) be better named, e.g. source and destination. Parameter names are important (even more important than good names for local variables) because they may be the only documentation about how the function should be called.


Mutlilevel page list in CodeIgniter I'm newly working with CodeIgniter and PHP. I wrote a category structured 'pages' code with listing pages as multilevel list or indented select box.

page                                        page_lang
-------------------------------------       ----------------------------
id_page | id_menu | id_parent | level       id_page |  title     | etc..
-------------------------------------       ----------------------------
   1    |    1    |     0     |   0            1    | Name 1     | etc..
   2    |    1    |     1     |   1            2    | Name 1.1   | etc..
   3    |    1    |     2     |   2            3    | Name 1.1.1 | etc..
   4    |    1    |     2     |   1            4    | Name 1.2   | etc.

My controller:

function get_menu_content($id_menu, $id_parent = 0, $level = 0)
{
  echo '<select>';
  $dropdown = $this->my_model->get_menu($id_menu,'dropdown');
  echo $dropdown;
  echo '</select>';  //echo in controller for quick test purpose
}

My_model has three functions: the first is getting result array, the other two is rendering as list and options.

class My_model extends CI_Model
{

  public $model_result;

  function __construct()
  {
     parent::__construct();
  }

function get_menu($id_menu = '1',$type = 'list')
{
  $sql = "
  SELECT a.id_parent, b.id_page, a.level, b.title
  FROM page a
  INNER JOIN page_lang b ON a.id_page = b.id_page
  WHERE
    a.id_menu = $id_menu";
  $query = $this->db->query($sql);

  if($query->num_rows() > 0)
  {
    $res = $query->result_array();
    foreach($res as $row):
      $items[$row['id_parent']][] = $row;
    endforeach;
    if($type =='list') $this->_menu_list($items); //renders ul tags
    if($type =='dropdown') $this->_menu_dropdown($items); //renders option tag
    return $this->model_result;
  }
}

function _menu_list($items, $parent = null)
{
  $index = $parent == null ? '0' : $parent;
  if (isset($items[$index]))
  {
    $this->model_result .= '
model_result .= $parent == null ? ' id="category" ' : '';
    $this->model_result .= '>';
    foreach ($items[$index] as $child)
    {
      $this->model_result .= '
'.$child['title'];
      $this->_menu_list($items, $child['id_page']);
      $this->model_result .= '
';
    }
    $this->model_result .= '
';
  }
}

function _menu_dropdown($items, $parent = null, $level = '0')
{
  $index = $parent == null ? '0' : $parent;
  $space = str_repeat(' &nbsp &nbsp ', $level);
  if (isset($items[$index]))
  {
    foreach ($items[$index] as $child)
    {
      $this->model_result .= '<option value="'.$child['id_page'].'">'.$space.$child['title'].'</option>' . "\n";
      $this->_menu_dropdown($items, $child['id_page'],$level+1);
    }
  }
}

It works a bit slowly. I need a cleaner code I guess. I will appreciate any improvement on the code based on my database structure.

One general comment -> YOU ARE NOT ESCAPING $id_menu IN YOUR SQL QUERY!

There is a section on how to do this in the CodeIgniter Database Query Documentation.

I'm not familiar with Code Igniter but there is nothing obvious that jumps out at me as to why this would be slow.


Static site generator using JavaScript Promises I have created a small static site generator (for learning purposes) in Javascript ES6, using Promises, but I am not certain of how to use them well. The code below works fine, but I wonder if i can't write my promises a better way.

I call my promises in SSG.start();

const yaml = require('js-yaml');
const fs   = require('fs-extra');
const ejs  = require('ejs');
const md   = require('markdown-it')({
    html: true,
    linkify: true,
    typographer: true
});

let SSG = {
    outputFolder: '../output/',

    contentFolder: '../content/',

    assetsFolder: '../assets/',

    contentFiles: async function (dir, filelist) {
        return new Promise((resolve) => {
            files = fs.readdirSync(dir);
            filelist = filelist || [];
            files.forEach((file) => {
                if (fs.statSync(dir + '/' + file).isDirectory()) {
                    return resolve(contentFiles(dir + '/' + file, filelist));
                } else {
                    filelist.push(file);
                    return resolve(filelist);
                }
            });
        })
    },

    generateHTMLFile: async function (file) {
        return new Promise((resolve) => {
            let fstream = fs.readFileSync(file, 'utf8');
            let params = fstream.match(/\+Params\+([\s\S]*?)\+\+\+/g);
            let content = fstream.match(/(\+Content[0-9]*\+|\+Content\+)([\s\S]*?)*(\+\+\+)/g);
            let contents = [];

            params = params[0].replace('+Params+', '').replace('+++', '');
            content = content[0].split('+++');

            try {
                params = yaml.safeLoad(params, 'utf8');
                for (it of content) {
                    it = it.replace('+++', '').replace(/(\+Content[0-9]*\+|\+Content\+)/g, '');
                    contents.push(md.render(it));
                }

                const funs = require('./functions.js');

                ejs.renderFile('../templates/' + params.template + '.ejs', {funs: funs, params: params, content: contents}, {}, (err, html) => {
                    if (err) console.log(err);
                    resolve({html: html, url: params.url});
                });

            } catch (e) {
                console.log(e);
            }
        });
    },

    saveHTML: function (html, url) {
        if (!fs.existsSync(this.outputFolder)) {
            fs.mkdir(this.outputFolder, { recursive: true }, (err) => {
                if (err) throw err;
            });
        }
        fs.writeFile(this.outputFolder + url + '.html', html, (err) => {
            if (err)  return console.log(err);
        });
    },

    saveStaticAssets: function () {
        fs.copySync(this.assetsFolder, this.outputFolder);
    },

    start: async function () {
        try {
            let time = Date.now();
            console.log('Génération des fichiers HTML...');

            let files = await this.contentFiles(this.contentFolder);

            for (file of files) {
                await this.generateHTMLFile(this.contentFolder + file).then(({html, url}) => {
                    this.saveHTML(html, url);
                });
            }

            this.saveStaticAssets();

            console.log('Généré en ' + (Date.now() - time) + 'ms');
        } catch (e) {
            console.error(e.message);
        }
    }
};

(async () => {
    await SSG.start();
})();

What do you think about it ?

First thing that bother me is why let SSG? Why is it an object? If you're using ES6+ (as you do), I'd go with making this a class. It's more obvious and more readable. Note that while doing this, you'll have to alter the function signature, i.e. generateHtmlFile: async function (file) { ... } becomes async generateHtmlFile(file) { ... }.

Since you have folder names stored as a properties of your object (outputFolder, contentFolder and assetsFolder), they should be moved now to the constructor. There is also a good candidate for another new property on line 51: ../templates/ should be also extracted. This is because there are no properties in JS class ATM. Also, while we're on the topic of class properties, I'd suggest that you should pass an object with folder names as a constructor argument. That way you can easily change the folders you're using by just instantiating an object with another folder names. When we sum up the info about properties, here's what it should look like:

class SGG {
  constructor(folders = {}) {
    this.outputFolder = folders.outputFolder || '../output/'
    this.contentFolder = folders.contentFolder || '../content/'
    this.assetsFolder = folders.assetsFolder || '../assets/'
    this.templates = folders.templates || '../templates/'
  }
}

The second thing that I didn't notice when I looked at the code here is that you have a some implicit variables: files on line 19 and it on line 45. So just add const in front of those two and you'll be fine.

At line 49, there is a require. I'd move it to the top of the file because that way you know right on what are the dependencies of your file. And it's more cleaner code without some unexpected turns.

Now, to the methods. First one is contentFiles which is just a recursive call to find out all the files in a contentFolder directory. I see that you are probably confused with promises because you use async and new Promise in the same function. When making function an async, you should know that whatever you return from the function, it will be wrapped inside a promise and it also gives you the ability to use await inside it. But since you don't have await here, then there is no need for making this function an async one. You are already returning a promise. But, I think that you don't need a promise at all! You are using these fs-extra methods that are not returning a promise nor have a callback, so they are perfectly synchronous functions. Also, the .forEach got all of this tangled a bit, so if you remove it and use for..of, you can untangle yourself and get a cleaner code.

The next method, generateHTMLFile has similar issue with promises as the one above. From ejs' github, I noticed that if you don't pass the callback function, it will return a promise. So what should be done here is to remove new Promise... thing and keep async and rewrite the ejs.renderFiles not to use the callback function.

The method saveHTML is confusing a bit: you're using sync methods everywhere, but here you decided to go with mkdir instead of mkdirSync or writeFileSync.

Once you apply all of what I've said, you should get something like this:

const yaml = require('js-yaml');
const fs = require('fs-extra');
const ejs = require('ejs');
const md = require('markdown-it')({
    html: true,
    linkify: true,
    typographer: true,
});
const funs = require('./functions.js');

class SSG {
    constructor(folders = {}) {
        this.outputFolder = folders.output || '../output/'
        this.contentFolder = folders.contentFolder || '../content/'
        this.assetsFolder = folders.assetsFolder || '../assets/'
        this.templateFolder = folders.templateFolder || '../templates/'
    }

    contentFiles(dir, filelist) {
        const files = fs.readdirSync(dir)
        filelist = filelist || []
        for (const file of files) {
            if (fs.statSync(dir + '/' + file).isDirectory()) {
                contentFiles(dir + '/' + file, filelist)
            } else {
                filelist.push(file)
            }
        }
        return filelist
    }

    async generateHTMLFile(file) {
        const fstream = fs.readFileSync(file, 'utf8');
        let params = fstream.match(/\+Params\+([\s\S]*?)\+\+\+/g);
        let content = fstream.match(/(\+Content[0-9]*\+|\+Content\+)([\s\S]*?)*(\+\+\+)/g);
        let contents = [];

        params = params[0].replace('+Params+', '').replace('+++', '');
        content = content[0].split('+++');

        try {
            params = yaml.safeLoad(params, 'utf8');
            for (let it of content) {
                it = it.replace('+++', '').replace(/(\+Content[0-9]*\+|\+Content\+)/g, '');
                contents.push(md.render(it));
            }

             const html = await ejs.renderFile(this.templateFolder + params.template + '.ejs', {
                funs: funs,
                params: params,
                content: contents,
            }, {})

            return {
                html,
                url: params.url,
            }

        } catch (e) {
            console.log(e);
        }
    }

    async saveHTML(html, url) {
        if (!fs.existsSync(this.outputFolder)) {
            await fs.mkdirSync(this.outputFolder, {recursive: true});
        }
        await fs.writeFileSync(this.outputFolder + url + '.html', html);
    }

    saveStaticAssets() {
        fs.copySync(this.assetsFolder, this.outputFolder);
    }

    async start() {
        try {
            let time = Date.now();
            console.log('Génération des fichiers HTML...');

            const files = this.contentFiles(this.contentFolder);

            for (const file of files) {
                await this.generateHTMLFile(this.contentFolder + file).then(({html, url}) => {
                    this.saveHTML(html, url);
                });
            }

            this.saveStaticAssets();

            console.log('Généré en ' + (Date.now() - time) + 'ms');
        } catch (e) {
            console.error(e.message);
        }
    }
};

(async () => {
    const ssg = new SSG()
    await ssg.start();
})();

Last note that I'd like to make is that Node already has a pretty decent file system utilities. The only thing that I've noticed that is different from fs-extra is that sync functions do not return promises, but they rather act as a regular functions. Anyhow, I'd argue that you don't need fs-extra and use fs instead. But it's up to you.

I maybe missed something or maybe got into details and nitpicking more than needed, but this is a basic refactor that I would do to your code. I hope that I didn't break some things while doing this :)

Edit:
While looking at the code for the second time, I noticed that I've missed this line in start:

await this.generateHTMLFile(this.contentFolder + file).then(({html, url}) => {
    this.saveHTML(html, url);
});`

It's maybe just me, but I don't like this mixture of await and then.
I'd just suggest that if you're going with await, keep it that way.
My suggestion here would be to go with something like this:

const {html, url} = await this.generateHTMLFile(this.contentFolder + file)
await this.saveHTML(html, url)

I've also just noticed that you can optimize the solution for speed by awaiting on multiple promises:

await Promise.all(files.map(async (file) => {
    const {html, url} = await this.generateHTMLFile(this.contentFolder + file)
    await this.saveHTML(html, url)
}))

This way you start multiple promises in parallel (again, not a true parallelization since JS is single threaded) and await on all of them to finish before moving on. This way you'll get a bit faster execution. Note that if one promise throws and error, this line will also throw an error before waiting on the other promises to end.


Tokenize a mathematical statement string The function tokenize generates tokens from a mathematical string.

It can chain characters together to form longer tokens.

This can be used to build a parse tree, in order to evaluate the math expression.

def get_char_type(char):
    """
    Returns a string denoting the type of char.

    >>> get_char_type('x')
    'letter'
    """
    operators = {'*', '/', '+', '-', '=', \
                '%', '~', '/', '&', '|', '>', '<'}
    if char.isdigit():
        char_type = 'number'
    elif char.isalpha():
        char_type = 'letter'
    elif char in operators:
        char_type = 'operator'
    else:
        char_type = 'other'
    return char_type

def tokenize(string):
    """
    Generates tokens from a mathematical statement string.

    >>> list(tokenize('3 + (4 ∗ 5)'))
    ['3', '+', '(', '4', '∗', '5', ')']

    >>> list(tokenize('x+=10'))
    ['x', '+=', '10']

    >>> list(tokenize('(729 + 4 * variable) ** 22 // 3'))
    ['(', '729', '+', '4', '*', 'variable', ')', '**', '22', '//', '3']
    """
    token_type = get_char_type(string[0])
    token = ''
    for char in string:
        if char == ' ':
            continue  # Spaces are not included
        new_type = get_char_type(char)
        if new_type != token_type:  # A new type of token has been found
            yield token
            token_type = new_type
            token = ''
        token += char
    if len(token) > 0:
        yield token

Possible errors/suspicious statements
In def get_char_type(char), you have repeated the statement
if char.isdigit():
    char_type = 'number'

twice. This is probably unintentional.
Note: The OP has removed this error in an edit before this answer was posted.
Suggestions
Instead of explicitly checking for whitespace in your generator expression and using a continue to skip whitespace, you can filter the input string by a regex (regular expression) to remove all whitespace before you run the loop over the characters of the string. Kind of like this:
whitespace_pattern = re.compile(r"\s")
for char in whitespace_pattern.sub("",string):
    new_type = get_char_type(char)
    ...

instead of
for char in string:
    if char == ' ':
        continue  # Spaces are not included
    ...

This looks a bit better, although it might not be as performant as your original code. You will also need an import re somewhere at the top of your code.
Also, instead of that if-elif-else in get_token_type(), what about this?
def get_char_type(char):
"""
Returns a string denoting the type of char.

>>> get_char_type('x')
'letter'
"""
operators = {'*', '/', '+', '-', '=', '%', '~', '/', '&', '|', '>', '<'}
characteristics_of_token_types = {
    (lambda char: char.isdigit()): 'number',
    (lambda char: char.isalpha()): 'letter',
    (lambda char: char in operators): 'operator',
    (lambda char: len(char) == 1): 'other' #default case, if extending functionality, put any new case before this
}
for characteristic in characteristics_of_token_types.keys():
    if(characteristic(char)):
        return characteristics_of_token_types[characteristic]

Why is this better? If you want to add more types, it just becomes a matter of specifying a characteristic of a token type (via a lambda dictionary key), and a name for the type (as the lambda key's value). It makes a bit more sense to me logically, however, opinions vary.
Also, note that what I did above is somewhat along the canonical path of emulating a switch-case in Python, which is, in fact, a better fit for this problem than if-else. I was looking for something like guards in Haskell, as I've recently become a functional programming guy, but I've got to make do with Python, so this is what I could think up.
Finally, I did not bother making your token-parsing code more "functional" as I think the result would be more complicated than what you already have.
Style
You have docstrings. That's a definite plus. You also follow the proper naming conventions for methods and variables, and there is adequate commenting, so no issues there.
However, a minor peeve - do you really need that line continuation when declaring the operators list? It doesn't look like you do, the whole thing fits in a line in below 40 characters all right.


Multilanguage Support in Apache Cordova I currently have a languages.js set up as such:

var MaterialProperties = 0;
var YieldStrength = 1;
var OtherPhrases = 2;

//continue to list other phrases - there are 40+

var English = [
    "Material Properties",
    "Yield Strength",
    "Other Phrases"
];

var Chinese = [
    // Chinese string that means "Material Properties",
    // Chinese string that means "Yield Strength",
    // Other phrases in Chinese
];

var SelectedLanguage = English; //default

Then I have the user select via dropdown box the language they would like, calling setLanguage(selected_language), which sets SelectedLanguage to either "English" or "Chinese", depending on the value of the dropbox selection.

Everywhere in my HTML where it should read "Material Strength" (or corresponding string in Chinese), I reference this as SelectedLanguage[MaterialProperties], which is the same as saying English[0] or Chinese[0], which returns the correct string.

Is there a better, more efficient, cleaner way to do this?

Typically localization might be achieved through storing localization in configuration files (perhaps in JSON) which would be deployed along with the application. The appropriate config would be loaded into memory based on localization setting such that main app code code just references a string identifier.

This would be in contrast to your approach of loading all translations into javascript variables in memory, which could get memory intensive if you have a large number of strings.

So you might take a look at a plug-in like this which takes this general approach.


API response json to C# object So I got kind of a weird format returned from the api I'm calling it's the following:
[
    [
        1609065000000,
        "22739.71000000",
        "22795.73000000",
        "22700.39000000",
        "22779.08000000",
        "38.81782800",
        1609065899999,
        "883167.94526884",
        2261,
        "18.58965200",
        "423036.58181400",
        "0"
    ]
]

    public static class StringExtensions
    {
        public static IEnumerable<Candlestick> ToCandleStickList(this string jsonObject)
        {
            var candleStickList = new List<Candlestick>();
            var list = JsonConvert.DeserializeObject<List<object[]>>(jsonObject);
            foreach (var obj in list)
            {
                candleStickList.Add(new Candlestick
                {
                    OpenTime = (long)obj[0],
                    Open = double.Parse((string)obj[1]),
                    High = double.Parse((string)obj[2]),
                    Low = double.Parse((string)obj[3]),
                    Close = double.Parse((string)obj[4]),
                    Volume = double.Parse((string)obj[5]),
                    CloseTime = (long)obj[6],
                    QuoteAssetVolume = double.Parse((string)obj[7]),
                    NumberOfTrades = (long)obj[8],
                    TakerBuyBaseAssetVolume = double.Parse((string)obj[9]),
                    TakerBuyQuoteAssetVolume = double.Parse((string)obj[10])
                });
            }

            return candleStickList;
        }
    }

    public class Candlestick
    {
        public long OpenTime { get; set; }
        public double Open { get; set; }
        public double High { get; set; }
        public double Low { get; set; }
        public double Close { get; set; }
        public double Volume { get; set; }
        public long CloseTime { get; set; }
        public double QuoteAssetVolume { get; set; }
        public long NumberOfTrades { get; set; }
        public double TakerBuyBaseAssetVolume { get; set; }
        public double TakerBuyQuoteAssetVolume { get; set; }
    }

I use the static method as follows
public async Task<IEnumerable<Candlestick>> GetCandleSticks(string symbol, string interval)
        {
            var endpoint = $"api/v3/klines?symbol={symbol}&interval={interval}";
            var response = await _httpClient.GetAsync(endpoint);

            if (response.IsSuccessStatusCode)
            {
                var result = (await response.Content.ReadAsStringAsync()).ToCandleStickList();

                return result;
            }

            throw new Exception($"");
        }

Does anyone got any suggestions for better code? Thanks in advance!

Json.NET has a very useful JsonConverter which you can use to add a custom JsonConverter. Which what you actually need.
So, you can do something like this :
public class CandlestickConverter : JsonConverter
{
    public override bool CanConvert(Type objectType)
    {
        return objectType == typeof(Candlestick);
    }

    public override object ReadJson(JsonReader reader , Type objectType , object existingValue , JsonSerializer serializer)
    {
        if(reader.TokenType == JsonToken.Null)
        {
            return null;
        }

        var candle = ( existingValue as Candlestick ?? new Candlestick() );

        var array = JArray.Load(reader);

        candle.OpenTime = array.ElementAtOrDefault(0)?.ToObject<long>(serializer) ?? 0;
        candle.Open = array.ElementAtOrDefault(1)?.ToObject<double>(serializer) ?? 0;
        candle.High = array.ElementAtOrDefault(2)?.ToObject<double>(serializer) ?? 0;
        candle.Low = array.ElementAtOrDefault(3)?.ToObject<double>(serializer) ?? 0;
        candle.Close = array.ElementAtOrDefault(4)?.ToObject<double>(serializer) ?? 0;
        candle.Volume = array.ElementAtOrDefault(5)?.ToObject<double>(serializer) ?? 0;
        candle.CloseTime = array.ElementAtOrDefault(6)?.ToObject<long>(serializer) ?? 0;
        candle.QuoteAssetVolume = array.ElementAtOrDefault(7)?.ToObject<double>(serializer) ?? 0;
        candle.NumberOfTrades = array.ElementAtOrDefault(8)?.ToObject<long>(serializer) ?? 0;
        candle.TakerBuyBaseAssetVolume = array.ElementAtOrDefault(9)?.ToObject<double>(serializer) ?? 0;
        candle.TakerBuyQuoteAssetVolume = array.ElementAtOrDefault(10)?.ToObject<double>(serializer) ?? 0;

        return candle;
    }

    public override void WriteJson(JsonWriter writer , object value , JsonSerializer serializer)
    {
        var candle = (Candlestick) value;
        serializer.Serialize(writer ,
            new[]
            {
                candle.OpenTime,
                candle.Open,
                candle.High,
                candle.Low,
                candle.Close,
                candle.Volume,
                candle.CloseTime,
                candle.QuoteAssetVolume,
                candle.NumberOfTrades,
                candle.TakerBuyBaseAssetVolume,
                candle.TakerBuyQuoteAssetVolume
            });
    }
}

Now, using this custom converter you can use the JsonConverter attribute on the class like this :
[JsonConverter(typeof(CandlestickConverter))]
public class Candlestick
{
    ...
}

Now you can do this directly :
var candles = JsonConvert.DeserializeObject<List<Candlestick>>(jsonObject);

So, you don't need to use extensions, you only need to do a custom converter which is the proper way to handle Json serialization when using Json.NET.


Project Euler 11: Largest product in a grid, Python3 Project Euler #11

  What is the greatest product of four adjacent numbers in the same
  direction (up, down, left, right, or diagonally) in the 20×20 grid?

I've seen some of the solutions of the problem and although they are in Python3, my solution has some differences.

Pylint rates it 7.27/10 and I will add docstring eventually. Some of the lines are too long(104/100) and fl is not a good name for a file and I agree.

The main issues are comparing product and product_temp and calculating the sum for a diagonal from down left to upper right is inconsistent with the calculating the other products.

The code:

#!/bin/env python3

"""
https://projecteuler.net/problem=11

In a 20x20 matrix iterate over all the rows, columns,
diagonals(left up -> down right, down left -> upper right)
and find the largest product of 4 numbers in the same direction

"""

def list_ints():
    with open('./textfiles/grid2020.txt') as fl:
        array = [[int(x) for x in line.split()] for line in fl]
    return array

def biggest_product():
    grid = list_ints()
    product = 0
    product_temp = 0

    col = 20
    row = 20
    for i in range(row):
        for j in range(col):
            #for row
            if i + 3 < 20:
                product_temp = grid[i][j] * grid[i + 1][j] * grid[i + 2][j] * grid[i + 3][j]

                if product < product_temp:
                    product = product_temp

            #for column
            if j + 3 < 20:
                product_temp = grid[i][j] * grid[i][j + 1] * grid[i][j + 2] * grid[i][j + 3]

                if product < product_temp:
                    product = product_temp

            #for diagonal from upper left to down right
            if i + 3 < 20 and j + 3 < 20:
                product_temp = grid[i][j] * grid[i + 1][j + 1] * grid[i + 2][j + 2] * grid[i + 3][j + 3]

                if product < product_temp:
                    product = product_temp

    #another loop from the other diagonal
    #diagonal left to upeer right
    for i in range(20, -1, -1):
        for j in range(20):
            if i + 3 < 20 and j - 3 >= 0:
                product_temp = grid[i][j] * grid[i + 1][j - 1] * grid[i + 2][j - 2] * grid[i + 3][j - 3]

                if product < product_temp:
                    product = product_temp
    return product

if __name__ == '__main__':
    print(biggest_product())

Edit:  Now I see the comments for calculating row and col comments are reversed.

One of the potential improvements would be to extract the logic of generating the 4 numbers in each of the directions (let's even generalize it for k numbers). What if we would create a generator that would do this for an arbitrary square matrix and k input numbers:

def generate_adjacent_items(grid, k):
    """
    For a square grid, generates lists of all possible k numbers in every direction
    (up, down, left, right and diagonally).
    """
    size = len(grid)
    for row in range(size):
        for col in range(size):
            # left -> right
            if row + k - 1 < size:
                yield [grid[row + index][col] for index in range(k)]

            # top -> bottom
            if col + k - 1 < size:
                yield [grid[row][col + index] for index in range(k)]

            # diagonal: top-left -> bottom-right
            if row + k - 1 < size and col + k - 1 < size:
                yield [grid[row + index][col + index] for index in range(k)]

            # diagonal: bottom - left -> top - right
            if row - k + 1 >= 0 and col + k - 1 < size:
                yield [grid[row - index][col + index] for index in range(k)]

(please recheck for off-by-ones)

Then, you can iterate over the generator and get your maximum number, e.g.:

from operator import mul
from functools import reduce

K = 4

# list_ints and generate_adjacent_items function definitions here

if __name__ == '__main__':
    sub_lists = generate_adjacent_items(list_ints(), K)

    max_sublist = max(sub_lists, key=lambda x: reduce(mul, x))
    print(max_sublist, reduce(mul, max_sublist))

Note that I'm using functools.reduce(), operator.mul and the built-in max() to determine the maximum of the multiplications, but you can do it "manually" the way you were doing it in your presented solution.

For the provided sample input, it prints:

[87, 97, 94, 89] 70600674


Avoiding nested if statements I occasionally find myself bumping into code like this (either in other projects or banging out initial prototypes myself):

 if @product.save
   if current_member.role == "admin"
     redirect_to krowd_path(@product)
   else
     redirect_to new_product_offer_path(@product)
   end
 else
   render :new
 end

What is a good way to avoid this type of situation all together?

One suggestion is to make the roles two classes, initialize them based on the role and call the save function.

 class UserRole
    def save(product)
     redirect_to new_product_offer_path(product)
    end
 end
 class AdminRole < UserRole
    def save(product)
     redirect_to krowd_path(product)
    end
 end
 def create_role(r)
    case r
    when :admin
        return AdminRole.new()
    else
        return UserRole.new()
    end
 end

 ....
 role = create_role(current_member.role)

 if @product.save
   role.save(@product)
 else
   render :new
 end


Image cache for lots of images using WeakReference on C# I'm developing an application to manage a player card-library from an online game. Each card has an image that is stored on disk with a numeric code, for example, the card with ID=1234 has his image stored as 1234.png and all the images are stored on the same folder.

As of now there are more than 3500 images for a total of 1 GB of hard disk usage.

The app doesn't need all the images loaded on memory, but depending on the context it may need from 8 to 100 images loaded on screen, and the next moment it may need a 100 different images.

As of now the images are loaded using IValueConverter and DataTemplate without storing on code behind the image, so everytime an image is needed, it is loaded from disk.

That is the current state of the project.

Now this is the code that I want to implement:

public class ImageAccessor
{
    private string Filepath;
    private Dictionary<int, WeakReference> References;

    public ImageAccessor(string filepath)
    {
        Filepath = filepath;
    }

    public BitmapImage GetImage(int id)
    {
        BitmapImage image = null;
        WeakReference reference = null;

        if (!References.TryGetValue(id, out reference))
        {
            image = GetFromDisk(id);
            reference = new WeakReference(image);
            References[id] = reference;
        }
        else
        {
            image = reference.Target as BitmapImage;
            if (image == null)
            {
                image = GetFromDisk(id);
                reference.Target = image;
            }
        }

        return image;
    }

    private BitmapImage GetFromDisk(int id)
    {
        Uri uri = new Uri(Filepath + id + ".png");
        return new BitmapImage(uri);
    }
}

I don't want to implement a singleton of that class but for now let's assume that my project always uses the same instance.

Everywhere that I read about WeakReference they recommend using it for "a single large object", but what if I want to use it on "a lot of relative small-medium size objects (200~500kb each)". Except from the advice of using them for big objects, those images meet the rest of the requirements:

Having all of them all the time on memory would be very costly (1GB on RAM)
Some images may not be used for the entire life of the app
Some images may be used only one or a couple of times
Some images may be used lots of times

And by using an image I mean displaying it on a Image control

I've never worked with weak references, and I still need to do some benchmarks to see if they are worth it, but I want to know if this code:

Is safe?
May incur on memory problems?
It can be improved?

For the sake of simplicity assume this:

The filepaths will always be correct
Every ID will correspond to an existing image always
Images will not be modified

It looks like weak reference would serve the purpose of what you are trying to achieve [IF] those images will go out of scope (not being directly referenced in any execution path) after handing them back to the caller of that method then they will be collected by GC at some point.
This MSDN article explains weak references in .net, I'll quote it here:

The garbage collector cannot collect an object in use by an application while the application's code can reach that object. The application is said to have a strong reference to the object.
A weak reference permits the garbage collector to collect the object while still allowing the application to access the object.
A weak reference is valid only during the indeterminate amount of time until the object is collected when no strong references exist. When you use a weak reference, the application can still obtain a strong reference to the object, which prevents it from being collected. However, there is always the risk that the garbage collector will get to the object first before a strong reference is reestablished.
Weak references are useful for objects that use a lot of memory, but can be recreated easily if they are reclaimed by garbage collection.

While your code should work fine, I would consider using an in-memory Cache instead with (either or both) capping the TTL (time to live) or count of objects that can occupy that cache (adding new items to the cache would invalidate and remove older items to maintain the capacity).
Check out MemoryCache class in .net framework.


UI error display window with task async in WPF I am new to async and was wondering if any improvement can be made to the solution I put together from scattered information on the web.

The following (working) code shows how a button press runs logic and if there are any errors they will appear in the returned task.Exception.Handle variable which I can check in my ContinueWith code. If it's not null, I open the error dialog window for the user. By passing in FromCurrentSynchronizationContext, I get to open the window on the UI thread and avoid issues with UI thread access.

Any improvements to make it more readable or suggest a different patter for this would be much welcomed.

public void StartButton()
{
    //prevent multiple button presses with IsRunning switch
    if (!IsRunning)
    {
        IsRunning = true;

        Threading.Task.Factory.StartNew(() =>
        {
            ProgramLogic();
            IsFabRunning = false;
        })
        .ContinueWith(p =>
        {
            if (p.Exception != null)
                p.Exception.Handle(x =>
                {
                    logger.Error(p.Exception);
                    windowManager.ShowDialog(new ForceCloseViewModel(p.Exception.Message), null, null);
                    IsFabRunning = false;
                    return true;
                });
        }, Threading.TaskScheduler.FromCurrentSynchronizationContext());
    }
}

This is a good fit for async await:

public async void RunProgramLogic()
{
    if (!IsRunning)
    {
        IsRunning = true;
        try
        {
            await Task.Run(() => ProgramLogic())
                      .ConfigureAwait(true); // schedules back to the synchronization context the method was called from.
        }
        catch (Exception e)
        {
            logger.Error(e); // Log or LogError feels like better names
            windowManager.ShowDialog(new ForceCloseViewModel(e.Message), null, null);
        }
        finally
        {
            IsRunning = false;
        }
    }
}

Regarding async await
As you can see using async and await removes quite a bit of noise from the code.
In general async void is smelly and should only be used for eventhavndlers.
In your case it feels like a decent fit since you handle the exceptions. Async void means it cannot be awaited.

Regarding command:
While I agree that this looks like a fit for a RelayCommand it is not trivial due to the async.

Here is a good read on commands for async scenarios.

Most RelayCommand implementations handle raising CanExecuteChanged poorly by calling CommandManager.InvalidateRequerySuggested which is very inefficient.

Here is a nice implementation.

Regarding scheduling
Always be explicit with .ConfigureAwait(true|false) when awaiting. The default is true which means that the code continues on the synchronizationcontext it came from. This is a strange default in my opinion and ConfigureAwait is a poorly named method.
In your case I think the logger and windowManager should be thread agnostic enabling .ConfigureAwait(false)
Suggested implementation:

public class WindowManager
{
    public void ShowDialog(object viewModel, ...)
    {
        if (Dispatcher.CurrentDispatcher.CheckAccess())
        {
            ShowDialogCore(viewModel, ...);
        }
        else
        {
            Dispatcher.CurrentDispatcher.BeginInvoke(new Action(() => ShowDialogCore(viewModel, ...)));
        }
    }

    private void ShowDialogCore(object viewModel, ...)
    {
        ...
    }
}

The upside with implementing it like this is that the caller will not need to remember to call it on the UI thread. It is the job of the windowmanager to schedule if needed. Hence fixing many potential bugs in one place.

Regarding the pattern Using a flag like this is fine and will work well if you refactor to relaycommand. A try-finally block makes it clear in my opinion.


LINQ approach to flatten Dictionary to string I have a Dictionary<string,string> and want to flatten it out with this pattern:

{key}={value}|{key}={value}|{key}={value}|

I tried with a LINQ approach at first but couldn't solve it, so I ended up writing an extension method like this:

public static string ToString(this Dictionary<string,string> source, string keyValueSeparator, string sequenceSeparator)
{
  if (source == null)
    throw new ArgumentException("Parameter source can not be null.");

  var str = new StringBuilder();
  foreach (var keyvaluepair in source)
    str.Append(string.Format("{0}{1}{2}{3}", keyvaluepair.Key, keyValueSeparator, keyvaluepair.Value, sequenceSeparator));
  var retval = str.ToString();
  return retval.Substring(0,retval.Length - sequenceSeparator.Length); //remove last  seq_separator
}

Is it possible to solve this with LINQ?

Something like this should work:

public static string ToString(this Dictionary<string,string> source, string keyValueSeparator, string sequenceSeparator)
{
  if (source == null)
    throw new ArgumentException("Parameter source can not be null.");

  var pairs = source.Select(x => string.Format("{0}{1}{2}", x.Key, keyValueSeparator, x.Value));

  return string.Join(sequenceSeparator, pairs);
}


Optimal way to filter DataFrame by many arguments, without usage of multiple ifs and for loops I am learning about survival analysis and I made some implementation in Python using lifelines and plotly. However depending on the number of arguments I would have to almost rewrite my function again because of all these ifs and fors. Could you suggest the way I could make it smarter?
Here is my code:

def classicLifelineFigure(frames, timeName = None, protocol = None, mutation = None, getter = getDfToSurvivalAnalysis):

    '''

    Function that returns lifeline figure object to be displayed on the website

    Parameters:

        frames --> dictionary with frames for features, mutations etc.
        timeName --> time on which we perform analysis
        protocol --> name of protocol if present in analysis
        mutation --> name of mutation if present in analysis
        getter --> function to get separate dataframes for analysis from one big dataframe

    '''

    kmf = KaplanMeierFitter()
    fig = go.Figure()

    if timeName == None:
        return fig

    dfToAnalysis = getter(frames, timeName, protocol, mutation)
    dfToAnalysis['TIME'] = dfToAnalysis['TIME'] / 365

    if protocol != None:

        if mutation != None:

            for prot in set(dfToAnalysis['PROTOCOL'].values):
                for mut in set(dfToAnalysis['MUTATION'].values):

                    tempDf = dfToAnalysis[(dfToAnalysis['PROTOCOL'] == prot) & (dfToAnalysis['MUTATION'] == mut)]
                    results = kmf.fit(tempDf['TIME'], tempDf['EVENT'])
                    traceDf = results.survival_function_.copy()
                    fig.add_trace(go.Scatter(
                        x = traceDf.index,
                        y = traceDf.iloc[:,0],
                        mode = 'lines',
                        name = 'Protocol value = {}, mutation value = {}'.format(prot, mut)
                    ))

        if mutation == None:
            for prot in set(dfToAnalysis['PROTOCOL'].values):
                tempDf = dfToAnalysis[dfToAnalysis['PROTOCOL'] == prot]
                results = kmf.fit(tempDf['TIME'], tempDf['EVENT'])
                traceDf = results.survival_function_.copy()
                fig.add_trace(go.Scatter(
                    x = traceDf.index,
                    y = traceDf.iloc[:,0],
                    mode = 'lines',
                    name = 'Protocol value = {}'.format(prot)
                ))

    elif mutation != None:
        for mut in set(dfToAnalysis['MUTATION'].values):

            tempDf = dfToAnalysis[dfToAnalysis['MUTATION'] == mut]
            results = kmf.fit(tempDf['TIME'], tempDf['EVENT'])
            traceDf = results.survival_function_.copy()
            fig.add_trace(go.Scatter(
                x = traceDf.index,
                y = traceDf.iloc[:,0],
                mode = 'lines',
                name = 'Mutation value = {}'.format(mut)
            ))

    else:
        results = kmf.fit(dfToAnalysis['TIME'], dfToAnalysis['EVENT'])
        traceDf = results.survival_function_.copy()
        fig.add_trace(go.Scatter(
            x = traceDf.index,
            y = traceDf.iloc[:,0],
            mode = 'lines',
            name = 'Survival function'
        ))

    fig.update_layout(showlegend = True)

    return fig

Comparisons to None should always be done with is or is not. This is because None is a singleton object and this will give you the right behavior even if you override some objects __eq__ method or similar. This is mentioned in Python's official style-guide, PEP8. It also recommends no spaces around = for keyword arguments and using lower_case for variables and functions.

As to your actual question, what you really want to do is apply some function (fitting and plotting) to either the whole dataset or grouped by one or more categorical values.
The latter is what pandas.DataFrame.groupby is for.

kmf = KaplanMeierFitter()

def plot_fit(df, fig, name='Survival function'):
    results = kmf.fit(df['TIME'], df['EVENT'])
    trace_df = results.survival_function_.copy()  # Is this copy really needed?
    fig.add_trace(go.Scatter(
        x=trace_df.index,
        y=trace_df.iloc[:,0],
        mode='lines',
        name=name
    ))

def do_fit(df, by=None):
    fig = go.Figure()
    if by is None:
        plot_fit(df, fig)
    else:
        for group, df2 in df.groupby(by):
            name = ...  # Somehow determine label from group
            plot_fit(df2, fig, name)
    fig.update_layout(showlegend=True)
    return fig

This function can be called either as do_fit(df), do_fit(df, "PROTOCOL") or do_fit(df, ["PROTOCOL", "MUTATION"]). The group variable is a tuple telling you which protocol and/or mutation it is, extracting a correct label from this is left as an exercise. I also omitted getting the dataframe with your getter function and other preparations. These should probably be done in the function calling this function.

It could be argued that this should be divided up even further and the fitting should be separated from the plotting.


Filtering a list based on conditions I'm currently filtering a list based on several conditions, which may or may not be present:

    def requests = Request
        .list()
        .findAll { if (exportFrom) { it.tsCreated.millis >= exportFrom.millis } else { it } }
        .findAll { if (exportTo) {  it.tsCreated.millis <= exportTo.millis } else { it } }
        .findAll { if (clientId) { it.resolvedClient?.clientId == clientId } else { it } }
        .findAll { if (params.exportType) { it.typeName == params.exportType } else { it } }
        .findAll { if (params.exportStatus) { it.status.toString() == params.exportStatus } else { it } }

For instance, exportFrom is a date coming from the user interface. If it's present, we only want to export requests that occurred on that date or later.  If not, we can disregard that test.

it will never be 0 or "", as it from a list of objects that have been loaded from the database and so have already been validated.

While this is readable and works, it seems like a poor solution and probably won't be very performant on a large dataset.  Is there a Groovy idiom for this kind of series of selective filters?

Few problems here.

Fragility and possible inefficiency by returning {it} rather than true
Iterating over the collection more often than is necessary (even though the later iterations may be over smaller sets).
Duplication of the same pattern

Point 1: Fragility

Just return true (oh, and not { true }).  That's what you want.  By returning the object you risk some odd circumstance where the object evaluates to false.  Why expose yourself to that risk?  And returning true is just cheaper.

Point 2: Repeated iteration

Each call to findAll is returning a new collection, which you iterate over with the next call.  That's a waste.  You can do it with one findAll and one closure.  The simplest (and most naive) way to do that would be this:

def requests = Request
    .list()
    .findAll { (exportFrom ? it.tsCreated.millis >= exportFrom.millis : true)
        && (exportTo ? it.tsCreated.millis <= exportTo.millis : true)
        && (clientId ? it.resolvedClient?.clientId == clientId : true)
        && (params.exportType ? it.typeName == params.exportType : true)
        && (params.exportStatus ? it.status.toString() == params.exportStatus : true)
        && true }

That will do just what your code does only without creating four intermediate copies of the list.  It is, however, ugly.  So one approach would be to turn each of those criteria into a function which takes a parameter and returns true or false.  So you would have

def requests = Request
    .list()
    .findAll { cond1(it) && cond2(it) && cond3(it) && cond4(it) && cond5(it) }

Which is at least cleaner.  However, let's consider

Point 3: Duplication of a pattern

You have a set of basic conditions.  Each, if true, has a matching predicate with which you test the list item for validity.  If the condition is not valid/present, you accept the list item.  So...

Imagine you create a Criteria class.  It should have two properties

criterion: the condition which, if true, requires a list element to be tested.  Should probably be a closure.
predicate: the test (another closure) to apply to the list element

The Criteria class should also have a reject method which might look like this

    boolean reject (Object item) {
        if (this.criterion(item)) ! this.predicate(item) else false
    }

(I used Object because I'm not sure what type is actually in your list)

Now you can create a list of Criteria objects, each containing a criterion and a matching test.  Lets say you call it criteriaList.  Now the main bit of code can look like this:

def requests = Request
    .list()
    .findAll { x -> ! criteriaList.find { c -> c.reject(x) } }

If none of the criteria objects reject the item, then the find call returns null (empty list) and ! null evaluates to true.  No rejections, so we like this item.

On the other hand, if a list item is going to fail one of the tests, then find will return the first Criteria object in the list which rejected the item (and not try any others after that).  So in that case ! criteriaList.find will evaluate to false and the item will be discarded.

Can you see why I used a reject method (which returns true if the test fails) rather than an accept method (which would return true if the test succeeds)?  I did that because find returns as soon as it finds a match, so we want to return as soon as one Criteria object rejects the list item.

There are other ways to do this (e.g. closure composition, although that's a more functional approach which you said isn't your style) but I hope this is a good example which will help you think of your own solutions.


Is there a 'better' way to find files from a list in a directory tree I have created a list of files using find, foundlist.lst.

The find command is simply find . -type f -name "<search_pattern>" > foundlist.lst

I would now like to use this list to find copies of these files in other directories.

The 'twist' in my requirements is that I want to search only for the 'base' of the file name. I don't want to include the extension in the search.

Example:

./sort.cc is a member of the list. I want to look for all files of the pattern sort.*

Here is what I wrote. It works. It seems to me that there is a more efficient way to do this.

./findfiles.sh foundfiles.lst /usr/bin/temp

#!/bin/bash
# findfiles.sh
if [ $# -ne 2 ]; then
    echo "Need two arguments"
    echo "usage: findfiles <filelist> <dir_to_search>"
else
    filename=$1
    echo "$filename"
    while read -r line; do
        name=$line
        # change './file.ext' to 'file.*'
        search_base=$( echo ${name} | sed "s%\.\/%%" | sed "s/\..*/\.\*/" )
        find $2 -type f -name $search_base
    done < $filename
fi

In think you can do three things to improve your script(s):

Try to avoid complexity O(N^2)
Try to reuse intermediate results
Try to reduce extra process calls

The first aspects comes about because you compare the ALL files in the first directory to ALL files in the second directory. If both directories scale more or less by N you will have a rather slow algorithm for really large directories.

The second aspect relates to calling find again and again instead of calling it once and then using the output of the call for comparison.

The third aspect relates to e.g. calling sed twice in a pipe for something that can be done by the bash itself.

I have prepared a slightly different approach which is far from elegant and/or perfect but which may give you some ideas. The key features are:

It only calls find once for each directory and stores the result in files.
Instead of a nested loop (with find being the inner loop) it uses sort with complexity O(NlogN) to find the matches.
It focuses on the basenames of the filenames for comparison. Only for the result the "long" names are looked up again.

This is the script:

#!/bin/bash
dir1=$1
dir2=$2

# list all files in dir a using format "filename filepath"
find $dir1 -type f -printf "%f %p\n" > dir1.lst

# sort filename portion alphabetically and append " 1" as source
cat dir1.lst | while read filename filepath
do
    echo ${filename%.*}
done | sort -u | while read filename
do
    echo "$filename 1"
done > dir1base.lst

# list all files in dir b using format "filename filepath"
find $dir2 -type f -printf "%f %p\n" > dir2.lst

# sort filename portion alphabetically and append " 2" as source
cat dir2.lst | while read filename filepath
do
    echo ${filename%.*}
done | sort -u | while read filename
do
    echo "$filename 2"
done > dir2base.lst

# now merge the two lists together; sort them by filename first and by source second
last=""
sort -k 1 -k 2  -u dir1base.lst dir2base.lst | while read filename dir
do
    # if we find an entry from source 2 that had the same filename from source 1 just before we have a match
    if [[ $dir -eq 2 && $last == $filename ]] ; then
        # output match
        echo $filename
    fi
    last=$filename
done | while read match
do
    # now grep for matches in both original files
    echo "*** File(s)"
    grep $match dir1.lst
    echo "*** match file(s)"
    grep $match dir2.lst
    echo "---"
done

Note that the output loop still has complexity O(M^2) with the inner loop being in the grep call. However, in this case M scales with the number of matches and not the original input size. The former should be considerably smaller.

Also note the first find command will have to be adapted to be more selective than it is now.


Queue implementation using arrays Please review the code:

    package com.gmail.practice;

import java.util.Arrays;

public class Queues {

    int head;
    int tail;
    int size;
    int[] queue;

    public Queues(int arraysize)
    {
        if(arraysize<0)
        {
            throw new IllegalArgumentException("size cannote be less than equals zero");
        }
        head = -1;
        tail = -1;
        size = arraysize;
        queue = new int[size];
    }

    public boolean isEmpty()
    {
        return(tail == -1 && head == -1);
    }

    public void enqueue(int value)
    {
        if((tail+1)%size == head)
        {
            System.out.println("the queue is full");
        }else if(isEmpty())
        {
            head++;
            tail++;
            queue[tail] = value;
        }else{
            tail = (tail+1)%size;
            queue[tail] = value;
        }
    }

    public int dequeue()
    {
        int value = 0;
        if(isEmpty())
        {
            System.out.println("the queue is empty cant dequeue");
        }else if(tail == head){
            value = queue[head];
            head = -1;
            tail = -1;
        }else{
            value = queue[head];
            head = (head+1)%size;
        }

        return value;
    }

    public void display()
    {
        System.out.println(Arrays.toString(queue) + " " + "tail is " + " "+ tail +" the head is" + " " + head);
    }

    public static void main(String[] args)
    {
        Queues q = new Queues(5);
        q.enqueue(3);
        q.enqueue(4);
        q.enqueue(2);
        q.enqueue(1);
        q.enqueue(5);
        q.display();
        q.dequeue();
        q.dequeue();
        q.display();
        q.enqueue(1);
        q.enqueue(5);
        q.dequeue();
        q.dequeue();

    }
}

I think the value variable in the dequeue method is potentially destructive. rather than printing "the queue is empty cant dequeue", and returning the default value of the value variable, I think it's better to throw an exception and catch it. For example on why it is destructive:

    if (        q.dequeue() == 5) // this will return 0 if the array is empty.

This is not good. The user will think the value that is being returned is 0 though it's already empty.

You initialized the size variable, but you didn't really do anything to it in your queue operations (E.G. size-1 when you dequeue an item). It doesn't make sense for the size variable to not change if you enqueue/dequeue items from the queue. But I'm not sure why it's there yet you don't have a size() method. If you're not going to implement a size method() (you should), I think the size variable is useless.
I think it would be more flexible if the array is resizable (E.G. size*2 if full, size/4 if too large).
Make it generic so you can add other types.


Good randomness in Python code for a lottery-like game I'm trying to build a lottery-like game in Python 3.6 where a function selects a winner from a participant list where every participant has a winning chance weighted by the value of the tickets they bought. I'm using the SystemRandom class in order to avoid software-state-dependency. Is this a good practice regarding good enough randomness? How could this code be improved? Also, does the shuffling does add anything to the randomness?

import random

# Randomly selects an element from a choice list using weighted probabilities
def randomWeightedSelection(choices, weights):

    # Build index list
    indices = list(range(len(choices)))

    # Shuffle indices
    random.shuffle(indices)

    # Shuffle weights accordingly
    shuffled_weights = [weights[i] for i in indices]

    # Create random generator
    rng = random.SystemRandom()

    # Select element
    selected_index = rng.choices(indices, weights=shuffled_weights, k=1)[0]

    return [choices[selected_index], selected_index]

According to the Python documentation random.SystemRandom() uses os.urandom() which

  [...] returns random bytes from an OS-specific randomness source. The returned data should be unpredictable enough for cryptographic applications, though its exact quality depends on the OS implementation.

Given that your system does a good enough job, the output of random.SystemRandom() should be indistinguishable from true randomness, and thus be good enough for your purpose.

Also, if the randomness provided by your system is indistinguishable from true randomness, shuffling does not add anything to the randomness of your output (even if you had also used a cryprographically secure RNG for shuffling).

In this case your code could be reduced to

import random

def randomWeightedSelection(choices, weights):
    rng = random.SystemRandom()

    indices = list(range(len(choices)))

    selected_index = rng.choices(indices, weights = weights, k = 1)[0]

    return [choices[selected_index], selected_index]


Generate a random integer of length N with unique digits The task is simple: given N, generate a random integer with no repeating digits.

'''
Generate a random number of length, N and all digits
are unique
'''
from __future__ import print_function
import random
from collections import OrderedDict

# keep generating till all are unique
# This is a brute force approach where I store the digits generated
# so far in a OrderedDict and if the next random number is already
# there, i ignore it.
def randN(n):
    digits = OrderedDict()
    while len(digits) < n:
        d = random.randint(0, 9)
        if d == 0 and not digits.keys():
            continue
        else:
            if not digits.get(str(d), None):
                digits[str(d)] = 1
    return int(''.join(digits.keys()))

def _assert(randi, n):
    assert len(str(randi)) == n
    assert len(set(str(randi))) == n

for _ in range(100000):
    _assert(randN(10), 10)
    _assert(randN(1), 1)
    _assert(randN(5), 5)

I have a feeling there is a better approach to solve this, which is why I am posting it here.

Not quite one line, but here's a much simpler solution:

import random

def randN(n):
    assert n <= 10
    l = list(range(10)) # compat py2 & py3
    while l[0] == 0:
        random.shuffle(l)
    return int(''.join(str(d) for d in l[:n]))

Explanation:

The problem wants N unique digits. To get those, first make a list of all the digits, then shuffle them and cut off the unwanted digits (it should be noted that if you have many more than 10 digits, shuffling the whole lot would be expensive if you only want a handful).

To turn the list of digits into an integer, the shortest way is to stringify each digit and then join them into one string, and parse that back into an integer.

But wait! If the first digit is 0, the parsed string will be one digit shorter sometimes. So, earlier in the code, simply repeat the shuffle until some other digit comes first.

Since the initial order of the list has the 0 at the beginning, we don't need to do an extra shuffle before the loop.


Finding out players gender and name in text based adventure game #include <iostream>
#include <string>
#define NL "\n"

int main(void) {

    //Acquires players first name
    std::string introOne = "Welcome to The World of Magick\nWhat is your name?\n";
    std::cout << introOne;

    std::string playerName;
    std::cin >> playerName;

    //Asks for players gender
    while(true) {
    std::cout << "Are you a boy or a girl?" << NL << "1. Boy | 2. Girl"
    << NL << "Choices are selected by typing in a single number" << NL;

    int playerGenderSelect;
    std::string playerGender;
    std::cin >> playerGenderSelect;

        //Sets gender depending on value entered
        switch(playerGenderSelect) {

        case 1:
        playerGender = "male";
        break;

        case 2:
        playerGender = "female";
        break;

        default:
        std::cout << "Not a valid number, try again" << NL;

    }

    //If the player entered a valid number, break loop, if not, continue it.
    if(playerGender == "male" or playerGender == "female") {
        break;
    }
    else {
        continue;
    }
}

    return 0;
}

I wrote this code for a text based adventure game I'm making too keep me busy over the weekend.

I usually do this kind of boredom-project in python but decided to tackle it in C++. I was wondering if there were any obvious taboos or things that could obviously be optimized better.

I see a number of things that may help you improve your code.

Use objects

The code has two things that look like a menu.  It also collects two pieces of data about a single player.  Those things, menu and player could and probably should be objects.  Perhaps you're a beginning programmer, and haven't learned about objects yet, but this kind of repeated task with associated data is really well-suited to object-oriented programming and that's something that C++ is very good at expressing.  For an example of a usable menu object, see this answer.

Prefer const variables to #define

Instead of using the old C-style #define as in this line:

#define NL "\n"

It's generally better and more type-safe to use a const variable like this:

static const std::string NL{"\n"};

However, I'd probably not define this particular one in either case and simply type \n where it's needed.

Use constant string concatenation

The code currently contains this:

std::cout << "Are you a boy or a girl?" << NL << "1. Boy | 2. Girl"
<< NL << "Choices are selected by typing in a single number" << NL;

But you don't really need to do it that way which potentially calls the << operator six times.  Instead, you could express the same thing as this:

std::cout << "Are you a boy or a girl?\n"
    "1. Boy | 2. Girl\n"
    "Choices are selected by typing in a single number\n";

Which is both more readable and only calls << once.  The compiler automatically concatenates the string literals together.

Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

Sanitize user input better

The code doesn't quite work as posted.  If I enter a string such as "Edward" to answer a question, the program stays in an endless loop.  It would be better to read a (text) line in and then convert it to a number.  Users can do funny things and you want your program to be robust.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


Making error messages more efficient I am using a way of making error messages, and it works fine. It is just starting to get a little annoying as my project gets larger.

I was wondering if there was a way to do with with out having to rename every part of the error, and make a code so it would pick up the function it occurred in.

Private Sub tsbEditNeighbour_Click(sender As Object, e As EventArgs) Handles tsbEditNeighbour.Click
    ' Error Checking

    On Error GoTo Err_tsbEditNeighbour_Click

    Dim CurrentRow As DataRowView = TryCast(PropertyNeighboursLoadRecordsBindingSource.Current, DataRowView)

    ' Load Warranty
    Dim uNeighbours As New frmNeighbours
    With uNeighbours
        .PropertyID = Val(lblPropertyIDValue.Text)
        .NeighbourID = Val(CurrentRow("NeighbourID"))
        .LoadNeighbours()
        .ShowDialog()
        Me.Show()
    End With

Err_tsbEditNeighbour_Click:
    If Err.Number <> 0 Then
        sErrDescription = Err.Description
        WriteAuditLogRecord(Me.Name, "tsbEditNeighbour_Click", "Error", sErrDescription)
        MsgBox("System Error occurred" & Chr(13) & "tsbEditNeighbour_Click" & Chr(13) & sErrDescription, MsgBoxStyle.Exclamation + MsgBoxStyle.OkOnly, "AztecCRM - Error Reporting")
    End If
End Sub

There's probably much more to say about this code, but there's one point that deserves immediate and serious attention: You're handling errors the way vba and vb6 did it... but this code can only be vb.net:

Handles tsbEditNeighbour.Click

In the .NET world, you throw away all this On Error GoTo Madness, and work with Exceptions instead.

Exceptions are a much more robust way of handling errors, and give you much more information than just an error number and description (including the whole stack trace, and the exact code file and line number that threw the exception): see System.Exception on MSDN.

The basic idea is that you try some code, and catch to handle any exceptions thrown:

Try
    'some code
Catch ex As Exception
    'error-handling
End Try

See Try-Catch-Finally statements on MSDN for the full syntax.

I'll get back to this post later, to review other points.