Organizing museum artwork I have a small Django project to help organize museum artwork. In my models.py file, my museum object is defined as follows:

class Museum(models.Model):
    """Model representation of an art museum"""
    title = models.CharField(max_length=50)
    website = models.URLField()
    description = models.TextField()
    image = models.ForeignKey(AppImage, blank=True, null=True)
    longitude = models.FloatField()
    latitude = models.FloatField()

    @property
    def marker_info(self):
        """Get information accessible for google map"""
        return { "title": self.title, "website" : self.website, "description" : self.description,
            "latitude" : self.latitude, "longitude" : self.longitude, "id" : self.id }

    @property
    def artwork(self):
        """Get the museum's artwork"""
        return Artwork.objects.filter(museum__id=self.id)

    @property
    def artists(self):
        """Get the museums' unique artists"""
        return { artwork.artist.id : artwork.artist for artwork in self.artwork }.values()

    @property
    def count(self):
        """Get the museum's number of artworks"""
        return len(self.artwork)

    @property
    def nearby(self):
        """Get neighboring museums by distance"""
        return sorted(Museum.objects.all().exclude(id__exact=self.id), key=lambda x: self.distance(x))

    def distance(self, other):
        """ Compute the Euclidean distance between
            this and another museum """
        def cartesian(obj, r=3959): # mean radius of earth in miles
            lon = math.radians(obj.longitude)
            lat = math.radians(obj.latitude)
            x = r * math.cos(lat) * math.cos(lon)
            y = r * math.cos(lat) * math.sin(lon)
            z = r * math.sin(lat)
            return x, y, z

        x1, y1, z1 = cartesian(self)
        x2, y2, z2 = cartesian(other)
        return math.sqrt((x1 - x2)**2 + (y1 - y2)**2 + (z1 - z2)**2)

    def __str__(self):
        return '%s, %s' % (self.title, self.website)

Since most of the business logic for display is stored in the model, my views are pretty clean:

def museum(request):
    return render(request, "museum.html",
        { "museum" : center(request) })

def nearby(request):
    return render(request, "tabs/nearby.html",
        { "nearby" : center(request).nearby })

def artists(request):
    museum = center(request)
    return render(request, "tabs/artists.html",
        { "artists" : [(artist, artist.count(museum)) for artist in museum.artists] })

def gallery(request):
    return render(request, "tabs/gallery.html",
        { "artworks" : center(request).artwork })

def center(request):
    return Museum.objects.get(pk=request.GET['id'])

With the MVVM pattern, we would put these functions in a view model class rather than in a model class. Coming from this perspective, I'm concerned that my business logic is crowding and polluting the purity of my model. Is this a valid concern? Is there a better place I should be putting these properties and business logic functions?

Django's philosophy is Thin views, Fat models. This may be true for small applications, as application grows, it becomes extremely complex to manage all business actions in model layer.

One of the suggested approach is to have a separate layer to handle business actions. This can be similar to view, name it orchestrator or business_actions.

The responsibility of the layer is to do one business action which can touch multiple models. Say, register a new client means creating new user, new client, send notification, sync to external system. Having all these in multiple models is cumbersome.

The business action layer can be modelled as class or simple function which takes data and does all the relevant work.

Here is an example todo application which is designed to address these concerns.


Lowest Common Ancestor here is my implementation to find the lowest common ancestor in a binary tree. It works but i would like to know if i could have done anything better or if i have missed an specific case. would really appreciate ur feedback:

//The function findOrQueue is to enqueue all elements upto target node to a queue
public void findOrQueue(Node target, Node top, LLQueue q) {
    int cmp = target.getData().compareTo(top.getData());
    if(cmp == 0) {
        q.enqueue(top);
        return ;
    }
    else if (cmp < 0) {
        q.enqueue(top);
        findOrQueue(target, top.getLeftChild(),q);
    }
    else {
        q.enqueue(top);
        findOrQueue(target, top.getRightChild(),q);
    }
}

public Node LCA(Node n1, Node n2) throws QueueEmptyException {
    LLQueue q1 = new LLQueue();
    LLQueue q2 = new LLQueue();
    findOrQueue(n1,getRoot(),q1);
    findOrQueue(n2,getRoot(),q2);
    Node t = null;
    while (!q1.isEmpty() && !q2.isEmpty()) {
        Node t1 = (Node)q1.dequeue();
        Node t2 = (Node)q2.dequeue();
        if(t1.getData() != t2.getData()) {
            return t;
        }
        else t = t1;
    }
    if(q1.isEmpty() && q2.isEmpty())
        return null;
    else
        return t;
}

I would simplify the findOrQueue function to make sure it stays DRY

public void findOrQueue(Node target, Node top, LLQueue q) {
    int cmp = target.getData().compareTo(top.getData());
    q.enqueue(top);
    if(cmp != 0) {
        Node nextTop = (cmp < 0) ? top.getLeftChild() : top.getRightChild();
        findOrQueue(target, nextTop, q);
    }
}

Since it's used recursively, it might not be a horrible idea to keep the exit condition explicit for readability:

if(cmp == 0) {
    return; // Exit condition
} else {
    // ... as above
}

Edit: The return condition can also be made more direct:

boolean bothEmpty = q1.isEmpty() && q2.isEmpty();
return (!bothEmpty) ? t : null;


interpre function implementation Here is the description of the problem.

  Write a simple interpreter which understands "+", "-", and "*" operations. Apply the operations in order using command/arg pairs starting with the initial value of value. If you encounter an unknown command, return -1.

  Example:
  interpret(1, ["+"], [1]) → 2
  interpret(4, ["-"], [2]) → 2
  interpret(1, ["+", "*"], [1, 3]) → 6

I'm trying to implement this function using recursion. Here is my implementation.

public int interpret(int value, String[] commands, int[] args) {
  // base case
  if (commands.length == 0 && args.length == 0) {
    return value;
  }

  // Get top command and arg to calculate value
  final String cmd = commands[0];
  final int arg = args[0];

  // Construct remaining commands and args for recursion
  final int[] remainArgs
          = Arrays.copyOfRange(args, 1, args.length);
  final String[] remainCommands
          = Arrays.copyOfRange(commands, 1, commands.length);

  switch (cmd) {
    case "*":
      return interpret(value * arg, remainCommands, remainArgs);
    case "+":
      return interpret(value + arg, remainCommands, remainArgs);
    case "-":
      return interpret(value - arg, remainCommands, remainArgs);
    default:
      // unknown command return -1
      return -1;
  }
}

Please comment on my implementation in regarding to code readability and efficiency. Please also share your implementation if you have a better idea. Thanks.

You don't need to use recursion here,  commands and args array have the same size you can simply loop through it.

long result=value;
for (int i=0; i<commands.length;i++) {
   result = calc(result, commands[i], args[i]);
}

And you need to check size of arrays before use, to prevent ArrayIndexOutOfBoundsException

if (commands.length != args.length)
    return -1;

P.S. You work with int, but result can be larger than Integer.MAX_VALUE, therefore use long type for result


Extract incomplete weeks from a DateTime collection The code below attempts to extract incomplete weeks from a List<DateTime>.

For example, a list containing all the days in Jan 2015 would result in the 5th to the 25th inclusive.

I know the list going in will contain unique dates, and are in date order. It seems to be working but I can't help but think this could be done better.

private IEnumerable<DateTime> extractIncompleteWeeks(IEnumerable<DateTime> dates)
{
    var mondays = dates
        .Where((d, i) => d.DayOfWeek == DayOfWeek.Monday)
        .Select(d => d.Date);

    var results = new List<DateTime>();

    foreach (var monday in mondays)
    {
        var fullWeek = new HashSet<DateTime>(dates.SkipWhile(d => d < monday).Take(7).Select(d => d.Date));

        if (fullWeek.Last().Date == monday.AddDays(6).Date)
            results.AddRange(fullWeek);
    }

    return results;
}

Naming:

Names of methods in C# use PascalCasing. Your method name would become ExtractIncompleteWeeks. Although the naming convention is now correct, the name is ambiguous. Extract incomplete weeks means you want to fetch only the dates of the weeks which are incomplete. Instead rename it to ChopIncompleteWeeks or ExtractCompleteWeeks. This makes clear that you want to eliminate the incomplete weeks or want to extract the complete weeks.

Return type:

The return type of the method is an IEnumerable<DateTime> yet you return a List<DateTime>. Either make the return type also List<DateTime> but preferably return one element at a time by using yield.

private IEnumerable<DateTime> ExtractCompleteWeeks(IEnumerable<DateTime> dates)
{
    ...

    foreach (var monday in mondays)
    {
        ...

        if (fullWeek.Last().Date == monday.AddDays(6).Date)
        {
            foreach(var day in fullWeek)
            {
                yield return day;
            }
        }
    }
}

Here are some questions from StackOverflow and Programmers.SE regarding yield:

Proper Use of yield return
A practical use of “yield” keyword in C#

I really like this answer from the first question to get a very basic understanding of using yield instead of a temporary list:

  Populating a temporary list is like downloading the whole video, whereas using yield is like streaming that video.

HashSet:

Is there a specific reason why you create a HashSet<DateTime> only to take the last element and compare it against monday.AddDays(6).Date? You can replace that line by following:

var fullWeek = dates.SkipWhile(d => d < monday).Take(7).Select(d => d.Date);

Complete code:

private IEnumerable<DateTime> ExtractCompleteWeeks(IEnumerable<DateTime> dates)
{
    var mondays = dates.Where(d => d.DayOfWeek == DayOfWeek.Monday)
                       .Select(d => d.Date);

    foreach (var monday in mondays)
    {
        var fullWeek = dates.SkipWhile(d => d < monday).Take(7).Select(d => d.Date);

        if (fullWeek.Last().Date == monday.AddDays(6).Date)
        {
            foreach(var day in fullWeek)
            {
                yield return day;
            }
        }
    }
}


Any better pagination logic? router/article.js
// =============================== GET / WITH PAGINATION ==============================

router.get('/?:page', async (req, res) => {
  const { page } = req.params; // current Page
  const { size } = req.params; // items per page
  const limit = size || 5; // if size isn't specified, set size to 5
  const offset = page ? (page - 1) * limit : 0; // skip previous items
  const navBarIndexLimit = 4; // how many page indexes to show in navigation bar like < 1 2 3 4 >
  const navBarIndexOffset = Math.floor(page / navBarIndexLimit); // offset previous indexes like < 5 6 7 8 >
  const navBarIndexArray = []; // index for navigating pages in view

  // fetch articles
  const articles = await Article.findAndCountAll({
    include: User,
    raw: true,
    nest: true,
    limit,
    offset,
  });
  const totalPage = Math.ceil(articles.count / limit);

  // if at last page, show previous indexes
  if (page >= totalPage) {
    for (let i = 0; i < navBarIndexLimit; i += 1) {
      const prevPage = page - i;
      navBarIndexArray.push(page - i);
      if (prevPage <= 0) {
        break;
      }
    }
    navBarIndexArray.sort();
  }

  // set index bar
  else {
    for (let i = 0; i < navBarIndexLimit; i += 1) {
      const nextPage = i + navBarIndexOffset * navBarIndexLimit;
      if (nextPage > totalPage) {
        break;
      }
      navBarIndexArray.push(nextPage + 1);
    }
  }

  articles.index = navBarIndexArray;
  res.render('article/article_home.hbs', { articles });
});

