Escaping XML to be used with SOAP I'm attempting to escape ampersands from a string before passing to PHP's SimpleXMLElement->addChild() for use with some SOAP webservices, but I don't want to double escape them. I'm getting these strings from a variety of sources so I can't count on them not being escaped already.  &amp;amp; or &amp; should be untouched but & or &blah blah blah should not.

<?php $s = preg_replace('/&([^; ]*)( |$)/','&amp;$1$2',$s)

Your code is hard to read, and a few test cases would be nice to know what this should handle. For example, do you want to allow multiple lines? Do you want to replace things that are not "amp", like &gt;? Are you sure this will correctly sanitize your inputs?

It seems to me that you only want to replace & (ampersand + space) by &amp;.

This version works:

for any kind of whitespace
across multiple lines:

$s = preg_replace('/&(\s)/m', '/&amp;$1/', $s);


Cleaning parameters before passing them to a model I'm writing an API and want to be able to pass in the attributes of a model without prefixing them with the name of the model.

I wrote this little extension to ActiveRecord in order to make it happen:

module ActiveRecord
  class Base
    def self.filter_attributes(hash)
      hash.stringify_keys.slice(*self.accessible_attributes.to_a)
    end
  end
end

And I use it like this:

class IdeaController < ActiveRecord::Base
  def create
    @idea = Idea.create(Idea.filter_parameters(params))
    respond_with @idea
  end
end

I have two questions:

What do you think about the name of the method? filter_parameters() seemed to make sense, but it's a little generic.
Are there uses of this that I'm not thinking of and that would cause problems, such as passing in nested attributes?

Honsetly, I don't see any sense in your extension.
Can you tell me what is the difference between

@idea = Idea.create(Idea.filter_parameters(params))

and

@idea = Idea.create(params)

mass assignment sanitizing make this code safety.


Refactoring try blocks with async/await I have two functions which I would like to simplify. They have largely the same logic, but one is asynchronous and uses one await in a try block. I'd like to have the logic in one place, but could not figure out a clean way of doing that. Any suggestions?

from requests import get, HTTPError

# Dummy functions for external functionality
def sync_send(url):
    return get(url)

async def async_send(url):
    return get(url)

def next(url):
    if url is None:
        return

    try:
        response = sync_send(url)
        return response.json()
    except HTTPError as e:
        if e.response.status_code == 404:
            return
        else:
            raise

async def async_next(url):
    if url is None:
        return

    try:
        response = await async_send(url)
        return response.json()
    except HTTPError as e:
        if e.response.status_code == 404:
            return
        else:
            raise

So the point of this all is to provide a way of requesting for resources in an API two ways, sync and async. Despite the simple (synchronous) dummy functions, in async_next we use asynchronous IO to retrieve things. Other than that, the situation is exactly as it is here.

I have accepted that the if url is None cannot be refactored, and I currently think my only option is to make a more complex exception hierarchy, raising a dedicated 404 error to catch it without logic. Any other ideas?

Let's have async implementation:

async def async_next(url):
    if url is None:
        return

    try:
        response = await async_send(url)
        return response.json()
    except HTTPError as e:
        if e.response.status_code == 404:
            return
        else:
            raise

Then you can provide following sync bridge:

def next(url):
    return asyncio.run(async_next(url))

Or the following if the event loop is available and running:

def next(url):
    return loop.run_until_complete(async_next(url))

Note: check asyncio high level commands for more details


IndexOrDefault that functions like FirstOrDefault I don't know why this is not in the .NET Library, but I need to be able to use an index against a list and not have it throw an exception if it is outside the bounds of the list.

This is what I came up with:

public static class LinqHelpers
{
    public static TListType IndexOrDefault<TListType>(this List<TListType> list,
                                                      int index)
    {
        var count = list.Count();
        if (index > count -1)
            return default(TListType);
        if (index < 0)
            return default(TListType);
        return list[index];
    }
}

This is my test:

List<string> somelist = new List<string> {"B", "A", "Q", "a", "t", "h"};
for (int i = -100; i < 100; i++)
{
    Console.WriteLine(somelist .IndexOrDefault(i));
}

I would appreciate any advice on missing error handling or if I am reinventing an already existing feature.  I also would not mind advice on a way to use an IEnumerable instead of a List with out iterating the IEnumerable more than once.

public static TListType IndexOrDefault<TListType>(this List<TListType> list, int index)

You're extending a concrete type, and the type parameter's name is misleading - it's not the type of the list, it's the type of its items. I'd just call it T. IList<T> calls it T, so I don't see a problem with T ;)

You should be extending an interface, in this case probably an IEnumerable<T>, like LINQ extension methods do.

var count = list.Count();

You're iterating the List<T> through its IEnumerable<T> interface (doesn't apply to List<T> because a List<T> is also an ICollection<T> see @svick's comment below, but in other cases that would be \$O(n)\$ already), and you haven't tried to find anything yet! A List<T> doesn't need to be iterated to retrieve its count, just use the Count property getter, like this:

var count = list.Count;

Actually this whole block:

    var count = list.Count();
    if (index > count -1)
        return default(T);
    if (index < 0)
        return default(T);
    return list[index];

Could be simplified to:

if (index > list.Count - 1 || index < 0)
{
    return default(T)
}

return list[index];

But that's still extending a List<T>. You're going to want to extend IEnumerable<T> instead... but the method you're implementing already exists in System.Linq:

public static T IndexOrDefault<T>(this IEnumerable<T> source, int index)
{
    return source.ElementAtOrDefault(index);
}

You're duplicating the functionality of the IEnumerable<T>.ElementAtOrDefault extension method.


Determining if three numbers are consecutive This code will determine if three numbers are consecutive for any order they're supplied to the method (any permutation of [n, n+1, n+2] should be accepted). Is there any way to make this code easier for me to write, or make the code more efficient? Any feedback would be most welcome!

I tried testing with 3! combinations of 2, 3, 4, and it seemed to work. I also tried 1, 2, 9, and 1, 0, 1. All cases seem to work.

public static boolean consecutive(int a, int b, int c) {
    if ( a == b || b == c || a == c) {
        return false;
    } else if (((a == b + 1 || a == b - 1) || (a == c + 1 || a == c - 1)) && ((b == c + 1 || b == c - 1))
               || ((b == c + 1 || b == c - 1) || (a == b + 1 || a == b - 1)) && (a == c + 1 || a == c - 1))  {
        return true;
    }
    return false;
}

The problem that I have with writing something like the original code is that it is complicated to be sure that it does the right thing in every situation.  You've taken three values and perform whopping fifteen comparisons on them.  What if you'd missed one?  The code would almost work, except it would occasionally (in that one situation) return the wrong result.

A conceptually simple way to handle this is

public static boolean consecutive(int... numbers) {
    Arrays.sort(numbers);

    for (int i = 1; i < numbers.length; i++) {
        if (numbers[i] != numbers[i-1] + 1) {
            return false;
        }
    }

    return true;
}

Sorting saves a lot of logic here.  Since we know the order, we can just check the differences directly.  If any adjacent numbers are not consecutive, we can return false.  If it makes it all the way through, they all must be consecutive.

Note that this also handles other than three numbers.  For example, it always returns true if there's zero or one number passed.  This may or may not be what you want.

Now we do five manual comparisons (including the comparison of i to numbers.length) to determine that three numbers are consecutive.  The sort does more comparisons, but we don't have to worry if we got those correctly.  The chances of there being an error in sort that made it through the Java compiler's testing is extremely low.

I find this version much easier to read and verify correctness than the original code.  For most cases, the difference in runtime is going to be minimal and unimportant.  My ability to write code that looks like this quickly and be sure it works is usually going to be more important.  And if you really need blazing speed, there are always options like @200_success offers.


Parsing CSV header for validation I have a simple application that reads a couple of csv files and outputs the input into catalogues for the website.  However, users have a habit of opening the csv files and inserting extra columns etc and it causes obvious issues, I have tried to tell people not mess with the files with no success, so i thought i would come up with a way round it.  Is there a better way of doing it than this?

    private static List<CatalogueLine> ImportCatalogueLines()
    {
        List<CatalogueLine> tempCatalogueLines = new List<CatalogueLine>();

        try
        {
            using (StreamReader reader = new StreamReader(ConfigurationManager.AppSettings["catalogue-lines-import-file"]))
            {
                int? catCodeColumn = null;
                int? prodCodeColumn = null;

                // Validate the column headers
                string[] header = reader.ReadLine().ToLower().Split(',');

                for (int i = 0; i < header.Length - 1; i++)
                {
                    if (header[i] == "catcode")
                        catCodeColumn = i;

                    if (header[i] == "prodcode")
                        prodCodeColumn = i;
                }
                if (catCodeColumn.HasValue == false || prodCodeColumn.HasValue == false)
                {
                    Console.WriteLine("Unable to validate the column headers, cancel the import.");
                    return tempCatalogueLines;
                }

                // Loop the rest of the lines.
                while (!reader.EndOfStream)
                {
                    // Split the line
                    string[] catalogueLine = reader.ReadLine().Split(',');
                    tempCatalogueLines.Add(new CatalogueLine
                    { CatCode = catalogueLine[(int)catCodeColumn], ProdCode = catalogueLine[(int)prodCodeColumn] });
                    Console.Write($"\rImporting Catalogue Lines:\t{ catalogueLine[(int)prodCodeColumn] } ");
                }
            }
            Console.Write($"\rImporting Catalogue Lines:\tCompleted.                    ");
        }
        catch (FileNotFoundException e)
        {
            Console.WriteLine($"{ e.Message }");
            return tempCatalogueLines;
        }
        catch (IOException e)
        {
            Console.WriteLine($"{ e.Message }");
            return tempCatalogueLines;
        }
        return tempCatalogueLines;
    }

Input Injection

Your function seems to depend at least on the input file, but this file is asked for by the function. You should follow the IoC principle and pass the file from outside (through function parameter).

var file = ConfigurationManager.AppSettings["catalogue-lines-import-file"];
var lines = ImportCatalogueLines(file);

Maybe like that, but dont get confused, as your method Is private, this should be pushed even further, somewhere outside the entire class....

Always False Conditional

if (header[i] == "catcode")
                        catCodeColumn = i;

                    if (header[i] == "prodcode")
                        prodCodeColumn = i

It Is impossible for header[i] to be equal to two different values at the same time. Use else if or switch.

Duplicit Columns Check

Further if you are super paranoid you might want to check that catCodeColumn And prodCodeColumn Is not assigned more than once. That would mean there Is a duplicit column header (one that you care of).

Redundant Comparisions

catCodeColumn.HasValue == false || prodCodeColumn.HasValue == false

The comparision to false Is redundant, use the negation operator "!".

!catCodeColumn.HasValue || !prodCodeColumn.HasValue

Or use de Morgans law

!(catCodeColumn.HasValue && prodCodeColumn.HasValue)

Split by comma or do proper csv parsing?

In case some column value contains a comma character (not talking about the quotes needed for it to work), your implementation Will fail to handle it. Of course only if the csv ever contains such values. Even if it should not, you say that users can modify it as they like. Time to get paranoid!

Error Reporting

You should not log everything to Console inside the function.  Especially not if you caught an exception, mute it and return possibly half-built result. Instead throw a custom exception, possibly let it carry the half-built list. But dont return with incomplete result. And put the logging to Its own scope.

General Remarks

Other than that I think your approach Is quite reasonable. If you cannot hardcode or configure the columns offset ať compile time, you have to determine it dynamically... But ofc if you used a proper csv parser with additional features like header checking, the code would look quite different, probably shorter...


Recreating strstr in C I'm trying to recreate strstr in c, my code works but I think it is too long, and it needs some improvement.

#include <stdlib.h>

static int      find_str_indx(char *str, int index, char *str_to_find)
{
    int i;
    int temp;
    int found_index;

    while (str[index] != '\0')
    {
        if (str[index] == *str_to_find)
        {
            temp = index;
            i = 0;
            found_index = index;
            while (str_to_find[i] != '\0' && str[index] != '\0')
            {
                if(str_to_find[i] != str[index])
                    found_index = -1;
                i++;
                index++;
            }
            if (found_index == -1)
                index = temp;
        }
        index++;
    }
    return (found_index);
}

char    *my_strstr(char *str, char *to_find)
{
    char    *found_str;
    int     found_index;
    int     i;

    found_str = (char*)malloc(sizeof(found_str));
    found_index = find_str_indx(str, 0, to_find);
    if (found_index != -1)
    {
        i = 0;
        while (str[found_index] != '\0')
        {
            found_str[i] = str[found_index];
            found_index++;
            i++;
        }
    }
    else
        found_str = NULL;
    return (found_str);
}

UPDATE

I have rewritten the code to get rid of allocating memory inside strstr, as advised by ratchet freak and Jerry Coffin.

char    *my_strstr(const char *haystack, const char *needle)
{
    size_t  needle_len;

    needle_len = strlen(needle);
    while (*haystack)
    {
        if (*haystack == *needle)
        {
            if (!strncmp(haystack, needle, needle_len))
                    return ((char *)haystack);
        }
        haystack++;
    }
    return (NULL);
}

This strikes me as somewhat...clumsy code that makes the job more difficult than truly necessary.

I'd break the task up into pieces, something like:

for every candidate position in the haystack
    if the needle matches at this point
        return success at this point
return failure

So that leaves us with two subordinate tasks:

Figure out the candidate positions
Check for a match starting at a specified point