article.home.hbs

    <-

  {{#each articles.index as |key|}}

    {{key}}

  {{/each}}

    ->

I've managed to paginate my articles and while it is working, I have a strong feeling that I made the pagination navigation bar part extremely complicated and I'm sure there would be a better way.
Using Express, Sequelize and Handlebars

Error handling You await inside an async function, but without a try/catch surrounding it. So, if Article.findAndCountAll rejects, you'll get an unhandled rejection (which is deprecated in Node) and no response will be sent.
Default destructuring and variable names
const { page } = req.params; // current Page
const { size } = req.params; // items per page
const limit = size || 5;

simplifies to
const { page, size = 5 } = req.params;

and using the size variable. Though, if possible, change the param and variable name to itemsPerPage; it's much more representative of what the variable contains than size. currentPage would also be a bit more precise. Similarly, totalPage, since it contains a number, would sound better as totalPages.
Sorting bug You have:
navBarIndexArray.sort();

This will sort lexiographically, not numerically. For example, it could produce an output of
[1, 11, 2]

Compare numerically instead, eg .sort((a, b) => a - b).
Pagination Or, even better, just insert the indicies in order to begin with.
Your current results don't seem quite natural. When near the end, I'd expect the nav to be full of all nearby pages: eg, when on page 5 of 6, I'd expect 3 4 5 6, not just 4 5 6. Implementing this will require a bit of extra logic, though.
Rather than two loops which do something very similar, identify the last page to display in the nav, then subtract by navBarIndexLimit to identify the first page to display in the nav, then iterate over it. See comments in snippet below:

const navBarIndexLimit = 4;
// currentToLastPageOffset: Amount to add to current page to get last page
// (eg: current page 4 results in last page 6, first page 3)
const currentToLastPageOffset = navBarIndexLimit - 2;
const makeNavbarIndexArr = (currentPage, totalPages) => {
  const lastPage = Math.min(
    totalPages,
    currentPage <= 1
      ? navBarIndexLimit - 1 // If on 0th or 1st page, set last page to 3
      : currentPage + currentToLastPageOffset
  );
  const firstPage = Math.max(lastPage - (navBarIndexLimit- 1), 0);
  // I prefer to create numeric arrays all at once with Array.from:
  return Array.from(
    { length: lastPage - firstPage + 1 },
    (_, i) => firstPage + i
  );
  // But you could also use a for loop and push if you find it easier to read
};
console.log(makeNavbarIndexArr(0, 9));
console.log(makeNavbarIndexArr(1, 9));
console.log(makeNavbarIndexArr(5, 9));
console.log(makeNavbarIndexArr(8, 9));
console.log(makeNavbarIndexArr(9, 9));
.as-console-wrapper {
  max-height: 100% !important;
}

Note that the above takes zero-indexed parameters and returns an array that's zero-indexed, though it's trivial to tweak it to be one-indexed instead if needed.
It's a reasonable amount of code, but the logic that needs to be implemented is somewhat complicated, so there's no avoiding at least some of it. While it could be condensed somewhat (for example, by removing the currentToLastPageOffset variable), that would make it harder to understand.
I highly recommend keeping the function in the snippet above - don't inline it into your page route, so as to clearly separate the calculation logic from the main route logic.
router.get('/?:page', (req, res) => {
    const { currentPage, itemsPerPage = 5 } = req.params;
    const offset = currentPage ? (currentPage - 1) * itemsPerPage : 0; // skip previous items
    Article.findAndCountAll({
        include: User,
        raw: true,
        nest: true,
        limit: itemsPerPage,
        offset,
    })
        .then((articles) => {
            const totalPages = Math.ceil(articles.count / itemsPerPage);
            articles.index = makeNavbarIndexArr(currentPage, totalPages);
            res.render('article/article_home.hbs', { articles });
        })
        .catch((error) => {
            // Handle errors, send error response
        });
});


Large switch statement bucketing system I've finally got a solution for a "weekly streak" bucketing system I've been working on but unfortunately it's resulted in a very large switch statement.

I'm running through a handful of dates and determining by a range of seconds (the length of a week) which bucket that date belongs to.

It's obvious to me that this could be refactored in a programmatic way because of the amount of repetition but I'm not sure where to start. Dictionary? Some kind of sorting algorithm? Recursion?

open func weeklyStreakCount(weeklyGoal target: Int) -> Int {
    let endDate = Date()
    let startDate = endDate.startOfWeek!
    let startDateInterval = Double(startDate.timeIntervalSinceNow)

    var workoutsPerWeek = [Int: Int]()
    let userWorkouts: [UserWorkoutEntity] = self.userWorkouts(completed: true)

    var numberOfGoodBuckets = 0

    for i in 0...100 {
        workoutsPerWeek.updateValue(0, forKey: i)
    }

    // calculate the time from now to seconds in a week and round to the nearest hundreths to create a bucket for that week
    for userWorkout in userWorkouts {
        let workoutTimeInterval = Double((userWorkout.completionDate?.timeIntervalSinceNow)!)
        let rawBucket = (startDateInterval - workoutTimeInterval) / numberOfSecondsInAWeek

        let bucket = Int(rawBucket * 1000)
        let abbrNumberOfSecondsInAWeek = 604

        switch bucket {
        case 0 ... abbrNumberOfSecondsInAWeek:
            workoutsPerWeek[0]! += 1
        case abbrNumberOfSecondsInAWeek ... (abbrNumberOfSecondsInAWeek * 2):
            workoutsPerWeek[1]! += 1
        case abbrNumberOfSecondsInAWeek * 2 ... (abbrNumberOfSecondsInAWeek * 3):
            workoutsPerWeek[2]! += 1
        case abbrNumberOfSecondsInAWeek * 3 ... (abbrNumberOfSecondsInAWeek * 4):
            workoutsPerWeek[3]! += 1
        case abbrNumberOfSecondsInAWeek * 4 ... (abbrNumberOfSecondsInAWeek * 5):
            workoutsPerWeek[4]! += 1
        case abbrNumberOfSecondsInAWeek * 5 ... (abbrNumberOfSecondsInAWeek * 6):
            workoutsPerWeek[5]! += 1
        case abbrNumberOfSecondsInAWeek * 6 ... (abbrNumberOfSecondsInAWeek * 7):
            workoutsPerWeek[6]! += 1
        case abbrNumberOfSecondsInAWeek * 7 ... (abbrNumberOfSecondsInAWeek * 8):
            workoutsPerWeek[7]! += 1
        case abbrNumberOfSecondsInAWeek * 8 ... (abbrNumberOfSecondsInAWeek * 9):
            workoutsPerWeek[8]! += 1
        case abbrNumberOfSecondsInAWeek * 9 ... (abbrNumberOfSecondsInAWeek * 10):
            workoutsPerWeek[9]! += 1
        default:
            break
        }
    }

    // Run through each bucket and see how many times the user hit their goal
    for i in 0...10 {
        if(workoutsPerWeek[i]! > target) {
            numberOfGoodBuckets += 1
        } else {
            break
        }
    }
    return numberOfGoodBuckets
}

First, I would replace the dictionary

var workoutsPerWeek = [Int: Int]()

by an array. A dictionary could be memory-saving if you have "few" keys in a "large range",
e.g. 1, 20, 300, 4000, as a "sparse-array" emulation. But that is not the case here:
The bucket numbers range from 0 to 9, so that an array is more appropriate,
easier to initialize, and easier to access (no forced unwraps needed).

It is unclear in your code how many entries are needed: You
initialize entries for 0...100, update entries for 0...9, and finally
evaluate entries 0...10. If you want to collect the data for the last 10 weeks then it would be

var workoutsPerWeek = Array(repeating: 0, count: 10)

Now you can use the computed bucket number as an index into the array and
replace the switch statement by

let rawBucket = Int((startDateInterval - workoutTimeInterval) / numberOfSecondsInAWeek)
if rawBucket >= 0 && rawBucket < workoutsPerWeek.count {
    workoutsPerWeek[rawBucket] += 1
}

The range check for the bucket number can also be done as

if workoutsPerWeek.indices.contains(rawBucket) { ... }

There are more things which can be improved: In

let workoutTimeInterval = Double((userWorkout.completionDate?.timeIntervalSinceNow)!)

the conversion to Double is not needed. And what if completionDate is nil?
Your code would crash in that case.

But my main point of criticism is how the bucket is computed.
A day does not necessarily have 24 hours. In regions with daylight saving time,
a day can have 23 or 25 hours if the clocks are adjusted one hour back or forward.
Applied to your code: A week does not necessarily have 604,800 seconds.

The proper way to compute calendar differences is to use the Calendar methods
and DateComponents:

for userWorkout in userWorkouts {
    // Ensure that completionDate is set, otherwise ignore this entry
    guard let completionDate = userWorkout.completionDate else { continue }

    // Compute #of weeks between completionDate and startDate
    let weeksAgo = Calendar.current.dateComponents([.weekOfMonth], from: completionDate, to: startDate).weekOfMonth!

    // Update corresponding bucket
    if workoutsPerWeek.indices.contains(weeksAgo) {
        workoutsPerWeek[weeksAgo] += 1
    }
}

Finally, the calculation of the number of consecutive weeks where the goal has been reached can be simplified to

let numberOfGoodBuckets = workoutsPerWeek.prefix(while: { $0 >= target }).count


Using JavaScript-bitwise AND and -bitwise NOT for setting a number to 0 Obviously is the XOR-operator is used in low-level programming-languages used a lot for setting a value back to all bits 0.

For example:

11010 ^ 11010 => 00000

Brought my teacher to figuring a out few exercises.

One is:
"Set the bits of a number back to 0 by using just the bitwise AND (&) and the bitwise NOT (~) operators."

I've got now this solution:

let readline = require('readline-sync');

const limit = 100;

// THAT THE PART WHICH IS IMPORTANT TO ME !!

function setNumberToNull(num) {
  // First invert the bits of the number.
  // Then compare every bit with the not
  // inverted number using an bitwise and.
  return num & (~num);
}

// ########################################

function testSetNumberToNull(limit) {
  let i;

  for (i = 0; i < limit; i++) {
    let testNum = Math.floor(Math.random() * 10000);
    let tmp = setNumberToNull(testNum);

    if (tmp) {
      return { test : i, message : testNum + ' caused failure.' };
    }
  }

  return { test : i, message : 'All tests passed.'};
}

console.log(testSetNumberToNull(limit).message);

I works fine. Nevertheless I would appreciate your review.

Well, yes, num & (~num) does result in zero. Not much to review there. Seems you've solved the task you were given.

Of course, one might as well return/assign a literal zero, and skip two bitwise operations.

There's one JavaScript-specific caveat to all of this though: JavaScript's bitwise operations only work on 32-bit integers. However, that's not a datatype that usually exists in JavaScript: Numbers in JavaScript are all IEEE 756 64-bit floats.

The moment you use a bitwise operator on them, however, they are converted (with potential loss) to 32-bit ints, and the result then represented as a float again.

Of course, that's all academic, since whatever int representation you get, a & (~a) will result in zero.

But other bitwise operations can give strange results. For instance, OR'ing with zero should just return the number unaltered, but if the number's too large, its most-significant bit will end up being interpreted as the sign bit as the internal 32-bit conversion is always signed. So you get things like this:

2147483647 | 0 // => 2147483647
2147483648 | 0 // => -2147483648

The second number is large enough to flip the high bit, and thus it's interpreted as negative.

Anyway, point is: Yes, a & (~a) will result in zero, for any value of a, despite JS's internal strangeness, because whatever it converts the input to, it also negates. But it's a special case; in other contexts bitwise operations in JS may be treacherous.


"Python AutoClicker" Part 2 I previously posted a question for codereview:
Python Autoclicker
And I received some great feedback and changes to do in the code.
I have done many of the changes mentioned in the answers and here is the final code:
from pynput.keyboard import Controller
from time import sleep

def auto_click(key: str, n_clicks: int, delay: float):
    keyboard = Controller()

    for _ in range(n_clicks):
        keyboard.tap(key)
        sleep(delay)

def main():
    key = input("Key to be autopressed: ")

    try:
        n_clicks = int(input("Number of autopresses (integer): "))
    except ValueError:
        print("\n The number of autopresses should be an integer value, defaulting to 1. \n")
        n_clicks = 1

    try:
        delay = float(input("Delay between each autopress in seconds (integer/float): "))
    except ValueError:
        print("\n The delay between each autoclick should be an integer or a decimal value, defaulting to 1 (second). \n")
        delay = 1

    auto_click(key, n_clicks, delay)

if __name__ == '__main__':
    main()

What is it that you think should be made better in this code? Any feedback and suggestions would be much appreciated.
Here's the github repo:
https://github.com/SannanOfficial/AutoKeyPython

Your code is generally fine. I suspect you are not doing enough validation (for
example, to forbid negative values or multi-character keys). Also, if you want
to expand your toolkit, you can think about eliminating the duplication in your
code by generalizing the input collection process.
Collecting user input involves a few common elements: an input prompt, a
function to convert the string to a value, a function to validate that value,
an error message in the event of bad input, and possibly some other stuff (max
allowed attempts, fallback/default value in the event of bad input). If we
focus on just the basics, we end up with a function along the lines of this
sketch:
def get_input(convert, validate, prompt, errmsg):
    while True:
        try:
            x = convert(input(prompt + ': '))
            if validate(x):
                return x
            else:
                print(errmsg)
        except ValueError:
            print(errmsg)

With that in hand, all of the algorithmic detail will reside in that single
function, and your main() will be little more than straightforward,
step-by-step orchestration code.
def main():
    key = get_input(
        str.strip,
        lambda s: len(s) == 1,
        'Key to be autopressed',
        'Invalid, must be single character',
    )
    n_clicks = get_input(
        int,
        lambda n: n > 0,
        'Number of autopresses',
        'Invalid, must be positive integer',
    )
    delay = get_input(
        float,
        lambda n: n > 0,
        'Delay between autopresses',
        'Invalid, must be positive float',
    )
    auto_click(key, n_clicks, delay)

As shown in that sketch, I would also encourage you to favor a more compact
style in messages to users. Brevity usually scales better across a variety of
contexts than verbosity: it's easier to maintain; it's easier to make
consistent across an entire application; and most people don't appreciate
having to slog through long messages when an equally clear short one
is possible.
Finally, you might also consider whether input() is the best
mechanism to get this information from the user. In my experience,
it is almost always preferable to take such values from the
command-line, either from sys.argv directly or, more commonly,
using a library like argparse. The advantages
to this approach are numerous.


Generic cached IEnumerable I've an IEnumerable<T> source which takes time to generate each item. Once an item is generated, I want to cache it to avoid recomputing the same thing. I want to separate out caching logic from original source.

A few things that bother me are:

Where should I have _cache? In CachedEnumerable or CacheEnumerator?
Thread safety in adding into cache.
Reinventing? Any existing .NET or other library which does this.
Why Dispose in IEnumerator? Should I be doing something there? _cache nuke?

class CachedEnumerable<T> : IEnumerable<T>
{
    private IList<T> _cache = new List<T>();
    IEnumerator<T> _cacheEnumerator = null;
    IEnumerable<T> _source = null;

    public CachedEnumerable(IEnumerable<T> source)
    {
        _source = source;
        _cacheEnumerator = _source.GetEnumerator();
    }

    public IEnumerator<T> GetEnumerator()
    {
        return new CachedEnumerator<T>(_cache, _cacheEnumerator);
    }

    System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

class CachedEnumerator<T> : IEnumerator<T>
{
    private IList<T> _cache;
    private IEnumerator<T> _source;
    private T _current;
    private int _index;
    private bool _moveNextStatus;

    public CachedEnumerator(IList<T> cache, IEnumerator<T> source)
    {
        _cache = cache;
        _source = source;
        Reset();
    }

    public T Current
    {
        get
        {
            if (_moveNextStatus)
            {
                return _current;
            }
            else
            {
                throw new InvalidOperationException();
            }
        }
    }

    public void Dispose()
    {
        _source.Dispose();
    }

    object System.Collections.IEnumerator.Current
    {
        get { return Current; }
    }

    public bool MoveNext()
    {
        _moveNextStatus = _MoveNext();
        return _moveNextStatus;
    }

    private bool _MoveNext()
    {
        _index++;

        if (_index < _cache.Count)
        {
            _current = _cache[_index];
            return true;
        }
        else
        {
            if (_source.MoveNext())
            {
                _current = _source.Current;
                _cache.Add(_current);
                return true;
            }
            else
            {
                return false;
            }
        }
    }

    public void Reset()
    {
        _index = -1;
        _current = default(T);
        _moveNextStatus = false;
    }
}

You need to access _cache in both classes, so it makes sense to have it as field in both of them too.
Your code is not thread safe. If you wanted to make it thread safe, I think the best solution would be to have a lock that you use whenever you access _cache or _source.
I don't think anything like this is in .Net.
What you're currently doing in Dispose() is wrong. You can't call Dispose() on _source and expect to use it later in another enumerator. In your case, I think you shouldn't do anything in Dispose(). You certainly shouldn't “nuke” _cache, because you're going to need it later!


Circular dependency in utility functions Some Background

I've been developing an ESLint plugin to catch Protractor/Jasmine specific code style violations.

In Protractor, there are two important notations: ElementFinder and ElementArrayFinder.

ElementFinder is a special object that corresponds to a single element

is typically defined via element(by.smth("")) or $("css selector").

ElementArrayFinder is a special object that corresponds to an array of elements

is typically defined via element.all(by.smth("")) or $$("css selector")

And, all of these notations can be chained. ElementFinder can also be retrieved from an ElementArrayFinder via get(), first() or last() methods, e.g.:

element.all(by.className("someClass")).element(by.tagName("td"));

The Task

In multiple ESLint rules I've been developing so far, I needed to locate the ESLint Call Expressions that are either ElementFinder or ElementArrayFinder.

Currently, I've come up with two utility functions that are circularly dependent on each other:

is-element-finder.js:

'use strict'

/**
 * Checks if a given node is an ElementFinder instance.
 *
 * @fileoverview Utility function to determine if a node is an ElementFinder
 * @author Alexander Afanasyev
 */

var elementArrayFinderGetMethods = [
  'get',
  'last',
  'first'
]
var elementFinderConstructors = [
  '$',
  'element'
]

module.exports = function (node) {
  var callee = node.callee
  if (!callee) {
    return false
  }

  // handling raw $ and element
  if (elementFinderConstructors.indexOf(callee.name) > -1) {
    return true
  }

  // handling $ and element chaining
  if (callee.type === 'MemberExpression' && callee.property) {
    if (elementFinderConstructors.indexOf(callee.property.name) > -1) {
      return true
    }

    // got element finder from element array finder
    var isElementArrayFinderMethod = elementArrayFinderGetMethods.indexOf(callee.property.name) > -1
    if (isElementArrayFinderMethod && callee.object && isElementArrayFinder(callee.object)) {
      return true
    }
  }

  return false
}
var isElementArrayFinder = require('./is-element-array-finder')

is-element-array-finder.js:

'use strict'

/**
 * Checks a given CallExpression node is an ElementArrayFinder instance.
 *
 * @fileoverview Utility function to determine if a node is an ElementArrayFinder
 * @author Alexander Afanasyev
 */

module.exports = function isElementArrayFinder (node) {
  var callee = node.callee

  if (callee) {
    // handling $$ shortcut
    if (callee.name === '$$') {
      return true
    }

    // handling element.all() and nested .all()
    if (callee.property && (callee.property.name === 'all' || callee.property.name === '$$')) {
      if (callee.object) {
        if (callee.object.type === 'Identifier') {
          return callee.object.name === 'element'
        }

        if (callee.object.type === 'CallExpression') {
          return isElementFinder(callee.object) || isElementArrayFinder(callee.object)
        }
      }
    }
  }

  return false
}
var isElementFinder = require('./is-element-finder')

The argument node here is a CallExpression ESLint object.

The Problem

These two functions successfully pass all of my tests and locate ElementFinder and ElementArrayFinder instances.

But, because they both can be chained, each of the functions need to call another, which leads to a circular dependency. How would you approach removing the circular dependency? I would also appreciate any comments about the code style and readability.

Note: using Standard JS style.

I understand that there is a bit of a context in order to understand the problem, but I am open to elaborate more or add any additional information.

Circular dependency

Although I find this circular dependency fishy, I don't know how to remove it. Maybe it's not possible.

However, as long as the circular dependency exists,
it would be better if the two functions were defined in the same file.
I don't like having them in separate files because:

It can be misleading, suggesting that one file is usable without the other, but it's not
The elementArrayFinderGetMethods and the conditions on it in is-element-finder violates the encapsulation of isElementArrayFinder

Readability

Symmetry helps readability.
In isElementFinder you use early return on callee,
but not in isElementArrayFinder.

You could further reduce the nesting by lifting a callee.object condition to one level higher:

  if (callee.object && callee.property && (callee.property.name === 'all' || callee.property.name === '$$')) {
    if (callee.object.type === 'Identifier') {
      return callee.object.name === 'element'
    }

    if (callee.object.type === 'CallExpression') {
      return isElementFinder(callee.object) || isElementArrayFinder(callee.object)
    }
  }

The callee.property && (callee.property.name === 'all' || callee.property.name === '$$') condition could be a good candidate to extract to a helper method.


Combinatoric problem in Haskell I am new to functional programming. Just now I solved my first "real world" task using Haskell and now I'm wondering whether I got it "right" and what can be improved.

The problem arises from tensor calculations (I am a theoretical physicist), where I have the tensor product of \$\frac{n}{2}\$ identical symmetric tensors and I am looking for all indistinguishable distributions of \$n\$ indices, i.e. terms like \$\eta^{a c} \eta^{b e} \eta^{d f}\$.

The combinatorics is easy, there are \$(n-1)\times(n-3)\times\dots\times1\$ different terms and prescriptions to enlist them all are easy to conceive.

I implemented the following algorithm:

input: string of even length ("indices")

output: rose tree representing all permutations as described above, a node is a pair (my implementation: two-element list) of indices, each branch from root node to leaf is one permutation

recursive construction:
From string of length \$n\$ (example: "abcdef"), construct \$n-1\$ nodes and "remainders", where the first index of each node is the first index of the input string (example: [("ab", "cdef"), ("ac", "bdef"), ("ad", "bcef"), ("ae", "bcdf"), ("af", "bcde")]). Repeat with all "remainders".

Now this is my implementation in Haskell:

import Control.Applicative
import Data.Tree

--- given string of n indices a:is,
--- construct all (n-1) two-character
--- strings a:i:[] and the reduced
--- string of (n-2) remaining indices
--- undefined if n == 1
innerEtas :: String -> [(String, String)]
innerEtas (x:[]) = undefined
innerEtas (x:xs) = map (\a -> ([x, a], filter (/=a) xs)) xs
innerEtas [] = []

--- build up tree by recursive application
--- of innerEtas
buildEtas :: String -> Forest String
buildEtas [] = []
buildEtas xs = let ys = innerEtas xs
               in map (\(node, list) -> Node node $ buildEtas list) ys

--- flatten Tree to list of strings
treeToStrings :: Tree String -> [String]
treeToStrings (Node n []) = [n]
treeToStrings (Node n ts) = liftA2 (++) [n] (forestToStrings ts)

forestToStrings :: Forest String -> [String]
forestToStrings ts = concat $ map treeToStrings ts

--- build ansatz tree from n indices
ansatzEtas :: Integer -> Forest String
ansatzEtas n
  | n < 0 = undefined
  | n `mod` 2 /= 0 = undefined
  | otherwise = buildEtas $ take (fromIntegral n) ['a'..'z']

main = do
        putStrLn $ drawForest $ ansatzEtas 8
        print $ forestToStrings $ ansatzEtas 8
        print $ (forestToStrings $ ansatzEtas 26) !! 10000

I am really impressed by Haskells lazy evaluation, which in the last line allows me to obtain certain combinations of ['a'..'z'] without first calculating them all! So I believe I got this aspect right.

So my question is, did I make any style errors? Did I miss idioms of functional programming and Haskell which would make the code better in some ways? Are there issues with space or time?

The algorithm is purely combinatorial: it doesn't depend on the actual values of the elements. That can be made explicit by using more general types. Compare these signatures:

ansatzEtas :: Integer -> Forest String
ansatzEtas :: Integer -> [a] -> Forest (a, a)
-- takes the alphabet as a parameter (length must be >= the first argument n)

The output is guaranteed to be a forest of pairs.
Now that a is a type parameter, we are guaranteed that the pairs contain only elements from the original list.

We can easily get back a forest of strings using the fact that trees are functors:

pairToList :: Forest (a, a) -> Forest [a]
pairToList = (fmap . fmap) (\(x, y) -> [x, y])

More precise types make mistakes much less likely, and can even help us know what to write. For example, imagine we're still trying to write innerEtas, and we want to know what to put in place of the underscore:

innerEtas :: String -> [(String, String)]
innerEtas (x:xs) = map (\a -> (_, filter (/=a) xs)) xs

We can try to compile this code as it is, with the underscore, and GHC will report that this hole is of expected type String.

• Found hole: _ :: String

It also helpfully tells us that xs :: [Char] is in scope. On a tired day, one might then mistakenly interpret that as advice to put xs in:

innerEtas (x:xs) = map (\a -> (xs, filter (/=a) xs)) xs
                               -- Bug!

Looking back, before we even start writing innerEtas, we know that it's supposed to give us pairs, and we can say it in the type.

innerEtas :: [a] -> [((a, a), [a])]
innerEtas (x:xs) = map (\a -> (_, filter (/=a) xs)) xs

Here we would be told the following expected type:

• Found hole: _ :: (a, a)

Which is hopefully much more helpful to determine what goes where.

This generalization may introduce a bit of a challenge, because innerEtas uses (/=), which requires an Eq constraint. Two options:

Just add the Eq a constraint everywhere, this is an easy solution, we still get most of the benefits of the generalization anyway.
Rewrite innerEtas to not use (/=), instead write a custom recursive function or use other combinators than filter. This can help avoid mistakes when making heavy use of Eq, but arguably this is not the case here.

Still, in this case there's a cute one-liner to extract an element from a list and keep the rest, nondeterministically, without using Eq:

pick :: [a] -> [(a, [a])]
pick xs = [(y, ys ++ ys') | (ys, y : ys') <- liftA2 zip inits tails xs]
-- inits, tails from Data.List

-- Example
pick [1,2,3] = [(1,[2,3]),(2,[1,3]),(3,[1,2])]

Some refactoring is possible between innerEtas and buildEtas. There is redundancy because both are checking whether the list is empty. In fact, x is quite a bit more special than the other elements, and we can take it apart in buildEtas already. Then innerEtas could take x separately from the tail as an argument, but it becomes a one-liner that we might as well inline:

buildEtas :: [a] -> Forest (a, a)
buildEtas [] = []
buildEtas (x : []) = undefined   -- I think this is unnecessary, but kept it to remain close to the original code
buildEtas (x : xs) = let ys = map (\(y, zs) -> ((x, y), zs) (pick xs)
                     in map (\(node, list) -> Node node $ buildEtas list) ys


Average interval between dates with random blanks I have a spreadsheet with order date data:

I need to find the average interval in days between each order date.  I have to both find a way to get past the blank cells in the row, and also take into account that some clients have 5-10 orders and some clients have 2 orders when calculating my average frequency (interval) between orders.

Sub DateInt()
    Dim CurrentSheet As Worksheet
    Dim LastRow As Integer
    Dim LastCol As Integer
    Dim CurrentRow As Integer
    Dim CurrentCol As Integer
    Dim GrandT As String
    Dim DateA As Date
    Dim DateB As Date
    Dim DateTtl As Integer
    Dim DateCount As Integer

    Set CurrentSheet = ActiveWorkbook.ActiveSheet
    LastRow = CurrentSheet.Range("A" & Rows.Count).End(xlUp).Row - 1
    LastCol = CurrentSheet.Cells(4, Columns.Count).End(xlToLeft).Column

    Cells(4, LastCol + 1).Value = "Avg Interval"
    Cells(4, LastCol + 2).Value = "Days Since Last Order"
    Cells(4, LastCol + 3).Value = "Last Order Date"
    Cells(4, LastCol + 4).Value = "Last Order v Avg Order"

    For CurrentRow = 5 To LastRow
        Cells(CurrentRow, LastCol).Value = Date
        Cells(CurrentRow, LastCol).NumberFormat = "mm/dd/yy"
        DateCount = 0
        DateTtl = 0
        DateC = DateAdd("d", 20, Date)

        For CurrentCol = 2 To LastCol
            If Cells(CurrentRow, CurrentCol).Value = "" Then
            Else
                If DateCount < 1 Then
                    DateA = Cells(CurrentRow, CurrentCol).Value
                Else
                    DateB = Cells(CurrentRow, CurrentCol).Value
                    DateTtl = DateDiff("d", DateA, DateB) + DateTtl

                    If DateValue(DateB) = DateValue(Date) Then
                    Else
                        DateA = DateB
                    End If
                End If

                DateCount = DateCount + 1
            End If
        Next CurrentCol

        Cells(CurrentRow, LastCol + 1).Value = DateTtl / DateCount
        Cells(CurrentRow, LastCol + 1).NumberFormat = "General"
        Cells(CurrentRow, LastCol + 2).Value = DateDiff("d", DateA, Date)
        Cells(CurrentRow, LastCol + 2).NumberFormat = "General"
        Cells(CurrentRow, LastCol + 3).Value = DateA
        Cells(CurrentRow, LastCol + 3).NumberFormat = "mm/dd/yy"
        Cells(CurrentRow, LastCol + 4).Value = Cells(CurrentRow, LastCol + 1).Value - Cells(CurrentRow, LastCol + 2).Value
        Cells(CurrentRow, LastCol + 4).NumberFormat = "#,##0_);[Red](#,##0)"
        Next CurrentRow

End Sub

Here is what my code ended up doing:

One thing that stands right out is formatting cells in each iteration of a loop. Less to do within each iteration should speed things up just a bit so why not just format the entire column after the loop has done all the calculations?

I am not sure sure if you are working with more than one workbook or if that's part of an automated process but

Set CurrentSheet = ActiveWorkbook.ActiveSheet
LastRow = CurrentSheet.Range("A" & Rows.Count).End(xlUp).Row - 1
LastCol = CurrentSheet.Cells(4, Columns.Count).End(xlToLeft).Column

seems like a bit of an overkill. You are not using the CurrentSheet (pascal case vs camel case you know) anywhere in the code but you don't quality the Cells. Why not just:

lastRow = Range("A" & Rows.Count).End(xlUp).Row - 1
lastCol = Cells(4, Columns.Count).End(xlToLeft).Column

Have you heard of offsets? Experience taught me that it's easier to maintain offsets rather than +1s +2s etc.

Cells(CurrentRow, LastCol + 1).Value = DateTtl / DateCount

=

Cells(currentRow, lastCol).Offset(0,1) = DateTtl / DateCount


Time and space complexity of base conversion code I have an algorithm that performs the base conversion. The input is a string and two ints b1 and b2. The string represents an int in base b1 and converts it into base in b2.

My code is:

def convert_base(num_as_string, b1, b2):
    if num_as_string == '0' or num_as_string == '-0':
        return num_as_string
    base10, degree = 0, 0
    is_neg = False
    if num_as_string[0] == '-':
        is_neg, num_as_string = True, num_as_string[1:]
    if b1 == 10:
        base10 = int(num_as_string)
    else:
        for digit in num_as_string[::-1]:
            base10 += string.hexdigits.index(digit.lower()) * (b1 ** degree)
            degree += 1
    if b2 == 10:
        return '-' + str(base10) if is_neg else str(base10)
    converted = []
    while base10 > 0:
        digit = base10 % b2
        converted.append(digit)
        base10 //= b2
    res = ''
    for i in converted[::-1]:
        res += string.hexdigits[i].upper()
    return '-' + res if is_neg else res

Now I am trying to see if this can be improved in terms of time and space complexity. But I am not sure how to analyze the complexity of this code.

I know everything is constant before for digit in num_as_string[::-1]:. In this loop, it's just `O(n) where n is just number of digits of the input.

Then in while base10 > 0, it runs the look while base10 becomes 0. So, this would be something like O(number of digits in base10)

Finally, on for i in converted[::-1], this would also be O(number of digits in base10).

So, I am assuming the time complexity of this code is something like O(n) + 2 * O(number of digits in base10) which is linear.

For space, I am assuming it's O(number of digits in base10) because I store it in converted list.

Are my observations correct? If not, what am I missing? Also, can this code be improved in terms of time and space complexity?

Thanks

Much of the following feedback is not really performance-related, but will make the code more easily legible and easily analyzed for the purposes of performance improvement.

Add type hints

b1 and b2 are presumably int, so add : int after their declaration. num_as_string and the function's return value are both str.

Multiple assignment

...has its limited applications, and in this case:

is_neg, num_as_string = True, num_as_string[1:]

there's not really an advantage to combining these two statements. Just do them separately, for legibility.

Exponentiation

You do this:

        base10 += string.hexdigits.index(digit.lower()) * (b1 ** degree)
        degree += 1

But rather than maintaining degree as a number that increases linearly, it's probably a better idea to maintain a number that increases by a multiplicative factor of b1 on every iteration.

Separation of concerns

Fundamentally, convert_base is doing two different things: parsing a string given a particular base into a number, and formatting a number given a particular base into a string. So just make two functions. It's more useful, testable, legible and maintainable.


Print strings in order with dynamic arrays I accomplished the following task to practice allocating on the heap.

reading words from stdin
sorting them in lexicographical order
printing the sorted words to stdout

To accomplish this is allocate the size of each word and the count of words.

I would like to hear any suggestions for improvements. Is there maybe an easier way to sort the words?

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

struct Word {
    char* signs;    // pointer to a word
    int sz;
    int capacity;
};

// allocates factor 2 of the current capacity
void reserve_space_for_letter(char **c, int *capacity)
{
    if (*capacity == 0) {   // allocate the first time
        *capacity = 1;
        *c = malloc(sizeof(char) * ((*capacity)));
    }
    else {
        *capacity *= 2;     // double the new capacity
        *c = realloc(*c, sizeof(char) * (*capacity));
    }
}

void reserve_space_for_words(struct Word** w, int *capacity)
{
    if (*capacity == 0) {   // allocate the first time
        *capacity = 1;
        *w = malloc(sizeof(struct Word) * ((*capacity)));
    }
    else {
        *capacity *= 2;     // double the new capacity
        *w = realloc(*w, sizeof(struct Word) * (*capacity));
    }
}

void print_words(struct Word* words,int sz)
{
    int i = 0;
    for (i = 0; i < sz; ++i) {
        printf("%s ", words[i].signs);
    }
}

int cmp(void const *lhs, void const *rhs) {
    struct Word left = *(const struct Word *)lhs;
    struct Word right = *(const struct Word *)rhs;

    return strcmp(left.signs, right.signs);
}

void deallocate_words(struct Word* words, int sz)
{
    int i = 0;
    for (i = 0; i < sz; ++i) {
        free(words[i].signs);
    }
    free(words);
}

int main()
{
    struct Word* words;     // Pointer to several words
    int sz = 0;
    int capacity = 0;

    int ch=0;
    bool in_word = true;

    reserve_space_for_words(&words, &capacity); // reserve space for first word
    ++sz;
    words[sz - 1].capacity = 0;
    words[sz - 1].sz = 0;

    while ((ch = getc(stdin)) != EOF) {

        if (words[sz - 1].sz == words[sz - 1].capacity) {       // if current letter sz = capacity
            reserve_space_for_letter(&words[sz - 1].signs, &words[sz - 1].capacity);
        }

        if (!isalnum(ch) && in_word) {      // end of current word;
            words[sz - 1].signs[words[sz - 1].sz] = '\0';
            in_word = false;
            continue;
        }
        else if (!isalnum(ch) && !in_word) {        // not in a word so dont bother doing sth
            continue;
        }
        else if (isalnum(ch) && !in_word) {     // the next word is reached
            in_word = true;

            if (sz == capacity) {       // maximum size of words reached
                reserve_space_for_words(&words, &capacity); // reserve space for first word
            }
            ++sz;       // iterate to the new word
            words[sz - 1].capacity = 0;
            words[sz - 1].sz = 0;

            reserve_space_for_letter(&words[sz - 1].signs, &words[sz - 1].capacity);
        }

        words[sz - 1].signs[words[sz - 1].sz] = ch;    // append the sign in the array
        ++words[sz - 1].sz;
    }
    qsort(words, sz, sizeof(struct Word), cmp);
    print_words(words, sz);
    deallocate_words(words, sz);

    getchar();
    return 0;
}

When allocating an array to hold arbitrary numbers of things, consider using calloc and reallocarray (reallocarray is not standard C, but there isn't a standard analog to calloc).

The reason is that calloc will fail if you try to allocate more memory than size_t can hold. If, for example on a 32-bit system, you decide you want to allocate 500 million of some struct, you could do this:

typedef struct { char a; } little_struct_t;
...
ptr = malloc(500000000 * sizeof(little_struct_t));

and it might work. But later, when you decide that your struct actually needs more stuff, you'll end up with something like this:

typedef struct { char a; int b; bool c } little_struct_t;
...
ptr = malloc(500000000 * sizeof(little_struct_t));

and boom, you've just caused a multiplication overflow without thinking about it. If instead you had used calloc, you'd either see the error when you checked that ptr was not NULL (see my next comment) or a happy segfault when you try to dereference ptr. Instead, now you have a 1705032704-byte ((sizeof(little_struct_t) * half a billion) & 0xffffffff)) chunk of memory, and if you're lucky you'll get a segfault when you write off the end of it. If you're not lucky, you'll corrupt a bunch of other memory before your program crashes.

My second comment is that you should always always check the return value of malloc. If it's not NULL, no harm done. If it is, either you want to crash right now or you want to take a different path. Whatever happens, you don't want your program to happily keep chugging along as though nothing's wrong.

Third, and this isn't super relevant to your example (though I would apply it in deallocate_words after the free call inside the loop), but it's good practice to null out your pointers after you free them. I think I've spent too many words already, so I'll just say that it will simplify debugging and get rid of double free issues in the future.


JavaScript responsive menu (dropdown/dropup) I have the following front-end code for a responsive navigation menu which I thought using in my WordPress website.

Clicking the .burger element should cause adjacent .menu element to appear or disappear, in dropdown or dropup respectively, but without a breakpoint conflict such as this, which I suffer from:

We open a browser window <=959px and open the menu.
We resize the window to >=960px and then we resize back to <=959px.
We see that the menu is still open and isn't restarted.

Why I seek review

Many programmers advised me to take a pure CSS approach (no JavaScript) but in this case I create my markup with a WordPress page builder (Elementor) hence it feels inefficient to me. I also feel my JavaScript is too long and a bit confusing.

You might know a simpler and shorter (more methodal and more intuitive) pattern in vanilla JavaScript available in 2018 (ES6/ES7?) or in jQuery 3.x.x.

Code

document.addEventListener('DOMContentLoaded', ()=>{
    let clicks = 0;
    let menu = document.querySelector('#menu-primary');
    let burger = document.querySelector('.burger');
    let isMenuVisible = false;

    burger.addEventListener('click', ()=>{
      isMenuVisible = !isMenuVisible;
      menu.style.display = isMenuVisible ? 'block' : 'none';
    });

    let mobileBehavior = ()=>{
      menu.style.display = 'none';
    };

    if (window.innerWidth <= 959) {
      mobileBehavior();
    }

    window.addEventListener('resize', ()=>{
      if (window.innerWidth <= 959) {
        clicks = 1;
      } else if (window.innerWidth >= 960) {
        menu.style.display = 'block';
      }
    });
});
.burger {display: block; text-align: center; color: var(--w); margin-bottom: 0 !important; font-weight: bold}
#menu-primary {display: none}

@media screen and (min-width: 960px) {
 .burger {display: none}
 #menu-primary {display: block}
}

BARS

Homepage

Contact_us

Update

I reported a problem with the code in this StackOverflow session; the problem is that the code can conflict with some page builders and does conflict with Elementor WordPress-Drupal page builder, as described there.

General feedback
Overall it seems somewhat simple and works fine except for the issue I saw you mentioned in a comment on the accepted answer to the related Stack Overflow post, about having to click the menu button twice after resizing it that is caused by a conflict with page builder as explained in question update above.
One thing to consider is getting rid of the variable isMenuVisible - just check the value of menu.style.display when updating the visibility of the menu.
Suggestions
const vs let
I would use const for any value that should not get re-assigned - e.g. menu, burger. That way if you inadvertently re-assign a value to those, an error will be thrown.
Identifying the element used to open the menu
If there really is only one element for opening the menu, why not use an id attribute to find that instead of a class name.
Querying DOM for elements
Using document.getElementById() to access those DOM elements will be quicker1 than using document.querySelector().
Variable clicks
The variable clicks doesn't appear to be used - just set to 0 initially and then updated during the resize event callback. Is that leftover from other code? Or does it control something else in your code?
simpler and shorter patterns
As far as shortcuts using ecmascript-6 features, the only things I could think of would be to remove the block on mobileBehavior (to make it a single line) and using a single argument with only one character (e.g. named something like _) for the arrow functions instead of an empty set of parentheses (though that only saves character for each function).
Method mobileBehavior() and the resize event callback
Since mobileBehavior() is only called once, that functionality could just be moved to the place it is called... Then it doesn't really make sense to have that function. Perhaps it would be suitable to define a function like setDisplayBasedOnWidth, which contains most of the functionality of the resize callback (except setting the display to none when the width is less than the threshold). Then that function can be called initially, as well as after a resize event.
See the updated code below for slight improvements adapted.

document.addEventListener('DOMContentLoaded', _ => {
  const menu = document.getElementById('menu-primary');
  const burger = document.getElementById('burger');

  burger.addEventListener('click', _ => {
    const currentDisplay = menu.style.display;
    menu.style.display = currentDisplay == 'none' ? 'block' : 'none';
  });
  const setDisplayBasedOnWidth = _ => menu.style.display = window.innerWidth <= 959 ? 'none' : 'block';

  window.addEventListener('resize', setDisplayBasedOnWidth);
  setDisplayBasedOnWidth();
});
:root {
  --w: #00ff00;
}

#burger {
  display: block;
  text-align: center;
  color: var(--w);
  margin-bottom: 0 !important;
  font-weight: bold
}

#menu-primary {
  display: none
}

@media screen and (min-width: 796px) {
  .burger {
    display: none
  }
  #menu-primary {
    display: block
  }
}

BARS

Homepage

Contact_us


Find the greatest gcd among the pairs of two equal sized arrays and prints the sum of that pair You are given two arrays AA and BB containing elements each. Choose a pair of elements x,y such that:

x belongs to array AA. y belongs to array BB. gcd(x,y) is the maximum for all pairs x,y.

If there is more than one such pair x,y having maximum gcd, then choose the one with maximum sum. Print the sum of elements of this maximum-sum pair.

NOTE: returns gcd(x,y) the largest integer that divides both x and y.

Constraints:

$$1≤N≤5\cdot 10^5$$
$$1≤A_i≤10^6$$
$$1≤B_i≤10^6$$

Input format:

The first line of the input contains n denoting the total number of elements of arrays AA and BB. Next line contains n space separated positive integers denoting the elements of array AA. Next line contains n space separated positive integers denoting the elements of array BB.

Output format:

From all the pairs having maximum gcd , print the sum of one that has the maximum sum.

public class Solution2 {

static int gcdcalc(int x, int y){
   if(y == 0)
   {
       return x;
   }
   return gcdcalc(y, x%y);
}

static int maximumGcdAndSum(int[] A, int[] B) {
    int gcd,sum,maxgcd=0,maxsum=0;
    for(int A_i = 0; A_i < A.length; A_i++)
    {
        for(int B_i = 0; B_i < B.length; B_i++)
        {
            sum = A[A_i] + B[B_i];
            gcd = gcdcalc(A[A_i], B[B_i]);
            if(maxgcd < gcd)
            {
                maxgcd = gcd;
                maxsum = sum;
            }
            if(maxgcd == gcd)
            {
                if(maxsum < sum)
                    maxsum = sum;
            }
        }
    }
    return maxsum;
}

public static void main(String[] args) {
    Scanner in = new Scanner(System.in);
    int n = in.nextInt();
    int[] A = new int[n];
    for(int A_i = 0; A_i < n; A_i++){
        A[A_i] = in.nextInt();
    }
    int[] B = new int[n];
    for(int B_i = 0; B_i < n; B_i++){
        B[B_i] = in.nextInt();
    }
    int res = maximumGcdAndSum(A, B);
    System.out.println(res);
 }
}

int gcd,sum,maxgcd=0,maxsum=0;

You don't need to declare gcd and sum yet.

    int maxGcd = 0;
    int maxSum = 0;

It's also generally better not to initialize more than one variable per line.  It's not a functional difference.  It's a readability thing.

The Java standard is to capitalize the second and later words.

            sum = A[A_i] + B[B_i];
            gcd = gcdcalc(A[A_i], B[B_i]);

This could be

            int sum = A[A_i] + B[B_i];
            int gcd = gcdcalc(A[A_i], B[B_i]);

You don't use these outside the current iteration.  So you can declare and initialize them at the same time.

            if(maxgcd == gcd)
            {
                if(maxsum < sum)
                    maxsum = sum;

This could be

            else if (maxgcd == gcd && maxsum < sum)
            {
                maxsum = sum;

If the maxgcd is less than gcd, then they won't be equal.  So we don't need to check both.  The else makes it so that we only check for equality if not less than.

You don't need two if statements.  One statement with two clauses is sufficient.  Due to the way that short circuit comparisons work, this will be the same efficiency as the original code.

I prefer to have a space between the if and the (.  I find it easier to see that it is an if and not a method call that way.

I would encourage you to add more vertical whitespace.  Again, I find breaking up the statements into logical groups makes it easier to read.

   if(y == 0)
   {
       return x;
   }
   return gcdcalc(y, x%y);

This could be just

   return (y == 0) ? x : gcdcalc(y, x%y);

Same efficiency, just a bit terser.

If you find yourself trying to wring out every bit of efficiency, there is an iterative version of this that is probably faster.


Getting all divisors from an integer I am using mod and my code works.  I am wondering if there is a more efficient way to write a method that returns the total number of divisors for an integer.

function getDivisorsCnt(n){
    var divisors = 0;
    mod = n;
    while (mod > 0){
      if(n % mod === 0){
        divisors++;
      }
      mod--;
    }
    return divisors;
}

Some general remarks:

Variables should be declared explicitly with var, otherwise you
create a global variable.
The indent amount is different between the first level and the
deeper levels.
The spacing is not consistent: while ( vs if(. Generally I would
insert a space at least after keywords, and around parentheses/braces etc.

Your implementation is correct but not efficient, as the number of loop iterations
and remainder operations is equal to the input number.

The count of divisors can be efficiently computed from the
prime number factorization: If
$$
 n = p_1^{e_1} \, p_2^{e_2} \cdots p_k^{e_k}
$$
is the factorization of \$ n \$ into prime numbers \$ p_i \$
with exponents \$ e_i \$, then
$$
 \sigma_0(n) = (e_1+1)(e_2+1) \cdots (e_k+1)
$$
is the number of divisors of \$ n \$, see for example
Wikipedia: Divisor function. Example:
$$
 720 = 2^4 \cdot 3^2 \cdot 5^1 \Longrightarrow
  \sigma_0(720) = (4+1)(2+1)(1+1) = 30 \, .
$$

An implementation in JavaScript would be

function getDivisorsCnt(n){

    var numDivisors = 1;
    var factor = 2; // Candidate for prime factor of `n`

    // If `n` is not a prime number then it must have one factor
    // which is <= `sqrt(n)`, so we try these first:
    while (factor * factor <= n) {
        if (n % factor === 0) {
            // `factor` is a prime factor of `n`, determine the exponent:
            var exponent = 0;
            do {
                n /= factor;
                exponent++;
            } while (n % factor === 0)
            // `factor^exponent` is one term in the prime factorization of n,
            // this contributes as factor `exponent + 1`:
            numDivisors *= exponent + 1;
        }
        // Next possible prime factor:
        factor = factor == 2 ? 3 : factor + 2
    }

    // Now `n` is either 1 or a prime number. In the latter case,
    // it contributes a factor 2:
    if (n > 1) {
        numDivisors *= 2;
    }

    return numDivisors;
}

As an example, getDivisorsCnt(720) requires 720 remainder operations
in your algorithm, but only 8 remainder operations and 6 divisions in this algorithm.


How would I format this JavaScript code so it's more readable How can I format this code so it's more readable?

$(".card").click(function(){
        $(this).stop().animate({
            width:'0px',
            marginLeft: margin+'px',
            opacity: 0.5},
            500,
            function(){
               $(this).siblings('.card').animate({
                   width: width + 'px',
                   marginLeft:'0px',
                   opacity:'1'},500);
            });
});

This is a very subjective question

I am a fan of separating things so each is identifiable and easily maintained in the future. However some will argue this does lead to more code, but more readable code in my opinion.

var animateCallback = function() {
    var props = {
        width: width + 'px',
        marginLeft: '0px',
        opacity: '1'
    };
    $(this).siblings('.card').animate(props, 500);
};
var clickHandler = function() {
    var props = {
        width: '0px',
        marginLeft: margin + 'px',
        opacity: 0.5
    };
    $(this).stop().animate(props, 500, animateCallback);
};
$(".card").click(clickHandler);


Evaluating result for mastermind comparison I'm implementing a Java version of the game Mastermind.

My version uses numbers to represent colors, for example 2315. This function compares a string of numbers to the secret code the object stored. It works, but I'm wondering if this could be done in a more elegant way.

public Answer evaluate(String guess) {
    boolean[] codeUsed = new boolean[code.length()];
    boolean[] guessUsed = new boolean[guess.length()];

    int correct = 0;
    int match = 0;

    // Compare correct color and position
    for (int i = 0; i < code.length(); i++) {
        if (code.charAt(i) == guess.charAt(i)) {
            correct++;
            codeUsed[i] = guessUsed[i] = true;
        }
    }

    // Compare matching colors for "pins" that were not used
    for (int i = 0; i < code.length(); i++) {
        for (int j = 0; j < guess.length(); j++) {
            if (!codeUsed[i] && !guessUsed[j] && code.charAt(i) == guess.charAt(j)) {
                match++;
                codeUsed[i] = guessUsed[j] = true;
                break;
            }
        }
    }

    return new Answer(guess, correct, match, guess.length() - correct - match);
}

The algorithm is short and concise. There might be a more clever way to do it, but it would likely be less readable.

This code will break if the guess is not the same length as the code. An empty string causes an out of out of bounds exception. A string longer than the code could cause other problems. You don't show what Answer is, but it looks like the last argument is the number of incorrect pins. This could lead to having more incorrect pins than there are in the code. You need to protect against each of these cases.

Since you are using strings, the value could contain characters other than digits. A guess with invalid characters should be treated differently than an incorrect guess.

You can add a check before the second loop to see if the all of the characters are correct. In that case you would skip over a N x M loop.


Efficient string concatenations with DOM I form DOM nodes as strings and append them to DOM tree like below using jQuery.

var dom = '

first name
'
             '
last name

';

$("#contacts").append(dom);

The above code is a small sample. In most of my cases dom will hold big strings.

When I recently read about JS performance tutorials, I saw this post. It mentioned that this way of string concatenation is not a good practice. It mentioned the use of .join() instead of concatenation. That seems like an old post, so which one is efficient in these days?

Strings are immutable. It means that when you allocate memory while creating a string, you are not able to reallocate it.
So, this code:

var a = 'a';
a = a + 'bc';

Will allocate a new block of memory for 'a' variable, instead of reusing already allocated.

When you do something like 'a' + 'b' + 'c' + 'd' each concatenation allocates a new block of memory for every temporary string.

When you use something like this:

var a = 'a';
a = [a, 'b', 'c'].join('');

join function calculates memory for result string and allocates it only once for a complete string.

As it is shown in the post that you mentioned, it is easier to handle and to avoid memory leaks in JS interpreter (it seems that IE6 and IE7 just not handle garbage collection for the first variant correctly).

If you are interested for the speed of string concatenation for different browsers you can try and view it here.

UPD: Not advocating join, just trying to explain why it was assumed as an optimized variant. As seen at jsPerf tests new browsers optimize string concatenation, so it is faster.


Implementing multiple custom sorts The following code works, but I'm sure there's a cleaner way to implement it and I'd appreciate any guidance on how to go about that.

I have a class Result which I want to sort sometimes by score (which is just an int) and sometimes by parts which are strings whose first char is a digit in [1,7] and whose second char is one of C, D, H, S, N (which are in alphabetical order except that the N needs to come last).

public class Result : IEquatable<Result>, IComparable<Result>
{
    private int score;// Unique NS value for all par contracts.
    private string pars;// All possible pars on this deal.

  // Default comparer for Result type.
    public int CompareTo(Result compareResult)
    {
        // A null value means that this object is greater.
        if (compareResult == null)
            return 1;

        else
            return this.Score.CompareTo(compareResult.Score);
    }
    public override int GetHashCode()
    {
        return score;
    }

    public bool Equals(Result other)
    {
        if (other == null) return false;
        return (this.Score.Equals(other.Score));
    }

    public override string ToString()
    {
        return "Score: " + Score + "   Par: " + Pars;
    }
};

public class InitArray
{
    public static readonly char[] bridgeChar = new char[Constants.CHAR_MAX];

    static InitArray()
    {
        for (int i = 0; i < bridgeChar.Length; i++)
        {
            bridgeChar[i] = Char.ToUpper((char)i);
        }
        //Only concerned with ordering C, D, H, S, N=NT.
        bridgeChar['N'] = bridgeChar['Z'];
    }
};

static void Main(string[] args)
{

    Result result = new Result();

    // Create a list of results.
    List<Result> results = new List<Result>();

    // Add results to the list.
    results.Add(new Result() { Score = 110, Pars = "3C" });
    results.Add(new Result() { Score = -200, Pars = "4Hx" });
    results.Add(new Result() { Score = -100, Pars = "3Hx" });
    results.Add(new Result() { Score = -100, Pars = "3Sx" });
    results.Add(new Result() { Score = 400, Pars = "3N" });

    // Sort on the list using the default comparer.

    results.Sort();

    // Sort calling the Sort(Comparison(T) overload using
    // an anonymous method.
    results.Sort(delegate (Result x, Result y)
    {
        if (x.Score == 0 && y.Score == 0) return 0;
        else if (x.Score == 0) return -1;
        else if (y.Score == 0) return 1;
        else return x.Score.CompareTo(y.Score);
    });

    Console.WriteLine("\nAfter sort by score:");
    foreach (Result aResult in results)
    {
        Console.WriteLine(aResult);
    }

    // Sort on pars using an anonymous method.
    results.Sort(delegate (Result x, Result y)
    {
        if ((x == null) && (y == null)) return 0;
        else if (x == null) return -1;
        else if (y == null) return 1;
        else
        {// Compare the two strings.
            if (x.Pars[0].CompareTo(y.Pars[0]) == 0)
            {
                // If the strings start with the same character, sort them by Denomination.
                return InitArray.bridgeChar[y.Pars[1]].CompareTo(InitArray.bridgeChar[x.Pars[1]]);
            }
            else
            { // sort them with ordinary string comparison.
                return x.CompareTo(y);
            }
        }
    });

    results.Reverse(); //Need them in ascending order.

    Console.WriteLine("\nAfter sort by denom:");
    foreach (Result aResult in results)
    {
        Console.WriteLine(aResult);
    }
  }
}

Output is as required:

After sort by score:
Score: -200   Par: 4Hx
Score: -100   Par: 3Hx
Score: -100   Par: 3Sx
Score: 110   Par: 3C
Score: 400   Par: 3N

After sort by denom:
Score: 110   Par: 3C
Score: -100   Par: 3Hx
Score: -100   Par: 3Sx
Score: 400   Par: 3N
Score: -200   Par: 4Hx

Without getting into why you designed your business logic this way (I would've made Pars a separate class or struct, to accommodate the complexity), I guess this is somewhat a decent approach.

By using an IComparer<T>, you can implement whatever logic you need, thus separating the concern of sorting Pars into its own place.

I've used a predefined list of characters (in C#, the simplest list of char is string, but you can use any type that has an IndexOf method) in order to have a single, distinct source of truth for the order of the chars that can be easily modified without breaking anything. I've named it PairOrder for lack of a better name, feel free to name it whatever you like.

using System;
using System.Linq;
using System.Collections.Generic;

public class Program
{
  public static void Main()
  {
    var data = new[] {
      new Result { Score = 110, Pars = "3C" },
      new Result { Score = -200, Pars = "4Hx" },
      new Result { Score = -100, Pars = "3Hx" },
      new Result { Score = -100, Pars = "3Sx" },
      new Result { Score = 400, Pars = "3N" }
    };

    var sortedByScore = data.OrderBy(item => item.Score)
      .ThenBy(item => item.Pars, new ParsComparer());

    Console.WriteLine("Sorting by Score first");
    foreach (var item in sortedByScore)
    {
      Console.WriteLine(item);
    }

    var sortedByPars = data.OrderBy(item => item.Pars, new ParsComparer())
      .ThenBy(item => item.Score);

    Console.WriteLine("\nSorting by Pars first");
    foreach (var item in sortedByPars)
    {
      Console.WriteLine(item);
    }
  }
}

public class Result
{
  public int Score { get; set; }
  public string Pars { get; set; }

  public override string ToString()
  {
    return string.Format("Score: {0}; Pars: {1}", Score, Pars);
  }
}

public class ParsComparer : IComparer<string>
{
  private string ParsOrder = "CDHSN";

  public int Compare(string a, string b)
  {
    var delta = a[0].CompareTo(b[0]);

    if (delta == 0)
    {
      var indexA = ParsOrder.IndexOf(a[1]);
      var indexB = ParsOrder.IndexOf(b[1]);

      delta = indexA.CompareTo(indexB);
    }

    return delta;
  }
}

The output is what you expected:

Sorting by Score first
Score: -200; Pars: 4Hx
Score: -100; Pars: 3Hx
Score: -100; Pars: 3Sx
Score: 110; Pars: 3C
Score: 400; Pars: 3N

Sorting by Pars first
Score: 110; Pars: 3C
Score: -100; Pars: 3Hx
Score: -100; Pars: 3Sx
Score: 400; Pars: 3N
Score: -200; Pars: 4Hx

Disclaimer: it's been a while since I've written any significant C# code. There are probably better ways to write and organize this code.

I also don't make any promises regarding performance. I don't think you'll have any issues, but the IndexOf lookup might introduce a bottleneck, specially if the list of chars gets too big.


Sieve of Eratosthenes in Haskell I've implemented the Sieve of Eratosthenes algorithm in Haskell. Please take a look. Any suggestions are greatly appreciated :)

import Data.List

-- a list identical with {seq} but for multiples of {p} which are replaced by {-1}
sieve_once :: [Integer] -> Integer -> [Integer]
sieve_once seq p = map (\x -> if x `mod` p == 0 then -1 else x) seq

-- a list of all prime numbers
sieve :: [Integer]
sieve = unfoldr (\acc -> let p:rest = dropWhile (\x -> x == -1) acc in Just(p, sieve_once rest p)) [2..]

-- a list of all prime numbers not greater than {n}
sieve_until :: Integer -> [Integer]
sieve_until n = takeWhile (\x -> x <= n) sieve

I'd suggest to filter out unneeded elements, instead of replacing them by -1. Keeping and processing all the -1 is unnecessary:

sieve_once :: [Integer] -> Integer -> [Integer]
sieve_once seq p = filter (\x -> x `mod` p /= 0) seq

-- a list of all prime numbers
sieve :: [Integer]
sieve = unfoldr (\(p:rest) -> Just (p, sieve_once rest p)) [2..]


Printing a staircase of text So basically, a user is prompted to input a number of stairs and a staircase when a guy on it is generated. I know this is sloppy and my variable names are rough, I'm actively working to change them now. I thought I would go ahead and post the working code though.

How can I improve on this? What can I do better to do more with less?

Note: I'm very new to Python and general programming.

def render(num, num3):
    num2 = num - 1
    white_space7 = ""

    white_space = ""
    for x in range(num):
        white_space = white_space + "       "
    c = num
    white_space5 = makeSpace(Num - num2)
    if num3 != 0:
        white_space7 = makeFloor(num3)
        print(white_space + "      *" + white_space5 + "*")
        print(white_space + "   o  *" + white_space5 + "*")
        print(white_space + "  /|\ *" + white_space5 + "*")
        print(white_space + "  / \ *" + white_space5 + "*")
        print(white_space + "*******" + white_space7 + "*")
    else:
        print(white_space + "      *" + white_space5 + "*")
        print(white_space + "   o  *" + white_space5 + "*")
        print(white_space + "  /|\ *" + white_space5 + "*")
        print(white_space + "  / \ *" + white_space5 + "*")
        print(white_space + "*******" + white_space7 + white_space5 + "*")

def makeSpace(num):
    white_space4 = ""
    for x in range(num):
        white_space4 = white_space4 + "       "
    return white_space4

def makeFloor(num):
    white_space6 = ""
    num4 = num + 1
    for x in range(num4):
        white_space6 = white_space6 + "*******"
    return white_space6

Num = raw_input("Number of Stairs:" )
Num = int(Num)

white_space2 = ""
for x in range(Num + 1):
    white_space2 = white_space2 + "       "
print(white_space2 + "*******")
y = Num

white_space2 = ""

for x in range(Num):
    if (x < Num - 1):
        render(y,0)
        y -= 1
    else:
        render(y,Num - 1)
        y -= 1

Conciseness

Python overloads operators, that is * works differently for numbers or numbers and strings. You can use it to shorten some functions of yours:

def makeSpace(num):
    return " " * 7 * num

def makeFloor(num):
    return "*" * 7 * (num + 1)

Global typo

white_space5 = makeSpace(Num - num2)
                         ^

That n should be lowercase as uppercase it refers to a global and referring to a global variable from a function is very bad.

Use your functions

You can use makeSpace once more in render:

def render(num, num3):
    num2 = num - 1
    white_space7 = ""

    white_space = makeSpace(num)
    white_space5 = makeSpace(num - num2)
    # ...

The same goes for the top level code (that should be put in a main function for modularity)

def main():

    Num = raw_input("Number of Stairs:" )
    Num = int(Num)

    white_space2 = makeSpace(Num + 1)
    print(white_space2 + "*******")
    # ...

Variable names

Naming the variables and arguments num or num3 or Num is a very bad idea as it does not tell me about the meaning of what is contained inside.
 I suggest, Num -> staircase_height as an example but all the names can and should be improved.

Avoid reassigning the same variable

You re-assign white_space7 and that gave me headaches when refactoring, reassigning is generally avoided in Python.

Simplify branching

The printing part in render:

print(white_space + "      *" + white_space5 + "*")
print(white_space + "   o  *" + white_space5 + "*")
print(white_space + "  /|\ *" + white_space5 + "*")
print(white_space + "  / \ *" + white_space5 + "*")
if num3 != 0:
    print(white_space + "*******" + white_space7 + "*")
else:
    print(white_space + "*******" + white_space5 + "*")

Extract common code out of branches.

and

for x in range(staircase_height - 1):
    render(staircase_height - x, 0)
render(staircase_height - (x + 1), staircase_height - 1)

Use less variables

You can use a simple subtraction in the loop to easily avoid the y variable:

for x in range(staircase_height):
    if (x < staircase_height - 1):
        render(staircase_height - x,0)
    else:
        render(staircase_height - x,staircase_height - 1)

and:

print(makeSpace(staircase_height + 1) + ("*" * 7))

Further simplify render

Simply right_pad  = makeSpace(1), not the long winded expression that always evaluated to one.


Convert integer to full text string duration I have a function I have written in VB for use in SSRS 2012. It takes an integer and converts it to full text duration - for example 900 becomes "15 minutes", 9000 becomes "2 hours 30 minutes", and 7201 is "2 hours 1 second".

The code works, and is:

Public Function SecondsFullText(ByVal TotalSeconds As Integer) As String

    Dim hours As Integer = new Integer()
    Dim minutes As Integer = new Integer()
    Dim seconds As Integer = new Integer()

    Dim hourString as String = ""
    Dim minuteString as String = ""
    Dim secondString as String = ""

    hours = floor(TotalSeconds / 3600)
    minutes = floor((TotalSeconds mod 3600) / 60)
    seconds = (TotalSeconds mod 3600) mod 60

    If hours = 1 Then
        hourString = Cstr(hours) & " hour"
    Else If hours > 1 Then
        hourString = Cstr(hours) & " hours"
    End If

    If minutes = 1 Then
        minuteString = Cstr(minutes) & " minute"
    Else If minutes > 1 Then
        minuteString = Cstr(minutes) & " minutes"
    End If

    If seconds = 1 Then
        secondString = Cstr(seconds) & " second"
    Else If seconds > 1 Then
        secondString = Cstr(seconds) & " seconds"
    End If

    If hours > 0 and (minutes > 0 or seconds > 0) Then
        hourString = hourString & " "
    End If

    If minutes > 0 and seconds > 0 Then
        minuteString = minuteString & " "
    End If

    return hourString & minuteString & secondString
End Function

My question is: how do I make this better? It seems very clunky, with too many IFs to handle the different possibilities for single/plural, spaces, etc.  I feel that this can be better, but I'm not sure how.

This is called in Report Builder 3.0 / SSRS 2012 with this expression: =Code.SecondsFullText(Parameters!Integer.Value)

You should look into the TimeSpan struct, which provides a nice method TimeSpan.FromSeconds().

Having filled a TimeSpan struct by e.g calling the FromSeconds() method, you can just access its properties Seconds, Minutes and Hours.

Instead of concating strings like you do, you should consider to use a StringBuilder.

To get rid of the If ... Else If you can just use the singular and only if the value is greater than 1 you add a s to the string/StringBuilder.

Based on the .NET Naming Guidelines method parameters should be named using camelCase casing hence TotalSeconds should be totalSeconds

Implementing the mentioned points will look like so

Public Function SecondsFullText(ByVal totalSeconds As Double) As String

    Dim timeSpan As TimeSpan = TimeSpan.FromSeconds(totalSeconds)
    Dim stringBuilder As StringBuilder = New StringBuilder()

    Dim hours As Integer = timeSpan.Hours
    Dim minutes As Integer = timeSpan.Minutes
    Dim seconds As Integer = timeSpan.Seconds

    stringBuilder.Append(ToText(hours, "hour"))
    If hours > 0 AndAlso (minutes > 0 OrElse seconds > 0) Then
        stringBuilder.Append(" ")
    End If
    stringBuilder.Append(ToText(minutes, "minute"))
    If minutes > 0 AndAlso seconds > 0 Then
        stringBuilder.Append(" ")
    End If
    stringBuilder.Append(ToText(seconds, "second"))
    Return stringBuilder.ToString()

End Function

Private Function ToText(value As Integer, singularName As String) As String

    If value = 0 Then Return String.Empty
    If value = 1 Then Return String.Format("{0} {1}", value, singularName)
    Return String.Format("{0} {1}s", value, singularName)

End Function


Use of dictionary for file-saving function I have the following function:

def save_to_file(data, columns_order, save_filename):
    """ Saves data to file
    :param data: data to be saved
    :param columns_order: the order of columns
                          if needed to rearrange
                          else put ""
    :param save_filename: the filename (csv, xls, xlsx)
    :return: True if done or None if error
    """
    try:
        df = pd.DataFrame(data)
        if columns_order:  # Rearrange columns if needed
            df = df[columns_order]
        if save_filename.endswith(".csv"):  # Save as csv
            df.to_csv(save_filename, index=False)
            return True
        elif any(save_filename.endswith(ext) for ext in [".xls", ".xlsx"]):  # Save as xls or xlsx
            df.to_excel(save_filename)
            return True
        else:
            print "Choose between csv, xls, xlsx."
            return None
    except Exception, e:
        print "Save_to_file error:", e
        return None

I don't know if I should leave it like that or put the functions in a dictionary:

func_dict = {".csv": df.to_csv(save_filename, index=False), ".xlsx:": df.to_excel(save_filename)}

Yes, you could reduce duplication (and make it easier to add new exporters) if you used a dictionary to map extensions to functions. However, you couldn't do it quite as you suggest; you would store the result of the method call in the dictionary, not the method itself. Here is one suggestion, using os.path.splitext to get the extension defined in save_filename and functools.partial to supply the additional argument required for CSVs.

from functools import partial
import os

EXPORTERS = {'.xls': pd.DataFrame.to_excel,
             '.xlsx': pd.DataFrame.to_excel,
             '.csv': partial(pd.DataFrame.to_csv, index=False)}

def save_to_file(data, save_filename, columns_order=None):
    """ Saves data to file

    :param data: data to be saved
    :param save_filename: the filename
    :param columns_order: the order of columns
                          if needed to rearrange

    :return: True if done or None if error

    """
    try:
        df = pd.DataFrame(data)
        if columns_order is not None:  # Rearrange columns if needed
            df = df[columns_order]
        _, ext = os.path.splitext(save_filename)
        if ext in EXPORTERS:
            EXPORTERS[ext](df, save_filename)
            return True
        else:
            print "Not a supported filetype: {}".format(ext)
    except Exception, e:
        print "Save_to_file error:", e

Note the following:

I have stored the methods on the class in the dictionary, allowing it to be defined outside the function, so you need to pass the instance df as well as the first argument save_filename;
Rather than force the user to specify "" if the columns_order isn't required, I have given it a default None value; and
return None is implicit.

Including the list of supported filetypes in the docstring and output message is duplication of the information now stored in the dictionary's keys - if you add another supported type, you now need to change three parts of your code.

Finally, you should review your error handling. For one thing, except Exception as e: is more modern than except Exception, e: (which is retained on;ly for backwards compatibility, and doesn't work at all in 3.x). Secondly, you should use more specific error handling - what could go wrong, and how should it be dealt with? At the very least, I would differentiate between:

The conversion to DataFrame or column rearrangement not working; and
A failure in file operations.

Thirdly, rather than return either True or None, it would be conventional to raise an error from your function in the cases where you currently print a message - if the data hasn't been saved, the calling function should do something about it - and return None (again, implicitly) otherwise.


Returning a new object reference when mutation happens i attempted to create a way (not sure if already implemented by someone) by which when i update a property of an object. it returns a new reference of the object with the new value.
i have a usecase in my mind for that purpose you can say i want to use it.

for some example reference you can consider redux reducer where you have to return a new reference everytime store is updated.

here is my small code which attempts to do it.

kindly ignore the setIn implementation its the first attempt.

function NewRef(jsObj) {
  Object.keys(jsObj).forEach((key) => {
    this[key] = jsObj[key];
  });
}

NewRef.prototype.set = function set(key, value) {
  let copyValue = null;
  if (Array.isArray(value)) copyValue = [...value];
  else if (typeof value === 'object') copyValue = { ...value };
  else copyValue = value;
  if (this[key] || ['object', 'boolean', 'number'].includes(typeof this[key])) {
    return Object.assign(Object.create(Object.getPrototypeOf(this)), {
      ...this,
      [key]: copyValue,
    });
  }
  throw new Error(`invalide key ${key} in object`);
};

NewRef.prototype.setIn = function set(nested, value) {
  let copyValue = null;
  if (Array.isArray(value)) copyValue = [...value];
  else if (typeof value === 'object') copyValue = { ...value };
  else copyValue = value;
  if (!Array.isArray(nested)) {
    throw new Error('setIn requires array of strings as first argument');
  } else {
    if (this[nested[0]] || ['object', 'boolean', 'number'].includes(typeof this[nested[0]])) {
      if (this[nested[0]][nested[1]] || ['object', 'boolean', 'number'].includes(typeof this[nested[0]][nested[1]])) {
        return Object.assign(Object.create(Object.getPrototypeOf(this)), {
          ...this,
          [nested[0]]: {
            ...this[nested[0]],
            [nested[1]]: copyValue,
          },
        });
      }
    }
    throw new Error(`invalid properties maybe ${nested.join(',')}`);
  }
};

NewRef.prototype.get = function get(key) {
  return this[key];
};

export default function NObject(jsObj) {
  const store = new NewRef(jsObj);
  return store;
}

with this piece of code i am able to do this.

const d1 = NObject({
   type: 'bool',
   data: {key:'value'}
   meta: {
      user: 'john doe'
   }
  })
const n1 = d1.set('type','mutated').set('data',[1,2])
const n2 = n1.setIn(['meta','user'],'foo bar');

both n1,n2 and d1 have different references. it works and is tested well.

please feel free to review it and give your suggestions. i am open to everything

Mutants are not evil.

I am not going to give a line by line review as I do not see this code as having any useful value. Rather is see it as dangerous as it gives a air of safety when none is present, thus much worse than the supposed monsters you wish to banash, as now you say there are none to worry about (in fact there are more).

First some basics..

Mutation

Object mutation is not a bad thing. In javascript it is a fundamental part of the language and contributes much to what makes it such an enjoyable language to use.

Mutation has turned into a cover all term referring to any change to an object, its structure, or content of its properties. The term is synonymous with bad practice, even assigning a variable is considered mutation and bad these days.

The term "Mutation" should be used to only when changes are made in such a way as to result in a state that can not be trusted to work.

Good and bad mutants

Consider the following simple object

var myNum = {
    valueOf : 0,
    add(num) { this.valueOf += num.valueOf; return this },
    toString() { return Number(this.valueOf).toFixed(2) }
};

// created as follows
var numA = {...myNum, valueOf: 1};
var numB = {...myNum, valueOf: 2};

// use it
console.log(numB.add(numA).valueOf); // >> 3
element.textContent = numB;  // >> 3.00

Good mutants

We can mutate it in a variety of ways that do not effect its operational quality. These mutations must be consciously implemented and as long as all involved are aware of the mutation there is no problem

numA.message = "Foobar"; // add a property
numA.toString = function() { return Number(this.valueOf).toFixed(4) }; //instance output format
myNum.toString = function() { return Number(this.valueOf).toFixed(3) }; //default output format
myNum.value = 2; // change its default state

Bad mutants

However there are mutations that are not deliberate, or there is error in understanding that make the object or template untrustable.

numA = 1; // reassignment
numA.add(1); // improper use
numA.value = "2"; // improper type
numA.add = (num) => { this.valueOf += num; return this }; // ambiguous closure use
myNum.toString = function() { return this.valueOf.toFixed(3) }; // missing procedural logic

Does your code protect against mutants?

const numAO = NObject(numA); // original instance remains
const numA = NObject({...myNum, value: "3"}); // already mutated
// next assigns a mutant property
const numAA = numAO.set("toString",function() { return this.valueOf.toFixed(3) });
// There are many more

So... Does your code protect against mutants? No it does not.

The original reference still remains and can be used. Duplication does not stop mutation
The internal state is not protected and manipulated without using the wrappers methods.
Using the wrappers does not stop mutant assignment that can result in untrusted state.
Each wrapper call creates a new instance, yet does not delete old and likely in use references.

And quite franckly you have added another level of procedure that will have a very negative effect on the source code quality and the coders workload. You will stop using it within a few hundred lines of a project as it is cumbersome.

In terms of readability, massive bloat in source size
In terms of comprehension as it does not provide a clear semantic understanding via its naming and how it is used.
Striking at the heart of a coders need to be lazy (AKA succinct, efficient, and elegant) to much overhead and typing.

Trust is not an option.

To protect against bad mutation you can not rely on trust (trust that the object is used correctly). One mistake at any point in using your wrapper and the whole house of cards falls.

Protection MUST be enforceable. Protection against mutation means that you simply can not create bad mutations.

How to protect against mutants

JavaScript is particularly prone to mutation, it is neither type nor object (class) safe.

Mutation has long been a problem in programming, and there are many ways devized to protect against them. Some good some not so good.

Not so good, Functional programing, relies on trust and is next to useless in my view.
Good, Encapsulation. The best ever as it is enforced by fundamental language constructs.

JavaScript provides a strong encapsulation mechanism that can ensure state safety no matter how much the coder wishes to break the rules.

Closure, encapsulates private properties safe from outside mutation.
Constants const as declared variables are immutable
Object methods to help protected properties eg myNum.toString protect against mutation Object.defineProperty(myNum,"tostring",{value : function() { return Number(this.valueOf).toFixed(2) }})
Getters and Setters create read and write only properties, and define a method of vetting state changes to prevent bad mutation.

Example

Taking the myNum example from above and turn it into an immutable object with enforced state safety and trust

const MyNum = (value) => {
    const error = () => { throw new ReferenceError("value is not a number") }
    const vet = (v, suppressError = true) => // vet state
        typeof (v = Number(v)) === "number" ? v : (
            suppressError ?
                0/0 : // creates trusted NaN
                error()
        );
    value = vet(value, false); // vet new internal state
    const add = num => value += num; // encapsulated method safe from mutation
                                     // and can run in trust (no vetting)
    return Object.freeze({ // Frozen object ensures no tampering
        set valueOf(num) { value = vet(num) },
        get valueOf() { return value },
        toString() { return value.toFixed(2) },
        add(myNum) {
            add(vet(myNum.valueOf, false))
            return this;
        },
    });
}