The first is pretty simple: the candidate positions are the beginning of the haystack through the length of the haystack minus the length of the needle (and if the needle is larger than the haystack, it can't possibly match).

The second is even simpler: step through both strings, and if they don't match at a given point, return indicating failure. If (and only if) you reach the end of the second without a mismatch, return indicating success.

So let's write some code for that:

size_t length(char const *s) {
    size_t i;
    for (i = 0; s[i]; i++)
        ;
    return i;
}

bool match(char const *a, char const *b) {
    while (*b)
        if (*a++ != *b++)
            return false;
    return true;
}

Then we can do the function itself:

char const *find_substr(char const *haystack, char const *needle) {
    size_t len1 = length(haystack);
    size_t len2 = length(needle);

    for (size_t i = 0; i+len2<=len1; i++)
        if (match(haystack + i, needle))
            return haystack + i;
    return NULL;
}

Then we probably want a bit of test code to verify that it works to at least some degree under some circumstances:

char const *check(char const *s) {
    if (s)
        return s;
    return "[NULL POINTER]";
}

#define elements(arr) (sizeof(arr)/sizeof(arr[0]))

int main() {
    char input1[] = "This is some input for the function";

    // tests: match middle, non-match, match begin, match end, empty string
    char const *tests[] = { "for", "was", "This", "ion", "" };

    for (size_t = 0; i < elements(tests); i++)
        printf("%s\n", check(find_substr(input1, tests[i])));

    printf("%s\n", check(find_substr("the", "this is a longer string")));
}

There are certainly other ways the job could be done, but that seems to me like it's at least a reasonable way to implement what's probably the simplest (and most naive) one.


Excel VBA script – concatenates multiple values using Loop I have an Excel VBA script that concatenates five values: three static text strings, and values contained in two dynamic user-input ranges.  One of the ranges contains values that need to be concatenated repeatedly in the output, top to bottom, over and over, until the end of the data in the other range is reached.

So, given sample values:

  TextA: Alpha
  TextB: Gamma
  TextC: Delta

  LoopRange:

  AAA
  BBB
  CCC

  UserRange:

  111
  222
  333
  444
  555
  666
  777
  888

The output should be:

AlphaAAAGamma111Delta
AlphaBBBGamma222Delta
AlphaCCCGamma333Delta
AlphaAAAGamma444Delta
AlphaBBBGamma555Delta
AlphaCCCGamma666Delta
AlphaAAAGamma777Delta
AlphaBBBGamma888Delta

The script I've written works, but in a rather janky way.  It builds the output in stages — creating a new column of data, combining it with existing values, combining those results with remaining values until the output is achieved, then deleting the leftovers.

The enhancement I'm looking for is how to avoid the piecemeal process currently in place.  I'm thinking there's a way to nest the Loop inside a For statement but I haven't been able to figure it out.

Sample of the worksheet

Sub LoopAndConcat()

Application.ScreenUpdating = False
Application.DisplayAlerts = False

Dim TextA As String
Dim TextB As String
Dim TextC As String
Dim LoopRange As Range
Dim CellA As Long
Dim CopyRange As Range
Dim CellB As Range
Dim LastRow As Long
Dim CellC As Long

TextA = ActiveSheet.Cells(3, "A").Value
TextB = ActiveSheet.Cells(6, "A").Value
TextC = ActiveSheet.Cells(9, "A").Value

Set LoopRange = Range(ActiveSheet.Cells(12, "A"), ActiveSheet.Cells(Rows.Count, "A").End(xlUp))

Do
CellA = CellA + 1
LoopRange.Copy Range("E" & Rows.Count).End(xlUp)(2)
Loop Until CellA = 10

Set CopyRange = Range(ActiveSheet.Cells(2, "E"), ActiveSheet.Cells(Rows.Count, "E").End(xlUp))

For Each CellB In CopyRange
  If Not CellB.Offset(0, -3).Value = "" Then
    CellB.Offset(0, -2).Value = TextA & CellB.Value & TextB
  End If
Next CellB

LastRow = Range("B" & Rows.Count).End(xlUp).Row

For CellC = 2 To LastRow
ActiveSheet.Cells(CellC, "C").Value = ActiveSheet.Cells(CellC, "C").Value & _
                                      ActiveSheet.Cells(CellC, "B").Value & _
                                      TextC
Next CellC

CopyRange.ClearContents

Application.ScreenUpdating = True
Application.DisplayAlerts = True

If Not LoopRange Is Nothing Then Set LoopRange = Nothing
If Not CopyRange Is Nothing Then Set CopyRange = Nothing

End Sub

The approach I took to your problem led me to separate it into two stages: the first determine the locations of the source data and the second processes the data to perform the concatenation.

Initially capturing your source data, I used the following data setup:

A few things to note:

Always use Option Explicit for the reasons listed there
Always define and set references to all Workbooks and Sheets
Work with Arrays, Not With Ranges

And so, based on those concepts, the setup method looked like this:

Option Explicit

Sub DataSetup()
    Dim wb As Workbook
    Set wb = ThisWorkbook

    Dim ws As Worksheet
    Set ws = wb.Sheets("Sheet1")

    Dim staticText(1 To 3) As String
    staticText(1) = ws.Range("A2")
    staticText(2) = ws.Range("A3")
    staticText(3) = ws.Range("A4")

    Dim startRow As Long
    Dim lastRow As Long
    Dim loopRange As Range
    startRow = 4
    lastRow = ws.Cells(ws.Rows.Count, "C").End(xlUp).Row
    Set loopRange = ws.Cells(startRow, "C").Resize(lastRow - startRow + 1, 1)

    Dim userRange As Range
    startRow = 3
    lastRow = ws.Cells(ws.Rows.Count, "E").End(xlUp).Row
    Set userRange = ws.Cells(startRow, "E").Resize(lastRow - startRow + 1, 1)

    Dim results As Variant
    results = LoopAndConcat(staticText, loopRange, userRange)

    Dim resultsRange As Range
    Set resultsRange = ws.Range("G2").Resize(UBound(results, 1), 1)
    resultsRange = Application.Transpose(results)
End Sub

You may notice that I didn't pass arrays to the LoopAndConcat function. We only really need that data as an array inside the function, no where else.

Inside the work of the concatenating function, I took advantage of the fact that your static data is limited to three strings. Because of this, you can do all the work in a single loop. Working from arrays and storing the results in an array makes this function very fast. The resulting array of data can be located anywhere in the workbook you need it, the function doesn't need to care where those results end up.

Function LoopAndConcat(fixedText() As String, _
                       loopArea As Range, _
                       userArea As Range) As Variant
    '--- for speed, copy the data to memory arrays
    '    (expected Range is "n" rows by one column)
    Dim loopData As Variant
    Dim userData As Variant
    loopData = loopArea
    userData = userArea

    '--- establish results array, properly sized
    Dim results() As Variant
    ReDim results(1 To UBound(userData, 1)) As Variant

    Dim finalText As String
    Dim i As Long
    Dim j As Long
    j = 1
    For i = 1 To UBound(userData, 1)
        finalText = fixedText(1) & loopData(j, 1) & _
                    fixedText(2) & userData(i, 1) & _
                    fixedText(3)
        results(i) = finalText
        j = j + 1
        If j > 3 Then
            j = 1
        End If
    Next i
    LoopAndConcat = results
End Function

My results:


Avoiding a call to CommandBarControl.Picture Earlier today, I encountered an Access Violation error when opening the VBE for SolidWorks 2006, while Rubberduck VBA was initializing.

I traced the Access Violation back to an attempt to set the Picture property of a CommandBarButton, and then posted on SO

Initially, I suspected that SolidWorks might have implemented the VBE in some wacky out-of-process way, as the Picture property uses StdOle's IPictureDisp which will throw an AccessViolation on crossing a process boundary.

But upon skipping the Picture assignments, Rubberduck was able to fully load, and I discovered that SolidWorks 2006's VBE has CommandBarControl's that don't have Picture or Mask properties.

A little more digging, and it became clear that Office first introduced Picture and Mask in Office XP, but evidently SolidWorks hadn't updated the CommandBars within their VBE implementation (they possibly used an older version of the VBA SDK?).

Rubberduck is using the embedded types of the Office Interop Assembly that contains the CommandBar types, and that is from after Office XP.

So it seems that accessing the non-existent Picture and Mask properties would ordinarily cause a catchable exception, but because they involve StdOle, .NET sees a process boundary has been crossed and throws an AccessViolation.

I had to find a way of safely setting the picture, so I've elected to cast the button to a dynamic and then try reading the property. I could have just cast to dynamic and set the Picture, but it seemed to deserve its own helper function, HasPictureProperty...

    public static void SetButtonImage(CommandBarButton button, Image image, Image mask)
    {
        button.FaceId = 0;
        if (image == null || mask == null)
        {
            return;
        }

        if (!HasPictureProperty(button))
        {
            return;
        }

        try
        {
            button.Picture = AxHostConverter.ImageToPictureDisp(image);
            button.Mask = AxHostConverter.ImageToPictureDisp(mask);
        }
        catch (COMException exception)
        {
            Logger.Debug("Button image could not be set for button [" + button.Caption + "]\n" + exception);
        }
    }

    private static bool HasPictureProperty(CommandBarButton button)
    {
        try
        {
            dynamic control = button;
            object picture = control.Picture;
            return true;
        }

        catch (Microsoft.CSharp.RuntimeBinder.RuntimeBinderException exception)
        {
            Logger.Debug("Button image cannot be set for button [" + button.Caption + "], because Host VBE CommandBars are too old.\n" + exception);
        }

        return false;
    }

Am I abusing dynamic in this instance? Should I have perhaps have opted for reflection over catching what might be many RuntimeBinderException's?

BTW - The upside of this fix will mean that Rubberduck VBA should now support Office 2000 and earlier, albeit the Rubberduck toolbar buttons won't have images.

SetButtonImage is called for every single CommandMenuItem in Rubberduck, regardless of whether or not Picture and Mask are overridden.

Whether a CommandBarButton has a Picture property depends on the [version of the] host application, and the bool result of HasPictureProperty will be the same for every single CommandMenuItem being initialized: if a RuntimeBinderException is thrown with the first button, it will be thrown and logged for every single button - this is a massive performance hit: throwing and catching exceptions like this, is pretty expensive.

I'm ok with the dynamic cast - though some reflection might actually perform better, but without changing the logic to stop making that test for every single button it would still be inefficient.

The first HasPictureProperty return value will be the value returned by every single subsequent call: you need to change the logic so as to avoid making that check more than once.


Validation class to avoid ugly if-else blocks I've answered this question on Stack Overflow. The problem is - there are 30 columns in an Excel sheet and I need to validate each of the fields with different validation logic.  The code should avoid a lot of if-else blocks. I have used strategy pattern to solve this. I would like to get a review of this code.

interface IValidator<T> {
  boolean validate(T field);
}

class SomeFieldOne<T> implements IValidator<T> {
  public boolean validate(T field) {
    System.out.println("SomeFieldOne validation");
    return true; // return true/false based on validation
  }
}

class SomeFieldTwo<T> implements IValidator<T> {
  public boolean validate(T field) {
    System.out.println("SomeFieldTwo validate");
    return true; // return true/false based on validation
  }
}

class Context {
  private IValidator validator;

  public Context(IValidator validator) {
    this.validator = validator;
  }

  public boolean validate(String field) {
    return this.validator.validate(field);
  }
}

public class TestValidation {
  public static void main(String[] args) {
    Context context;

    context = new Context(new SomeFieldOne());
    System.out.println(context.validate("some field one"));

    context = new Context(new SomeFieldTwo());
    System.out.println(context.validate("some field two"));

    // test other fields ....
    // .........
  }
}

First of all, you're not using your generics properly here:

private IValidator validator;

I would change this to

private final IValidator<String> validator;

Because:

final because the value does not change, and should not change. The field should be immutable
<String> to use generics properly. Otherwise you should get a compiler warning saying IValidator is a raw type. References to generic type IValidator<E> should be parameterized

However, I think your Context is not very useful. I don't see any reason for why you need it. You can accomplish the exact same things by using:

public class TestValidation {
  public static void main(String[] args) {
    IValidator<String> context;
    context = new SomeFieldOne();
    System.out.println(context.validate("some field one"));
    context = new SomeFieldTwo();
    System.out.println(context.validate("some field two"));
    // test other fields ....
    // .........
  }
}

This is less code and has the same effect. And it also looks better and improves readability as changing strategy now calls only one constructor instead of two.

I assume you're compiling with Java 1.6 or above, and therefore your classes that implements the IValidator interface should mark the validate method with @Override to comply with the Java coding conventions. (Marking methods with @Override that overrides interface methods was not supported at all in previous Java versions)

@Override
public boolean validate(T field) {


Hackerrank Day 8: Dictionaries and Maps solution in C++ This is a solution for the Day 8 hackerrank.com challenge. The basic idea of the challenge is to create a mapping of names to phone numbers based on the the given input, and later look up the phone numbers for certain names. If the entry exists, print it and its number. If it doesn't, print out "Not found".

Here's my solution in C++:

#include<iostream>
#include<map>
using namespace std;
int main()
{
    int i, n;
    cin>>n;
    string name, number, key;
    map<string, string> phone_dir;
    for(i=0; i<n; i++)
    {
        cin>>name>>number;
        phone_dir.insert(pair <string, string> (name, number));
    }
    while(cin>>key)
    {
        if (phone_dir.find(key) != phone_dir.end())
        {
             cout<<key<<"="<<phone_dir.find(key)->second<< endl;
        }
        else cout<< "Not found"<<endl;
    }

    return 0;

}

Is there a more efficient solution in C++ for this problem?

You should not use using namespace std; This is bad practice that will get you into trouble once you want to use a function that has the same name.
You can use descriptive names. So instead of "n" use "numEntries" or whatever.
You should declare the iteration variable inside the for loop rather than at the beginning.
std::map has an emplace method that you should use rather than insert.
You are using a std::map, which is is a red black tree. You most likely want a std::unordered_map
You search twice for the key. With modern C++17 you can do the following:

if (auto res = phone_dir.find(key); res != phone_dir.end())
{
     std::cout << key << "=" << res->second << std::endl;
} else {
     std::cout << "Not found" << std::endl;
}

You are using std::endl which involves a flush of the stream. You definitely only want a \n here.
Whitespace is cheap. Grab some to improve readability
Same for lines. It is bad practice to have braces on one condition and pu the other one on the same line. This severely affects readability
return 0; is implied for main so you can skip that.
I almost forgot, you should reserve the space in the map.


Script to load other .js scripts I made a simple script to load other scripts into the page without worrying about caching. It's meant to be used by a considerable amount of people, so I would like it to be as clean as possible, and I would like your advice.

window.load_scripts = function () {
    var callbacks = {
        jqueryCallback: function (script_path) {
            $.getScript(script_path, function (data, textStatus, jqxhr) {
                if (jqxhr.status === 200) {
                    console.log(script_path + ' loaded correctly.');
                } else {
                    console.log(script_path + ' failed to load');
                }
            });
        },
        vanillaCallback: function (script_path) {
            var script = document.createElement('script');
            script.src = script_path + '?d=' + (new Date()).getTime();
            document.head.appendChild(script);
        }
    }
    var callback;
    //if we don't have arguments, just return.
    //no scripts have to be loaded
    if (!arguments || !arguments.length)
        return;

    //if we have jquery, we will use $.getScript
    callback = window.$ ? callbacks['jqueryCallback'] : callbacks['vanillaCallback'];

    //loop through all scripts
    for (let i = 0; i < arguments.length; i++) {
        var path = arguments[i];
        //just be sure that the argument is a string
        if (typeof path === "string") {
            callback(path);
        }
    }
}

few suggestions...

I would lose the jQuery part and stick to vanilla js for several reasons

jQuery's getScript doesn't add a script to the page like your vanilla example does, getScript actually uses eval to execute the code, which means any safe mode scripts will not be able to change any global state.
$ is used by lots of libraries, not just jQuery. There's a real high chance you're going to get a false positive if you're only checking for $ rather than jQuery.
The jQuery code is just unnessecary. Your vanilla code will always work, there is no benefit to using jQuery.

Don't forget to add your onload callback so scripts that use this to include other scripts can use those other scripts after they have loaded.
Declare your function. There is no benefit to doing window.whatever = function(){};. when you use function whatever(){} the function does not need to be defined at the top of the script, plus it's less bits.

Here's a function I wrote to include jQuery and stuff in userscripts, feel free to take it or parts of it.

function loadScripts(scripts, appendTo) {
  appendTo = appendTo || "body";
  return new Promise((done) => {
    (function recurse(i) {
      if (undefined === scripts[i]) return done();
      let src = scripts[i];
      let script = document.createElement('script');
      script.type = 'text/javascript';
      if (script.readyState) { //IE
        script.onreadystatechange = () => {
          if (script.readyState == 'loaded' || script.readyState == 'complete') {
            script.onreadystatechange = null;
            setTimeout(() => {
              recurse(++i);
            }, 1);
          }
        };
      } else { //Others
        script.onload = () => {
          setTimeout(() => {
            recurse(++i);
          }, 1);
        };
      }
      script.src = src;
      document.getElementsByTagName(appendTo)[0].appendChild(script);
    })(0);
  });
}

// Example usage
loadScripts(["https://code.jquery.com/jquery-2.2.4.min.js"]).then(()=>{
  $("body").append("loaded with jQuery");
});


Comparing hash members to other hash members I've written a script that deletes a member of a hash if an array within the has contains all the other elements of another array within the same hash.

It works but it looks a little messy. Is there a better way to do this?

def title_contains_blocks(candidate, titles, index)
  test_group = titles.dup
  test_group.delete_at(index)
  test_group.each do |title|
    return true if true === title[:blocks].all?{|block| candidate[:blocks].include?(block)}
  end

  return false
end

titles = [
  {:blocks => [1,2,3,4,5,6]},
  {:blocks => [1,2,3]},
  {:blocks => [4,5,6]}
]

titles = titles.delete_if.each_with_index {|candidate, index|
  title_contains_blocks(candidate, titles, index)
}

puts titles.inspect
#=> [{:blocks=>[1, 2, 3]}, {:blocks=>[4, 5, 6]}]

Convert the Hashes to Arrays, then use the difference operations on them.

a = { :a => 1, :b => 2, :c => 2 }
b = { :a => 1 }

p (b.to_a - a.to_a).empty?

You can do other things, like identify if two Hashes overlap in any places, by use the intersection too:

p (a.to_a & b.to_a).any?


Secure logout: session termination I've been reading the security issue on logging out from a website system written in PHP, using sessions.

My current code is:

session_start();

if (isset($_SESSION["logged_in"])) {
    unset($_SESSION["logged_in"]);
    unset($_SESSION["ss_fprint"]);
    unset($_SESSION["alive"]);
    session_destroy();
    session_regenerate_id(true);
}
// NEW MODIFIED CODE
session_start();

if (isset($_SESSION["logged_in"])) {

$_SESSION = array();

if (ini_get("session.use_cookies")) {
    $params = session_get_cookie_params();
    setcookie(session_name(), '', time() - 42000,
        $params["path"], $params["domain"],
        $params["secure"], $params["httponly"]
    );
}

session_destroy();

header("Location: ../index.php");
die();
} else {
header("Location: ../online.php");
die();
}

I use this class.

The code from the class should ensure and protect against hijacking and capture and fixation.

I have generated a session with this code from the above link, and I want to logout properly.

I tried print_r() out all $_SESSION data, and it was empty after I ran my logout code.

Is my logout secure enough?

OBS:: This system I'm making is not for some big company with a huge big mega need for security, but the basics should be implemented.

looks alright enough. i would change is replace all those unset() lines with just $_SESSION = array();

and check the manual, it has a sample to clear your session cookies if you have them enabled.


Breadth and Depth First Search in Java BFS and DFS in Java. Please give suggestions!!

Class Main {
    public void bfs()
    {
        // BFS uses Queue data structure
        Queue queue = new LinkedList();
        queue.add(this.rootNode);
        printNode(this.rootNode);
        rootNode.visited = true;
        while(!queue.isEmpty()) {
            Node node = (Node)queue.remove();
            Node child=null;
            while((child=getUnvisitedChildNode(node))!=null) {
                child.visited=true;
                printNode(child);
                queue.add(child);
            }
        }
        // Clear visited property of nodes
        clearNodes();
    }

    public void dfs() {
        // DFS uses Stack data structure
        Stack stack = new Stack();
        stack.push(this.rootNode);
        rootNode.visited=true;
        printNode(rootNode);
        while(!stack.isEmpty()) {
            Node node = (Node)s.peek();
            Node child = getUnvisitedChildNode(n);
            if(child != null) {
                child.visited = true;
                printNode(child);
                s.push(child);
            }
            else {
                s.pop();
            }
        }
        // Clear visited property of nodes
        clearNodes();
    }
}

Class Node {
   Char data;
   Public Node(char c) {
      this.data=c;
   }
}

There seem to be some inconsistencies here. What is s? I guess this is the stack? Also declaring a class Main is a little strange. You certainly want to have a main method, but a Main class?

Also I'd suggest you not use raw collections e.g. replace

Queue queue = new LinkedList();

with

Queue<Node> queue = new LinkedList<Node>();

You may like to consider implementing DFS recursively so that you don't have to explicitly use a stack, although that's a matter of taste. Also, where is getUnvisitedChildNode() defined?  Where are the edges of the graph actually stored? This could be e.g. an adjacency list carried around by each node, or perhaps in another object, but it should be somewhere.


How can I optimize my insert function for the cuckoo hash table? I'm working on class for a hash table where collisions are corrected using cuckoo hashing, I had difficulty writing the insert function, the function is supposed to insert the key if the h(key) is available, if not it is supposed to find the shortest "path" for moving other keys in order to find a place for the inserted key, I'm supposed to have maximum maxdepth moves.
Is there any way to improve my code?

class Cuckoo:
    def __init__(self,funcs,tableSize,maxDepth):
        self.table = [None]*tableSize
        self.funcs = funcs
        self.excess = []
        self.maxDepth = maxDepth
        self.size = tableSize

    def insert(self,key):
        if Cuckoo.find(self, key) != None: # if key exists
            return None
        # key doesn't exist
        min_insert_way = Cuckoo.insert2(self, key, [], 0)
        if min_insert_way[0] == -1: # if we can't commit max 3 replacement
            self.excess.append(key)
            return -1
        # we can commit max 3 replacement
        i = len(min_insert_way) - 1
        orginal_place = self.funcs[min_insert_way[-1]] (key,self.size) # it is the index of key in which it entered
        while ( i>=0 ):
            function_index = min_insert_way[i]
            replaced_element = self.table[self.funcs[function_index](key, self.size)]
            self.table[self.funcs[function_index](key, self.size)] = key # we replace he replaced element with key
            key = replaced_element
            i = i - 1
        return orginal_place

    def insert2(self, key, way, displacements):
        '''
        the function get 4 parameters:
        1. self == the hash table
        2. key
        3. way which is in the begging: way == []
        4. displacements == how many self.funcs have used

        the function return the way in which the insert will be. "way" is list of indexes of the self.funcs in which we do the insert
        for exmple, if way == [2,3], we will put "key" in self.table[self.funcs[3]] instead of "replaced_element".
        then, we will put "replaced_element" in self.table[self.funcs[2]].
        as you can see, the indexes are from the right to the left.
        if we have to use self.excess: way == [-1, X(1), X(1), ... X(max_depth)]
        '''
        if (displacements > self.maxDepth): # if we couldn't find max 3 displacements
            return [-1]
        for i in range(0, len(self.funcs)):
            if self.table[self.funcs[i](key,self.size)] == None: # if one of the functions gives empty place
                return [i]
        # there is no an empty place
        replaced_element = self.table[self.funcs[0](key,self.size)] # "replaced_element" will be replaced by "key"
        min_way = Cuckoo.insert2(self, replaced_element, way, displacements+1)
        min_way.append(0) # because we have used self.funcs[0]
        for i in range (1,len(self.funcs)):
            replaced_element = self.table[self.funcs[i](key,self.size)] # "replaced_element" will be replaced by "key"
            current_way = Cuckoo.insert2(self, replaced_element, way, displacements+1)
            current_way.append(i) # because we have used h(i)
            if len(current_way) < len(min_way):
                min_way = current_way
        return min_way

For one thing, I would suggest using a better name for your helper-function, that describes what it does:

def _find_insert_path(...):

Note the leading underscore - this is Python convention for "don't call this directly from outside the class". Users should call insert, which doesn't have the underscore.

Second, you could use exceptions to do handle the case where the insertion depth is exceeded more cleanly. Alter _find_insert_path to do:

if displacements > self.maxDepth:
    raise KeyError("Maximum insertion path length exceeded.")

Which makes insert use:

try:
     min_insert_way = self._find_insert_path(key, [], 0)
except KeyError:
    self.excess.append(key)
    return -1
...

Note the call to _find_insert_path; this is an instance method, you don't need to call it on the class and pass the instance Cuckoo.method(self, ...) you can call it directly on the instance self.method(...).

This:

for i in range(0, len(self.funcs)):
    if self.table[self.funcs[i](key,self.size)] == None:
        return [i]

Is a pretty awkward way of looping over self.funcs, not least because the start argument to range already defaults to 0. Instead, use enumerate:

for i, func in enumerate(self.funcs):
    if self.table[func(key, self.size)] is None:
        return [i]

Note that I am testing for the singleton None with identity (is) not equality (==) per PEP-0008.

Your loop over min_insert_way is similarly awkward; creating and incrementing your own index is usually a sign you're doing something wrong. You could instead use:

for function_index in reversed(min_insert_way):

Fianlly, it seems odd that your helper function returns a list of indices for self.funcs. Instead, why not have it return the list of indices within self.table to which each item in turn is moved? Then you don't have to call the hash functions at all in insert, which becomes much simpler:

def insert(self, key):
    if self.find(key):
        return None
    try:
        path = self._find_insert_path(key, [], 0)
    except KeyError:
        self.excess.append(key)
        return -1
    for index in reversed(path):
        key, self.table[index] = self.table[index], key
    return path[-1]

Note the parallel assignment of key and the value at self.table[index]; this uses tuple packing and unpacking to replace three lines of code with one and remove the temporary variable replaced_element.

This will also make the loop I mentioned above even simpler; you don't even need enumerate:

for func in self.funcs:
    i = func(key, self.size)
    if self.table[i] is None:
        return [i]

You could also make it neater using default arguments:

def _find_insert_path(self, key, path=None, depth=0):
    if path is None:
        path = []
    ...

Now the call from insert is simply:

path = self._find_insert_path(key)


Using QSqlQuery::bindValue() for variable number of placeholders I am familiar with the simple use of QSqlQuery::prepare() and QSqlQuery::bindValue() as in:

QSqlQuery query;
query.prepare("SELECT name, address FROM members WHERE id = :id");
query.bindValue(":id", member_id);
query.exec();

In this scenario I show a list of people (members) to the user, all with a check-box in front. The user can simply check which members to delete.
So now, the number of :id are variable.

Currently, I solve this issue in quite a untidy fashion. All the id's checked people will be put in an array of integers called checked_items_ids and then I simply for-loop that array:

QString prepare_query = QString("DELETE FROM members WHERE id = ").append(checked_items_ids[0]);

for(int i = 1; i < checked_items_ids.count(); i++)
    prepare_query.append(QString(" OR id = %1").arg(checked_items_ids[i]));

QSqlQuery query(prepare_query);
query.exec();

Is there a 'tidier' way to do this? Using bindValue() or anything that I might be overlooking?

I'm not the expert for QSqlQuery, so my answer might be slightly off the mark. I see nothing wrong in not using prepare() and bindValue(). It also seems to me a bit "odd" to use these in a DELETE statement. As I said, I'm not the expert on this, there may be other guys that can shed some light on this aspect.

I see two cosmetic issues:

the way you use checked_items_ids, first time with null argument, second time iterating. A QStringList can help in constructing the SQL statement.
setting the SQL statement in the constructor. Setting it in the exec() function might be more "natural".

These are cosmetic issues only. They do, however, improve readability of your code.

QStringList querylist;
for(auto id : checked_items_ids)
    querylist << QString("id = %1").arg(id);
QString querystring = QString("DELETE FROM members WHERE ") +
                  querylist.join(" OR ");

QSqlQuery query;
query.exec(querystring);


Filter a collection in a PagedCollectionView I have a scenario wherein I have to filter a collection in a PagedCollectionView.
The filter criteria are Name and status which are checkboxes with Enabled and Disabled labels. If Enabled is checked it should filter out the list with all enabled rules and vice-versa. I have given it a try, but I believe this isn't the best way of doing it.

     private PagedCollectionView _autoDepRuleList;
    public PagedCollectionView AutoDepRuleList
    {
        get
        {
            return this._autoDepRuleList;
        }
        set
        {
            this._autoDepRuleList = value;
            RaisePropertyChanged("AutoDepRuleList");
        }
    }

    private bool _isEnabled;
    public bool IsEnabled
    {
        get
        {
            return this._isEnabled;
        }
        set
        {
            this._isEnabled = value;
            RaisePropertyChanged("IsEnabled");
        }
    }

    private bool _isDisabled;
    public bool IsDisabled
    {
        get
        {
            return this._isDisabled;
        }
        set
        {
            this._isDisabled = value;
            RaisePropertyChanged("IsDisabled");
        }
    }

    private string _name;
    public string Name
    {
        get
        {
            return this._name;
        }
        set
        {
            this._name = value;
            RaisePropertyChanged("Name");
        }
    }

     public void Search()
    {
        var tempList = new AutoDeploymentRuleList();
        var filterList = new AutoDeploymentRuleList();

        foreach (AutoDeploymentRule item in this.AutoDepRuleList)
        {
            tempList.Add(item);
        }
        if (IsEnabled && IsDisabled)
        {
            foreach (var item in tempList)
            {
                filterList.Add(item);
            }
        }
        else if (IsEnabled)
        {
            foreach (var item in tempList.Where(r => r.Status.Equals("Enabled")))
            {
                filterList.Add(item);
            }
        }
        else if (IsDisabled)
        {
            foreach (var item in  tempList.Where(r => r.Status.Equals("Disabled")))
            {
                filterList.Add(item);
            }
        }
        else
        {
            foreach (var item in tempList)
            {
                filterList.Add(item);
            }
        }
    this.AutoDepRuleList = new PagedCollectionView(filterList.Where(r => r.Name.ToLower().Contains(this.Name.ToLower())));

Now the idea is to do away with the above block and go for the below logic. I did it for the Name property, but I'm not sure how I can do it for the Status property which can be anything either Enabled or Disabled.

    this.AutoDepRuleList.Filter = new System.Predicate<object>(o =>
    {
        AutoDeploymentRule rule = (AutoDeploymentRule)o;

        if (rule.Name.ToLower().IndexOf(this.Name.Trim().ToLower()) >= 0)
        {
            return true;
        }
        else
        {
            return false;
        }
    });
}

How can I optimize it by removing those redundant where clauses and putting the entire thing under Predicate<object>?

If performance is your concern, won't boxing and unboxing the variable unnecessarily add an overhead? Why would you want to do that?

Wouldn't this be much easier?

this.AutoDepRuleList //if this is queryable/enumerable, or convert first
.Where(r => (isDisabled && r.Status.Equals("Disabled")) ||
             (isEnabled && r.Status.Equals("Enabled")) ||
              (!isDisabled && !isEnabled))
//either ToList() if you just need to assign to another list, or ForEach if you want
//to iterate over list and do something
    .ForEach(filterList.Add);

Update: had forgotten the condition where both isDisabled and isEnabled were false.


Bootstrap file input styler This simple JavaScript code styles all file inputs to match with Bootstrap's style.

Simply, it turns this:

Into this input:

(function(window){
    window.jQuery(function($){
        $('input[type="file"]').each(function(){
            var $this = $(this);
            var $parent = $this.parent();
            $this.detach();
            $parent.prepend(
                $('
' +
                    '' +
                        '' + ($this.attr('data-label') || 'New image:') + '' +
                    '' +
                    '<input id="' + $this.attr('id') + '_path" type="text" class="form-control" readonly="readonly">' +
                    '' +
                        '' +
                            ($this.attr('data-button-label') || 'Browse') +
                            '

' +
                        '' +
                    '' +
                '
')
            );

            $this.appendTo('#' + $this.attr('id') + '_input-group label:last-child div:last-child');

            var $path = $('#' + $this.attr('id') + '_path', $parent).click(function(){
                $this.click();
            });

            $this.bind('change.input_file_styler', function(){
                $path.val(this.files[0] ? this.files[0].name : '');
            });
        });
    });
})(Function('return this')());

I really can't put my finger on what's wrong... but I know that something is stinking in there!

What can I improve in this code?

If you want to try it out:

(function(window){
 window.jQuery(function($){
  $('input[type="file"]').each(function(){
   var $this = $(this);
   var $parent = $this.parent();
   $this.detach();
   $parent.prepend(
    $('
' +
     '' +
      '' + ($this.attr('data-label') || 'New image:') + '' +
     '' +
     '<input id="' + $this.attr('id') + '_path" type="text" class="form-control" readonly="readonly">' +
     '' +
      '' +
       ($this.attr('data-button-label') || 'Browse') +
       '

' +
      '' +
     '' +
    '
')
   );

   $this.appendTo('#' + $this.attr('id') + '_input-group label:last-child div:last-child');

   var $path = $('#' + $this.attr('id') + '_path', $parent).click(function(){
    $this.click();
   });

   $this.bind('change.input_file_styler', function(){
    $path.val(this.files[0] ? this.files[0].name : '');
   });
  });
 });
})(Function('return this')());

<link href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" rel="stylesheet" type="text/css"/>

<input type="file" id="file" data-label="Select file to upload:" data-button-label="...">

Function('return this')()

It took two minutes to me to understand what it does. Let's see what it does step by step

Function('return this')()

This is another way of creating function. An anonymous function which simply returns this object. () at the end of it will invoke this function immediately. This is equivalent to window. Why not just use window instead of writing this mysterious statement.

Do we need IIFE?

IIFE is used to avoid polluting global scope. In the code, there is no code other than window.jQuery(function($){ inside it. Thus, IIFE is not containing any variable or function that could be global. As variables/functions defined inside a function are not accessible from outside of it(provided variables are declared using var keyword), the callback function of window.jQuery(function($){ serves as the container scope.

In simple words, IIFE can be removed safely.

jQuery(function($) {

I, generally don't use the shorthand of ready. It looks confusing for readers who are not familiar with it.

jQuery(document).ready(function() {

is readable. jQuery, call the function when document is ready.

window.jQuery

window here is not required as jQuery is global object and only jQuery will work. No harm in using window but using only jQuery will save some keystrokes.

If you're not using any other library like prototype.js, MooTools, YUI, etc. that also uses $ namespace, you can use $ instead of typing jQuery.

See Avoiding Conflicts with Other Libraries

:file selector

You may use :file pseudo-selector to select <input type="file" /> elements. This again, saves some keystrokes and is readable too.

$('input:file').each(function() {

Caching $this.attr('id')

$this.attr('id') is used 6 times in the code. This can be cached just like you cached $(this) so that, jQuery don't have to read the attribute value again and again.

var id = $this.attr('id'); // Cached the ID of element

and use id in the code.

attr() vs data()

To read the value of data-* attribute, jQuery has provided data() method.
Instead of $this.attr('data-button-label'), $this.data('buttonLabel') can be used.

bind()

bind() is deprecated. To bind events us on(). bind internally calls on. By using on() directly, yo'll save one extra function call. See this answer on StackOverflow.

jQuery(document).ready(function($) {
  'use strict';

  $('input:file').each(function() {
    var $this = $(this);
    var $parent = $this.parent();
    var id = $this.attr('id');

    $this.detach();
    $parent.prepend(
      $('
' +
        '' +
        '' + ($this.data('label') || 'New image:') + '' +
        '' +
        '<input id="' + id + '_path" type="text" class="form-control" readonly="readonly">' +
        '' +
        '' +
        ($this.data('buttonLabel') || 'Browse') +
        '

' +
        '' +
        '' +
        '
')
    );

    $this.appendTo('#' + id + '_input-group label:last-child div:last-child');

    var $path = $('#' + id + '_path', $parent).click(function() {
      $this.click();
    });

    $this.on('change.input_file_styler', function() {
      $path.val(this.files[0] ? this.files[0].name : '');
    });
  });
});

<link href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" rel="stylesheet" type="text/css" />

<input type="file" id="file" data-label="Select file to upload:" data-button-label="...">


Different ways of creating a multiplication grid in ruby The idea is to create a function that gets a number passed in and ouputs a multiplication grid.

For example the desired output for grid(10) would be :

1,2,3,4,5,6,7,8,9,10
2,4,6,8,10,12,14,16,18,20
3,6,9,12,15,18,21,24,27,30
4,8,12,16,20,24,28,32,36,40
5,10,15,20,25,30,35,40,45,50
6,12,18,24,30,36,42,48,54,60
7,14,21,28,35,42,49,56,63,70
8,16,24,32,40,48,56,64,72,80
9,18,27,36,45,54,63,72,81,90
10,20,30,40,50,60,70,80,90,100

I've come up with two variations of the same solution:

Variation 1

def grid1(grid_size)
  output = String.new
  for y in 1..grid_size
    line = Array.new
    for x in 1..grid_size
      line << x * y
    end
    output += "#{line.join(",")}\n"
  end

  output
end

Variation 2

def grid(grid_size)
  output = (1..grid_size).to_a.map do |y|
    (1..grid_size).to_a.map{|x| x*y}.join(",")
  end

  output
end

Any other ideas on how to solve this or ways that the current solutions could be improved would be most appreciated!

return from function anti-pattern

For the second example, doing

x = computation()
x

In a function is an anti-pattern, you should return the result directly:

computation()

It is more common and easier for the same result.

No Array

The first function does not need an array, just append to the string directly.


Tracking employee data We have hundreds of employees and for tracking each employees' data, and I am using this code:

<?php
$sqldelivery9 = "SELECT COUNT(*) as count FROM orders WHERE employeename = 'nawaz' AND DATE(reattemptdate) = DATE(NOW() - INTERVAL 2 DAY)";
$resultdeliverys9 = $db_handle->runSelectQuery($sqldelivery9);
$numrowsresultdelivery9 =$resultdeliverys9[0]['count'];
echo $numrowsresultdelivery9;

Also, I am tracking the last 7 days of records of each employee, so it's going to be 100*7= 700 SQL queries. Is this acceptable? Will it going to affect performance? Is there any better way to do it with minimal SQL queries?

Definitely not. You can do that all in a single query.

SELECT employeename, DATE(reattemptdate) as date, COUNT(*) as count
    FROM orders
    WHERE employeename in ('name1', 'name2', 'name3', '...')
        AND DATE(reattemptdate) > DATE(NOW() - INTERVAL 7 DAY)
    GROUP BY employeename, date

This will give you data in the form of

name1 | 2018-09-05 | 5
name1 | 2018-09-06 | 7
name1 | 2018-09-07 | 12
name1 | 2018-09-08 | 9
name1 | 2018-09-09 | 22
name2 | 2018-09-05 | 3
name2 | 2018-09-06 | 5
name2 | 2018-09-07 | 9
name2 | 2018-09-08 | 11
name2 | 2018-09-09 | 16
...

All you need is to iterate over the data.

Of course you can drop the employeename in ('name1', 'name2', 'name3', '...') condition if you want to search over ALL employees and not just "specific n" ones.


Caching a JS selector then only using it once I have this code:

var submit = document.getElementById('submit');
var error = document.getElementById('error');
var url;

submit.onclick = function(){
    url = document.getElementById('url').value;
    if(url == ''){
        error.style.display = 'block';
        return false;
    }
};

Now, it's very simple in functionality. But I'm wondering if caching the error selector, for example, is worth caching or not? is it a perf hit to cache a selector if its only accessed once?

My general rule of thumb is to cache a selector lookup within a given function so you only look it up once for each function invocation, but to not cache it globally.

If something was being called hundreds or thousands of times in a loop, you would want to cache it outside the loop just for common sense efficiency.

Caching things globally is pretty much never needed for user event types of things because the performance of looking up a few objects in the DOM is super fast when compared to any sort of user action time.  In other words, you wouldn't be able to tell the difference so you might as well go with the most maintainable and reliable code which avoids unnecessary globals.


Project Euler Problem 10. Sum of all primes < 2mil I'm new to coding so I can't create complex code yet. I do as much as I can with fors and ifs, since while I know a bit about arrays and other topics, I am not familiar enough with them to apply them on my own. This code works, but if I tell it to go to 2 million in main the program never finishes; 200,000 took about a minute, while 20,000 took a few seconds.

I get that this code is unoptimized and does too many unnecessary checks and calculations. Is there a way to optimize the program with just loops and conditions?

#include<iostream>
int nopf(int k){
    int sn=0,b;
    for(b=2;b<k;b++){
        if(k%b==0)sn++;
    }
    return sn;
}

main(){
    int i,s=0;
    for(i=2;i<200000;i++){
        if(nopf(i)==0)s+=i;
    }

    cout<<s;

}

NB: nopf is a function to check if a number is prime or not.

One of the fastest way to find primes is with a Sieve, which works like this:

(image courtesy of linked Wikipedia article)

All you have to do is create an array with 2 million elements:

#define MAX 2000000

// ...

bool* getSieve()
{
    bool isPrimeArray[] = new bool[MAX + 1];

Then you perform the sieve to it:

    for (int i = 2; i <= n; i++) {
        isPrimeArray[i] = true;
    }
    for (int i = 2; i * i <= n; i++) {
        if (isPrimeArray[i]) {
            for (int j = i; i * j <= n; j++) {
                isPrimeArray[i * j] = false;
            }
        }
    }

And sum it:

    int index = 0;
    long result = 0;
    for (boolean isPrime : isPrimeArray) {
        if (isPrime) {
            result += index;
        }
        index++;
    }
    return result;

Result:

#include <iostream>

#define MAX 2000000

bool* getSieve()
{
    bool isPrimeArray[] = new bool[MAX + 1];
    for (int i = 2; i <= n; i++) {
        isPrimeArray[i] = true;
    }
    for (int i = 2; i * i <= n; i++) {
        if (isPrimeArray[i]) {
            for (int j = i; i * j <= n; j++) {
                isPrimeArray[i * j] = false;
            }
        }
    }
}

int sumAllPrimes()
{
    bool* isPrimeArray = getSieve();
    int index = 0;
    long result = 0;
    for (boolean isPrime : isPrimeArray) {
        if (isPrime) {
            result += index;
        }
        index++;
    }
    return result;
}

int main()
{
    cout << sumAllPrimes();
}


Class or not in Page Scraper (Python BeautifulSoup) I am trying to figure out whether the below is better than just having a raw script or just a script with functions.
class WebsiteImages(object):

    def __init__(self,photographer,Website_url):
        self.photographer = photographer
        self.Website_url = Website_url

    def GetNumberOfResultPages(self):
        #Get the exact number of pages in the results
        from selenium import webdriver
        browser = webdriver.Firefox()
        browser.get(self.Website_url)
        last_number_page = browser.find_elements_by_xpath("//span[@class='search-pagination__last-page']")
        for i in last_number_page:
            number_of_pages = i.text
        return number_of_pages

    def GetImageIds(self):
        number_of_pages = self.GetNumberOfResultPages()
        Website_ids = []
        self.number_of_pages = number_of_pages
        #For each page get the image IDs
        import urllib
        from bs4 import BeautifulSoup
        import sys
        from time import sleep
        for page in range(1,int(number_of_pages)+1):
            #Extract the image id only and save it in file
            url = urllib.request.urlopen(self.Website_url+'&page='+str(page)+'&sort=best')
            sleep(1)
            content = url.read()
            soup = BeautifulSoup(content, 'lxml')
            #search on page for div class and extract the id between the gi-asset attribute
            images_found = soup.find_all('gi-asset') #gi-asset is the HTML object that contains the image and the id in the search results
            for i in range(len(images_found)):  #range(len(images_found))
                Website_id = images_found[i].get('data-asset-id') #data-asset-id' is the HTML object that contains the ID
                if Website_id not in 'placeholder':
                    Website_ids.append(Website_id)
        return Website_ids

# Define some photographers
john_smith = WebsiteImages('John%20Smith', 'https://www.WebsiteImages.co.uk/search/photographer?assettype=image&photographer=John%20smith')

# Now we can get to the image IDs easily
#print(john_smith.GetNumberOfResultPages())
print(john_smith.GetImageIds())

The idea of using the class was to make the script more organised and the outputs accessible for different search results. Example below:
one_guy = WebsiteImages('One%20Guy', 'https://www.WebsiteImages.co.uk/search/photographer?photographer=John%20smith')

two_guy = WebsiteImages('Two%20Guy', 'https://www.WebsiteImages.co.uk/search/photographer?photographer=John%20smith')

Class inheritance
Classes no longer have to be subclasses from object
class WebsiteImages:

Naming Conventions
Methods and variable names should be snake_case. Classes should be PascalCase, so you got that correct :-).
Imports
Any and all imports should go at the top of the file.
f"" strings
You should use f"" strings to directly implement variables into your strings.
url = urllib.request.urlopen(f"{self.website_url}&page={page}&sort=best")

User Interaction
The way a user has to interact with this program is pretty challenging to understand. If I was a normal user, I would do something like this:
person = WebsiteImages("John Smith", "https://www.WebsiteImages.co.uk")

But I would get an error, or no images returned because I didn't know the name had to be encoded with %20 and I didn't know I needed to specify a very particular url. I would blame it on the program and look for something else that's more user friendly.

I would solve this by only requiring a name. Since you're going to one website, you can save the url as an instance variable and sort things out there, instead of relying on a user to provide perfect input. Never trust user input.
john = WebsiteImages("John Smith")
print(john.get_image_ids())

And within the constructor you can do something like this:
def __init__(self, photographer: str):
    self.photographer = '%20'.join(photographer.split())
    self.website_url = f"https://www.WebsiteImages.co.uk/search/photographer?photographer={self.photographer}"


Caesar and Vigenère ciphers in Haskell the simple way There are two ciphers, Caesar and Vigenère, both with an encoder and a decoder. Both work with spaces and can be passed any case (the output will be always lowercased, though).
I'm a complete Haskell beginner and so my implementations are quite ugly and wordy. I'd welcome any help which would help me refactor the code, making it simpler, elegant and more readable.

I think it could be possible to more reuse and reduce this code. I can't think of any way to do that, though.

EDIT: Zeta's answer is great. I've refactored the following code according to his advice and added a new 'cipher' function which allows the user to create new ciphers. I raised a new question with the updated code, you can see it here.

import Data.Char (toLower, ord, chr)

caesar :: Int -> String -> String
caesar n s = unwords $ map (map chr) coded
  where
    coded = map (map helper) $ words s
    helper = (+) base . flip mod 26 . (+) n . flip (-) base . ord . toLower
    base = ord 'a'

unCaesar :: Int -> String -> String
unCaesar n s = unwords $ map (map chr) decoded
  where
    decoded = map (map helper) $ words s
    helper = (-) base . flip mod 26 . (+) n . (-) base . ord . toLower
    base = ord 'z'

I feel there should be a way to not handle the spaces like this, manually. Maybe zipping over a cycled key, somehow?

import Data.Char (toLower, ord, chr)

vigenere :: String -> String -> String
vigenere k s = unwords $ map (map chr) coded
  where
    coded = zipWith (zipWith helper) (words s) (words $ assign s 0)
    helper x y = base + mod (diff (toLower x) + diff (toLower y)) 26
    base = ord 'a'
    diff = flip (-) base . ord
    assign str i
        | null str = ""
        | head str == ' ' = ' ' : assign (tail str) i
        | otherwise = (k !! i) : assign (tail str) (mod (i + 1) (length k))

unVigenere :: String -> String -> String
unVigenere k s = unwords $ map (map chr) decoded
  where
    decoded = zipWith (zipWith helper) (words s) (words $ assign s 0)
    helper x y = base - mod (base - ord (toLower x) + diff (toLower y)) 26
    base = ord 'z'
    diff = flip (-) (ord 'a') . ord
    assign str i
        | null str = ""
        | head str == ' ' = ' ' : assign (tail str) i
        | otherwise = (k !! i) : assign (tail str) (mod (i + 1) (length k))

My issue with both your ciphers is that they don't preserve whitespace. Even for a lower-case only string, the following property does not hold:

unCaesar n (caesar n xs) == xs

Indeed, only the following property holds:

let xs' = unwords (words xs) in unCaesar n (caesar n xs') == xs'

But that might be your design, so let us ignore that for now. Instead, let us have a look at your first cipher.

Caesar

The nice property about Caesar is that you can encrypt the same way you decrypt. If you move a character n characters forward, how many characters do you need to move it to get back the original one? 26 - n.

With that in mind, we can heavily reduce the size of unCaesar:

unCaesar :: Int -> String -> String
unCaesar n = caesar (26 - n)

We could also use caesar (-n) since we're using mod, but that's not important. It would fail if we used rem, though.

Now that we've reused unCaesar, let us have a look at caesar:

caesar :: Int -> String -> String
caesar n s = unwords $ map (map chr) coded
  where
    coded = map (map helper) $ words s
    helper = (+) base . flip mod 26 . (+) n . flip (-) base . ord . toLower
    base = ord 'a'

Point-free programming isn't always best practices. It's clever, but you really don't want to fix something like that in the middle of the night. Compare your code to

caesar :: Int -> String -> String
caesar n s = unwords $ map (map chr) coded
  where
    coded    = map (map helper) $ words s
    helper c = (ord (toLower c) - base + n) `mod` 26  + base
    base     = ord 'a'

It is even shorter. It is easier to read than the point-free version, too. No flip. But if we want to preserve whitespace, it is still not optimal. Since we're handling a single character in helper either way, let us just keep spaces and handle all other characters:

caesar :: Int -> String -> String
caesar n = map helper
  where
    helper ' ' = ' '
    helper c   = chr $ (ord (toLower c) - base + n) `mod` 26  + base
    base       = ord 'a'

We've lost all applications of unwords and words, and instead of the map (map …) we only have a single map.

Being able to advance a lower-case ASCII character seems somewhat important, so let us refactor that:

caesar :: Int -> String -> String
caesar n = map helper
  where
    helper ' ' = ' '
    helper c   = asciiAdvance n c

asciiAdvance :: Int -> Char -> Char
asciiAdvance n c = chr $ (ord (toLower c) - base + n) `mod` 26  + base
  where
    base = ord 'a'

We will revisit Caesar later.

Vigenère

First of all, let us apply the point-free to non-pointfree conversion and use pattern-matching in assign:

vigenere :: String -> String -> String
vigenere k s = unwords $ map (map chr) coded
  where
    coded      = zipWith (zipWith helper) (words s) (words $ assign s 0)
    helper x y = base + mod (diff (toLower x) + diff (toLower y)) 26
    base       = ord 'a'
    diff c     = ord c - base

    assign ""     _ = ""
    assign (x:xs) i
        | x == ' '  = ' '      : assign xs i
        | otherwise = (k !! i) : assign xs (mod (i + 1) (length k))

Hm. assign just cycles through k and skips spaces. We can implement it without length and !! if we hand it two lists: our secret, and our cycled key:

vigenere k s = unwords $ map (map chr) coded
  where
    coded      = zipWith (zipWith helper) (words s) (words $ assign s (cylce k))
    …
    assign ""     _      = ""
    assign (x:xs) (y:ys)
        | x == ' '  = ' ' : assign xs (y:ys)
        | otherwise = y   : assign xs ys

But for a second, let us again say that you want to keep the whitespace. How would that look like?

vigenere :: String -> String -> String
vigenere k s = map chr coded
  where
    coded          = zipWith helper s (assign s (cycle k)
    base           = ord 'a'
    diff c         = ord c - base

    helper ' ' ' ' = ' '
    helper x   y   = base + mod (diff (toLower x) + diff (toLower y)) 26

    assign ""     _ = ""
    assign (x:xs) i
        | x == ' '  = ' '      : assign xs i
        | otherwise = (k !! i) : assign xs (mod (i + 1) (length k))

Hm. zipWith helper and assign have the same type. Maybe we can fuse them?

vigenere :: String -> String -> String
vigenere k s = map chr $ helper (cycle k) s
  where
    base   = ord 'a'
    diff c = ord c - base

    helper _      []     = []
    helper (y:ys) (x:xs)
      | x == ' '  = ' '          : helper xs (y:ys)
      | otherwise = modify x y   : helper xs ys

    modify x y = base + mod (diff (toLower x) + diff (toLower y)) 26

In order to share code between vigenere and unVigenere, we need one last step. Let us change modify's type to Char -> Char -> Char:

vigenere :: String -> String -> String
vigenere k s = helper (cycle k) s
  where
    base   = ord 'a'
    diff c = ord c - base

    helper _      []     = []
    helper (y:ys) (x:xs)
      | x == ' '  = ' '          : helper (y:ys) xs
      | otherwise = modify x y   : helper ys     xs

    modify x y = chr $ base + mod (diff (toLower x) + diff (toLower y)) 26

If we rewrite unVigenere the same way, we'll notice that it looks very similar:

unVigenere :: String -> String -> String
unVigenere k s = helper (cycle k) s
  where
    base   = ord 'a'
    diff c = ord c - base

    helper _      []     = []
    helper (y:ys) (x:xs)
      | x == ' '  = ' '          : helper (y:ys) xs
      | otherwise = modify x y   : helper ys     xs

    modify x y = chr $ base + mod (diff (toLower x) - diff (toLower y)) 26

Note that I've used the properties of mod again, just as in Caesar.

Everything is the same. Except for the modification. In unViginere, we subtract the key, and in vigenere we add it. So let us move that into another function:

withKey :: (Char -> Char -> Char) -> String -> String -> String
withKey f k s = helper (cycle k) s
  where
    helper _      []     = []
    helper (y:ys) (x:xs)
      | x == ' '  = ' '   : helper (y:ys) xs
      | otherwise = f y x : helper ys     xs

vigenere :: String -> String -> String
vigenere = withKey modify
  where
    modify k v = asciiAdvance (ord k - ord 'a') v

unVigenere :: String -> String -> String
unVigenere = withKey modify
  where
    modify k v = asciiAdvance (26 - (ord k - ord 'a')) v

And we're done.


RC4 implementation in C #include <stdio.h>
#include <stdint.h>
#include <assert.h>

typedef uint8_t byte;
typedef struct
{
    byte i, j;
    byte S[256];
} Rc4State;

void swap(byte *a, byte *b)
{
    byte temp = *a;
    *a = *b;
    *b = temp;
}

/*Initialization & initial permutation
also initialize i&j counters in state for stream generation*/
void initState(const byte K[256], int keylen, Rc4State *state)
{
    byte T[256];
    assert(keylen>=1 && keylen<=256);
    int i;
    for(i = 0; i < 256; i++) {
        state-> S[i] = i;
        T[i] = K[i % keylen];
    }

    //Initial permutation of S
    byte *S = state->S;
    int j = 0;
    for(i = 0; i < 256; i++) {
        j = (j + S[i] + T[i]) % 256;
        swap(&S[i], &S[j]);
    }

    //Initialize counters in state
    state->i = state->j = 0;
}

/*Encrypt/Decrypt text by XORing with next byte of keystream*/
byte crypt(byte text, Rc4State *state)
{
    byte t, k;
    byte *i = &(state->i), *j = &(state->j);
    byte *S = state->S;
    *i = (*i+1) % 256;
    *j = (*j+S[*i]) % 256;
    swap(&S[*i], &S[*j]);
    t = (S[*i] + S[*j]) % 256;
    k = S[t];

    return text ^ k;
}

I considered removing the %256 since it's implied in unsigned 8 bit arithmetic but left it in for clarity.

I asked this on SO.

Any comments would be appreciated.

From top to bottom…

#include <stdio.h> appears to be left over from development, and can be removed.
swap() is not part of your library's interface, and should be declared static.
To give your library a sense of unity, I'd rename initState() → rc4InitState() and crypt() → rc4Crypt().  The latter renaming is also important so as not to clash with the traditional Unix crypt(3) function.  Also for consistency, I'd make Rc4State *state the first parameter of each function.
Assertions are not really an appropriate mechanism for parameter validation.  The use of assert() in initState() is marginal.  You should assert conditions that you know to be true, rather than conditions that you hope to be true.  Alternatively stated, assertion failures should arise from programmer errors, not user errors.

Change int keylen to uint8_t keylen, and most of the validation problem goes away anyway.  (In C, i % 0 has undefined behaviour.)
On the other hand, you could add an assert((byte)(S[i] + 256) == S[i]) to put to rest your concerns about overflow.
Calling crypt() to encrypt a byte at a time is inefficient.  The function should accept a byte array and length.  A compiler might inline a function, but if you're writing this as a library, the linker can't optimize away a function call.
*i and *j are a bit unconventional for my taste.  Here's how I would express it:

static byte rc4CryptByte(Rc4State *state, byte plainText)
{
    byte *S = state->S;
    byte i = ++(state->i);
    byte j = (state->j += S[i]);

    swap(&S[i], &S[j]);
    byte t = S[i] + S[j];
    byte k = S[t];

    return plainText ^ k;
}

void rc4Crypt(Rc4State *state, byte text[], size_t len)
{
    for (size_t i = 0; i < len; i++)
    {
        text[i] = rc4CryptByte(state, text[i]);
    }
}


Rename files to titlecased I wrote this Python script to rename specified files to "titlecase", for example:

hello.txt to Hello.txt
hello there.txt to Hello There.txt
path/to/file.txt to path/to/File.txt

The main script:

#!/usr/bin/env python

import os
import re
import argparse

re_junk = re.compile(r'[._-]')
re_spaces = re.compile(r'\s\s+')

def print_rename(old_filename, new_filename):
    print('{} -> {}'.format(old_filename, new_filename))

def do_rename(old_path, new_path):
    print_rename(old_path, new_path)
    os.rename(old_path, new_path)

def get_new_path(old_path):
    """ Get the new path, titlecased and (a little bit) sanitized.
    - Only operate on the basename:
        + don't touch parent directories
        + don't touch the extension
    - Sanitize:
        + replace junk characters with space
        + replace multiple spaces with single space
        + trim extra spaces at start and end

    :param old_path: the path to rename
    :return: titlecased and a little bit sanitized new path
    """
    dirpart, filepart = os.path.split(old_path)
    if filepart.startswith('.'):
        return old_path

    base, ext = os.path.splitext(filepart)
    base = re_junk.sub(' ', base)
    base = re_spaces.sub(' ', base).strip()
    if not base:
        return old_path

    return os.path.join(dirpart, base.title() + ext)

def titlecase(old_path, fun):
    if not os.path.exists(old_path):
        return

    new_path = get_new_path(old_path)
    if old_path == new_path:
        return

    fun(old_path, new_path)

def main():
    parser = argparse.ArgumentParser(description='Rename files to "titlecased" and "sanitized".')
    parser.add_argument('-n', '--dry-run', action='store_true', help='Print what would happen, don\'t rename')
    parser.add_argument('paths', nargs='+')

    args = parser.parse_args()
    fun = print_rename if args.dry_run else do_rename

    for path in args.paths:
        titlecase(path, fun)

if __name__ == '__main__':
    main()

Unit tests:

#!/usr/bin/env python

import unittest

from titlecase import get_new_path

class TestTitleCase(unittest.TestCase):
    def test_hello(self):
        self.assertEqual('Hello', get_new_path('hello'))

    def test_hello_txt(self):
        self.assertEqual('Hello.txt', get_new_path('hello.txt'))

    def test_hello_there_txt_dotted(self):
        self.assertEqual('Hello There.txt', get_new_path('hello.there.txt'))

    def test_hello_there(self):
        self.assertEqual('Hello There', get_new_path('hello there'))

    def test_hello__there(self):
        self.assertEqual('Hello There', get_new_path('hello  there'))

    def test_dot(self):
        self.assertEqual('.', get_new_path('.'))

    def test_dotdot(self):
        self.assertEqual('..', get_new_path('..'))

    def test_subdir(self):
        self.assertEqual('some/path/File.txt', get_new_path('some/path/file.txt'))

    def test_subdir_nobasename(self):
        self.assertEqual('some/path/.txt', get_new_path('some/path/.txt'))

    def test_subdir_dotdot(self):
        self.assertEqual('../path/File.txt', get_new_path('../path/file.txt'))

    def test_subdir_dotdot_in_middle(self):
        self.assertEqual('some/../path/File.txt', get_new_path('some/../path/file.txt'))

if __name__ == '__main__':
    unittest.main()

Is there a better way I missed? Anything to improve?
Better names maybe? Interesting unit test cases I missed?

(This script (along with some other utility scripts) is on GitHub.)

I strongly oppose testing for file existence. It introduces a race condition: the path may exist at the time of test and disappear by the time of use; yes I am paranoid. Besides, it really impedes dry runs/unit testing - to try an interesting case you need to actually create a file. The pythonic way is to ask forgiveness, not permission:

try:
    os.rename(old_path, new_path)
except OSError:
    pass # or an error message

Notice that it would also take care of other failures of os.rename, such as new_path already exists or is too long; the directory is read-only, etc.
I am also not sure that "sanitization" is to be done unconditionally. A command line option would be nice.
Typically a debugging/diagnostic messages are sent to stderr. A quiet (or verbose) option is also desirable.
fun doesn't strike as a particularly good name.


Smallest element in an array that is repeated exactly ‘k’ times This is a problem from GeeksForGeeks and I have tried to solve it on my own and I have written this code.

  Given an array of size n, the goal is to find out the smallest number
  that is repeated exactly ‘k’ times where k > 0? Assume that array has
  only positive integers and 1 <= arr[i] < 1000 for each i = 0 to n -1.

  Examples:

  Input:

arr[] = {2 2 1 3 1}
k = 2

  Output: 1

  Explanation:

  Here in array:

  2 is repeated 2 times
  1 is repeated 2 times
  3 is repeated 1 time

  Hence 2 and 1 both are repeated 'k' times

  i.e 2 and min(2, 1) is 1

  Input:

arr[] = {3 5 3 2}
k = 1

  Output: 2

  Explanation:

  Both 2 and 5 are repeating 1 time but min(5, 2) is 2

How can I improve this code?

#include <iostream>
#include <vector>
#include <map>

template<class T>
void findElement(std::vector<T>& vec, int k)
{
  std::map<T, int> count;
  for(T x : vec)
  {
    count[x]++;
  }

  typename std::map<T, int>::iterator itr;
  for(itr = count.begin(); itr != count.end(); itr++)
  {
    if(itr->second == k)
    {
      std::cout << itr->first <<'\n';
      return;
    }
  }
  std::cerr << "No such element \n ";
}

int main()
{
  std::vector<char> v;
  v.push_back('r');
  v.push_back('t');
  v.push_back('q');
  v.push_back('r');
  v.push_back('u');
  v.push_back('q');
  v.push_back('s');
  int k;
  std::cout << " Enter the number of repetitions you want : ";
  std::cin >> k;
  std::cout << "The smallest element that has " << k <<" reptition is : ";
  findElement(v, k);
}

Interface

There's no point in limiting your function inputs to a vector. You can take two input iterators instead. It'll make your function more flexible and more "standard library-like".
One function should do one focused thing. Your function finds such an element  and  prints it as the same time. These concerns are unrelated. You can return std::optional<T> instead to represent either such an element or its absence. Note: it works for C++17 only. You can use boost::optional or return a smart pointer if it's not available.

Modern C++

There's no need for things like this anymore:

typename std::map<T, int>::iterator itr;
for(itr = count.begin(); itr != count.end(); itr++)

for (const auto& kv : count) is much better, isn't it?
You can also use initialization list to create a vector with the elements you need: std::vector<char> v({'r', 't',..., 's'})

Performance

for (T x : vec) creates an unnecessary copy. Use for (const T& x : vec) or for (const auto& T : vec) instead.
You can also use std::unordered_map to do the counting and then choose the smallest element (it's more efficient because one can find the minimum in O(n). It's "easier" than sorting the input). One caveat: you need a hash function for T in this case. You can pass it as another template parameter, defaulted to std::hash.


Stack Implementation Using Linked Lists I have tried Implementing Stacks Using Linked Lists, and would like some tips and tricks regarding optimizations or ways to simplify/compact processes.

It has 3 functions, Push, Pop, and Peek (For checking the top item in stacks)

class Node:
    def __init__(self, data, next=None):
        self.data = data
        self.next = next

class Stack:
    def __init__(self):
        self.bottom = None
        self.top = None
        self.length = 0

    def peek(self):
        return self.top.data if self.top != None else None

    def push(self, data):
        NewNode = Node(data)
        if self.length == 0:
            self.bottom = self.top = NewNode
        else:
            top = self.top
            self.top = NewNode
            self.top.next = top
        self.length += 1
        return self

    def pop(self):
        length = self.length
        top = self.top
        if length == 0:
            raise IndexError('No items in stack')
        elif self.bottom == top:
            self.bottom = None
        else:
            NextTop = self.top.next
            self.top = NextTop
        self.length -= 1
        return top.data

Output:

stack = Stack()

stack.push(0)
stack.push(1)
stack.push(2)
stack.push(3)

print(stack.peek())

print(stack.pop())
print(stack.pop())
print(stack.pop())
print(stack.pop())

stack.push(5)
stack.push(2)
print(stack.pop())
print(stack.pop())

-----------------------
3
3
2
1
0
2
5
-----------------------

First, I believe that the Node class is an implementation detail. You could move it inside the Stack or your could rename it _Node to indicate that it is private.

Next, I will refer you to this answer to a different CR question, also written by me: https://codereview.stackexchange.com/a/185052/106818

Specifically, points 2-7:

... consider how the Python list class (and set, and dict, and tuple, and ...) is initialized. And how the Mutable Sequence Types are expected to work.

Because your code is implementing a "mutable sequence type." So there's no reason that your code shouldn't work the same way. In fact, if you want other people to use your code, you should try to produce as few surprises as possible. Conforming to an existing interface is a good way to do that!
Create an initializer that takes a sequence.

class Stack:
    def __init__(self, seq=None):
        ...
        if seq is not None:
            self.extend(sequence)

Implement as many of the mutable sequence operations as possible.
Use the standard method names where possible: clear, extend, append, remove, etc.
Implement special dundermethods (method names with "double-underscores" in them: double-under-methods, or "dundermethods") as needed to make standard Python idioms work:

def __contains__(self, item):
    for i in self:
        ...

def __iter__(self):
    node = self.head

    while node:
        yield node.value
        node = node.next

Implement your test code using standard Python idioms, to prove it's working and to show developers how your code should be used!

Finally, some direct code criticisms:

Don't use equality comparisons with None. Use is None or is not None instead. This is a PEP-8-ism, and also actually faster.
You don't really use self.bottom for anything. Go ahead and delete it.
Don't use CamelCase variable names. That's another PEP-8 violation. Use snake_case for local variables.


Class for Value + Error Code This is a class similar to std::pair and std::tuple inspired by Alexandresku's Expected<T>. Let me know if this class can be helpful for simple error handling.

template<class E, class T>
class CheckValue{
public:
    template<class UT>
    CheckValue(const E &e, UT &&t) : e(e), t(std::forward<UT>(t)){}

    operator E() const{
        return e;
    }

    T &get(){
        return t;
    }

    const T &get() const{
        return t;
    }

private:
    E e;
    T t;
};

... and here is some usage. I did usage with bool first, but deliberately redesign it to work with enum class to show full power.

enum class CalcError{
    OK,
    ERROR,
    DIV_BY_ZERO
};

CheckValue<CalcError, float> calc(float a, float b){
    if (b == 0)
        return { CalcError::DIV_BY_ZERO, 0 };

    return { CalcError::OK, a / b };
}

void prn(float a, float b){
    switch( auto val = calc(a, b) ){
    case CalcError::DIV_BY_ZERO:
        std::cout << "Division by zero" << std::endl;
        break;

    case CalcError::ERROR:
        std::cout << "Error"        << std::endl;
        break;

    default:
    case CalcError::OK:
        std::cout << val.get()      << std::endl;
        break;
    }
}

int main(){
    prn(5, 2);
    prn(5, 0);
}

The code looks quite good to me in general. Here are a few remarks.

Avoid implicit conversion operators

The implicit conversion operator

template <class E, class T>
CheckValue<E, T>::operator E() const;

might do you more harm than good. In particular, if both E and T are of arithmetic type, forgetting to call get will happily use the error code instead of the value and you'll probably have a bad time tracing down this bug.

Maybe a plain old function would serve you better.

template <class E, class T>
E
CheckValue<E, T>::status() const;

Take sink arguments by-value

It seems to me that you expect E to be some cheaply copyable value type, like an enum or an integer so you can easily afford always making a copy. That's perfectly reasonable but then I'm surprised why you take it by const reference. Just take it by-value instead. If you're not so sure whether it will always be cheap to copy, apply std::move to it instead. It should never cost you anything and might sometimes benefit you.

template <class UT>
CheckValue(E, UT&& t) : e(std::move(e)), t(std::forward<UT>(t)) {}

Of course, you could also decide to take both parameters by forwarding reference, just in case.

template <class UE, class UT>
CheckValue(UE&& e, UT&& t) : e(std::forward<UE>(e)), t(std::forward<UT>(t)) {}

Consider special-casing the “success” case

Currently, your class does not know any semantics about E.  If its intention is to be used as “success or failure” indicator, you could make this explicit. This would also allow you to omit the error code when there actually is no error.

For example, you could add a third template parameter that indicates the “success” value (note that I have re-ordered the parameters).

template <class ValueT, class StatusT, StatusT Good = static_cast<StatusT>(0)>
class CheckValue;

Then you could offer a single-argument constructor

template <class ValueT, class StatusT, StatusT Good>
template <class V>
CheckValue<ValueT, StatusT, Good>::CheckValue(V&& v)
    : e(Good), t(std::forward<V>(v)) {}

and your happy path would be just return 42;.

Note that I have deliberately not marked the single-argument constructor as explicit in this case. Otherwise, you'd have to write return {42};. Which version you prefer is probably a matter of taste.

If your class knows about the distinct “success” case, you can add a check for it.

template <class ValueT, class StatusT, StatusT Good>
explicit
CheckValue::operator bool() const noexcept
{
  return (this->e == Good);
}

This would allow code like this.

if (const auto cv = prn(5, 2))
    std::cout << cv.get() << '\n';
else
    std::cerr << "error: " << to_string(cv.status()) << '\n';

Instead of a single Good value, you could have a more general predicate but this would deprive you of the ability to default it in the lucky case and smells like over-engineering to me.

Think whether you want a “no value” state

If your computation fails, you currently have to pass a dummy value to the constructor because it always initializes the t member. If an object of type T is expensive to construct, this would be wasteful. It also feels bad having to specify a value when you actually have none.  Of course, you also don't want to default-construct the t member in this case as this might be equally costly or not even possible.

The usual way to handle this is to use a properly aligned buffer instead of a typed variable T.

char buffer[sizeof(ValueT)] alignas(ValueT);

In your constructor, you use placement-new to create an object inside the buffer, if you have one.

template <class ValueT, class StatusT, StatusT Good>
template <class V>
CheckValue<ValueT, StatusT, Good>::CheckValue(V&& v) : e(Good)
{
  new (this->buffer) ValueT(std::forward<V>(v));
}

Your get function would then cast the buffer to a pointer of the appropriate type, after optionally checking that there is a value in it.

template <class ValueT, class StatusT, StatusT Good>
ValueT&
CheckValue<ValueT, StatusT, Good>::get()
{
  assert(this->e == Good);
  return *reinterpret_cast<ValueT *>(this->buffer);
}

Don't forget to invoke the destructor, too.

template <class ValueT, class StatusT, StatusT Good>
CheckValue<ValueT, StatusT, Good>::~CheckValue()
{
  if (this->e == Good)
    reinterpret_cast<ValueT *>(this->buffer)->~ValueT();
}

You would remove the two-argument constructor now. In order to construct an object in an error-state, you'll need a static factory function.

template <class ValueT, class StatusT, StatusT Good>
CheckValue<ValueT, StatusT, Good>
CheckValue<ValueT, StatusT, Good>::make_failure(StatusT e)
{
  assert(e != Good);
  CheckValue bad {};  // use private default-constructor
  bad.e = e;
  return bad;
}

The private default constructor does not initialize the object.

template <class ValueT, class StatusT, StatusT Good>
CheckValue<ValueT, StatusT, Good>::CheckValue() noexcept {}

It must never be called without setting e to anything but Good immediately afterwards.  (Or the destructor and the other functions would invoke undefined behavior.)  Therefore, it is essential that this dangerous constructor is private.

You will also need to write custom copy / move constructors and assignment operators. You might want to implement them in terms of a custom swap overload.

Consider restricting the type of the error code

Since the code makes a few assumptions about the type of the error code, you might as well make these explicit and restrict it to POD types. To do this, simply put

static_assert(std::is_pod<StatusT>::value, "Status code must be POD type");

at the top of your class definition.

Apply noexcept where appropriate

If a function has no pre-conditions and cannot fail, declare it as noexcept.

Consider using more descriptive names

Some of your variable names seem overly terse to me. As you could see in the example code I've written above, I'd strongly consider renaming t to value, e to status and so forth.

I also recommend that you establish a convention of naming (private) member variables specially. Personally, I like suffixing them with an underscore (eg value_); others prefer the m_ or M_ prefix (eg m_value). Pick what you like and use it consistently.

Use nouns for type names

An object represents a thing, not an action. A type represents a class of things. Therefore, it is generally accepted that type names should be nouns. In your case, CheckedValue would be preferred instead of CheckValue.